New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 843103 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug


Participants' hotlists:
Audio-Service


Sign in to add a comment

Make remaining tests pass when running audio service OOP.

Project Member Reported by maxmorin@chromium.org, May 15 2018

Issue description

WebRtcGetUserMediaBrowserTest.GetUserMediaFailToAccessAudioDevice, some muting test WebRtcGetUserMediaBrowserTest.GetAudioStreamAndCheckMutingInitiallyUnmuted and friends, and some cast browser tests aren't passing.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/458cd0b27109ad772ea7c1420785beab8d4bbc07

commit 458cd0b27109ad772ea7c1420785beab8d4bbc07
Author: Max Morin <maxmorin@chromium.org>
Date: Wed May 16 13:58:46 2018

Force audio service to be in process in muting tests.

Bug:  843103 
Change-Id: I1040b014620079865b1141c52ac8ec90fec26ab7
Reviewed-on: https://chromium-review.googlesource.com/1061293
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559077}
[modify] https://crrev.com/458cd0b27109ad772ea7c1420785beab8d4bbc07/content/browser/webrtc/webrtc_getusermedia_browsertest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/432064159b1332d60a56bb92a40ef9d6e6b1af19

commit 432064159b1332d60a56bb92a40ef9d6e6b1af19
Author: Max Morin <maxmorin@chromium.org>
Date: Thu May 17 09:17:17 2018

Fix GetUserMediaFailToAccessAudioDevice with out of process audio

This tests tells the audio manager to fail creating an audio stream,
but accessing the audio manager directly isn't possible when running
out of process. Thus, a command line switch is introduced.

I ran the test 100 times for each combination of
(in content streams, in process audio service streams, out of process
audio service streams) and (tsan, release) to ensure it's not flaky in
any way, but note that running it out of process requires updating the
flag whitelist in content::UtilityProcessHost (next CL, since we'll need
a few flags).

Bug: 835742,  843103 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I8a6a659523f39a1c66c3a9ed9d37de9eedbc8265
Reviewed-on: https://chromium-review.googlesource.com/1059594
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559466}
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/content/browser/webrtc/webrtc_getusermedia_browsertest.cc
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/media/audio/audio_manager_base.cc
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/media/audio/audio_manager_base.h
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/media/audio/audio_output_resampler.cc
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/media/base/media_switches.cc
[modify] https://crrev.com/432064159b1332d60a56bb92a40ef9d6e6b1af19/media/base/media_switches.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62d1b5d08068c7310f809243cd7a7e5c4d1c30b7

commit 62d1b5d08068c7310f809243cd7a7e5c4d1c30b7
Author: Max Morin <maxmorin@chromium.org>
Date: Thu May 17 09:42:05 2018

Add audio switches to utility process whitelist.

This CL adds the audio related switches which are used by the audio
service to the whitelisted switches for utility processes. This is
needed when running the audio service out of process.

Also moves all the ifdefed switches to the bottom.

Bug:  843103 
Change-Id: I2172d02b28f5011a915c89d9509dd15ccb166950
Reviewed-on: https://chromium-review.googlesource.com/1059624
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559472}
[modify] https://crrev.com/62d1b5d08068c7310f809243cd7a7e5c4d1c30b7/content/browser/utility_process_host.cc

Cc: guidou@chromium.org m...@chromium.org
There are a couple of test fixes in review at https://chromium-review.googlesource.com/c/chromium/src/+/1062038. The only remaining test after that is TabCaptureApiTest.ApiTests, which Guido is currently looking at. 

Comment 5 Deleted

Comment 6 by guidou@chromium.org, May 17 2018

In TabCaptureApiTest.ApiTests the test that specifically fails is tabIsUnmutedWhenTabCaptured() [1], which checks that when tab capture starts, the tab is unmuted.

I observed that, when run in-process, this is acomplished by the sequence: 
AudioInputController::DoRecord() [2], which invokes
  WebContentsAudioInputStream::Impl::Start() [3], which invokes
    WebContentsAudioInputStream::Impl::UnmuteWebContentsAudio()
      WebContents::SetAudioMuted()

When I run the test with the OOP service, AudioInputController::DoRecord() is never called.





[1] https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/tab_capture/api_tests.js?type=cs&q=api_tests.js&sq=package:chromium&g=0&l=118

Comment 7 by guidou@chromium.org, May 17 2018

Cc: maxmorin@chromium.org
Owner: m...@chromium.org
The tab is unmuted as a result of the renderer process invoking content::MojoAudioInputIPC::RecordStream()
  AudioInputStreamPtr::Record()
  
When the audio service runs in process, the tab is unmuted in the following sequence:
  media::AudioInputController::Record()
    content::AudioInputDelegateImpl::OnRecordStream()
      media::MojoAudioInputStream::Record()
        content::AudioInputDelegateImpl::OnRecordStream()
          media::AudioInputController::Record()
            AudioInputController::DoRecord()
              content::WebContentsAudioInputStream::Impl::Start()
                WebContentsAudioInputStream::Impl::UnmuteWebContentsAudio()  
                  content::WebContentsImpl::SetAudioMuted()

When the service runs OOP, this is handled by 
  audio::LoopbackStream::Record()

but it does not result in a call to content::WebContentsImpl::SetAudioMuted().

This happens in all tests in TabCaptureApiTest.ApiTests, but it only results in failure in tabIsUnmutedWhenTabCaptured(), which specifically checks that the tab is unmuted.

miu@: Can you take a look?

Comment 8 by olka@chromium.org, May 17 2018

Cc: olka@chromium.org

Comment 9 by olka@chromium.org, May 17 2018

Failing tab capture test runs can be found here https://chromium-review.googlesource.com/c/chromium/src/+/1057807

(The other failures there are taken care of, CLs awaiting review)

Comment 10 by m...@chromium.org, May 17 2018

Looks like some of the sound/muting unit_tests fail too. Looking into it...

Comment 11 by olka@chromium.org, May 17 2018

Thanks! Note there is an unlanded CL with a fix for some tests
https://chromium-review.googlesource.com/c/chromium/src/+/1062038

Comment 12 by olka@chromium.org, May 17 2018

That CL fixes SoundContentSettingObserverTest and the other observer one.
So only TabCaptureApi remains
(Any failures on Android/ChromeOS can be ignored)
Project Member

Comment 14 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3bd958dc2cf2a726126eaa5420bbdb3afd281aa

commit a3bd958dc2cf2a726126eaa5420bbdb3afd281aa
Author: Yuri Wiitala <miu@chromium.org>
Date: Thu May 17 23:14:41 2018

Fix 'forced tab unmute' behavior when using OOP Audio Service

The new Audio Service uses separate infrastructure for muting and tab
capture. The legacy implementation, however, did not; and there was some
logic in chrome/browser/ui/tab_utils.cc mitigating that. This change
conditionally allows the mute state of a tab to be toggled/updated
independently of tab capture. It also adds TODOs to mark the logic that
should be deleted once the Audio Service launches.

Bug:  843103 , 824019 ,672469
Change-Id: Ie21f4895717b5769808b87676f723a9ed5a30af4
Reviewed-on: https://chromium-review.googlesource.com/1065065
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559727}
[modify] https://crrev.com/a3bd958dc2cf2a726126eaa5420bbdb3afd281aa/chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc
[modify] https://crrev.com/a3bd958dc2cf2a726126eaa5420bbdb3afd281aa/chrome/browser/ui/tabs/tab_utils.cc
[modify] https://crrev.com/a3bd958dc2cf2a726126eaa5420bbdb3afd281aa/chrome/test/data/extensions/api_test/tab_capture/api_tests.js

Comment 15 by m...@chromium.org, May 17 2018

Status: Fixed (was: Started)

Comment 16 by olka@chromium.org, May 18 2018

\o/
Project Member

Comment 17 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5dcd11bcb43052b089561a443c8f677ceed692d

commit d5dcd11bcb43052b089561a443c8f677ceed692d
Author: Max Morin <maxmorin@chromium.org>
Date: Fri May 18 11:23:59 2018

Fix Chrome unit tests when using the audio service.

WebContentsImpl accesses ServiceManagerConnection when we're
toggling muting using the audio service. This is always set
in production, but in tests we need to make sure that it is
initialized.

Tbr: markusheintz
Bug:  843103 
Change-Id: Ief7819edbeed6616e9e7defb5860b422f6862f9b
Reviewed-on: https://chromium-review.googlesource.com/1062038
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559860}
[modify] https://crrev.com/d5dcd11bcb43052b089561a443c8f677ceed692d/chrome/browser/content_settings/sound_content_setting_observer_unittest.cc
[modify] https://crrev.com/d5dcd11bcb43052b089561a443c8f677ceed692d/chrome/browser/media/media_engagement_contents_observer_unittest.cc

[bulk-edit: disregard if N/A] Can the owner please set milestone to this bug if applicable?

Sign in to add a comment