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

Issue 835742 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Task


Participants' hotlists:
AudioService-FixIt


Sign in to add a comment

Add browser tests testing various audio failure modes.

Project Member Reported by maxmorin@chromium.org, Apr 23 2018

Issue description

https://chromium-review.googlesource.com/c/chromium/src/+/973367 had an erroneous thread check. This wasn't caught by unit tests since they were (reasonably) single threaded, but it also wasn't caught by browser tests, since the code path in question was handling an error, and all of our browser tests verify that audio works in various ways. We should add some browser tests for when audio doesn't work in various ways (stream creation failure, authorization timeout, frame deleted before stream was created, ...).

The problem was eventually caught when noel@ reenabled AudioPlayerBrowserTests in  Issue 835626 . These tests seem to run with PulseAudio on Chrome OS, thus testing various error conditions, but I suspect that will be fixed at some point :).

Putting this on my "later" list for now.
 
Project Member

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

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

commit 9855fd4c1b9a2bb2e14f93aeed278b97f7e0c396
Author: Noel Gordon <noel@chromium.org>
Date: Tue May 08 05:55:24 2018

[ChromeOS] AudioPlayerBrowserTest: use faked audio layer

These AudioPlayerBrowserTest run on a target_os="chromeos" linux build
which attempts to use PulseAudio for audio, and always fails [1].

The tests still PASS, since they don't need to test lower-layer audio,
but they do output PulseAudio LOG failure messages. To avoid that, use
a fake audio layer instead, which can be requested by a media command-
line switch kDisableAudioOutput after crrev.com/539638.

[1] PulseAudio won't work in this case and the LOG output of the tests
indicate that (see  bug 835626  comments #3 and #7).

Bug:  835626 ,835742
Change-Id: I8113c15a123bfb8048141e7a20c8d02cf3341e13
Reviewed-on: https://chromium-review.googlesource.com/1047045
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556704}
[modify] https://crrev.com/9855fd4c1b9a2bb2e14f93aeed278b97f7e0c396/chrome/browser/chromeos/file_manager/audio_player_browsertest.cc

Project Member

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

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

commit 2c490c99574601b697fbddaff50eb58212a11ecb
Author: Noel Gordon <noel@chromium.org>
Date: Wed May 09 00:21:45 2018

[ChromeOS] Cargo cult crrev.com/556704 into VideoPlayerBrowserTest

Again, PulseAudio layer is unsupported in a target_os="chromeos" build
and VideoPlayerBrowserTest produce LOG errors about that. Again, use a
fake audio layer to eliminate it, cargo cult crrev.com/556704.

Good enough until a future CL provides a more general fix, but step #1
is to kick the VideoPlayerBrowserTest's tyres on the bots.

Bug:  835626 ,835742, 835642 
Change-Id: I58cc1c188e95d39f8e75230a761e7b69ff99cabb
Reviewed-on: https://chromium-review.googlesource.com/1049768
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557023}
[modify] https://crrev.com/2c490c99574601b697fbddaff50eb58212a11ecb/chrome/browser/chromeos/file_manager/video_player_browsertest.cc

Project Member

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

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

commit 52c8de92c54cdf8f7afcc2d88ac09024dfb4fede
Author: Noel Gordon <noel@chromium.org>
Date: Wed May 09 23:31:21 2018

FileManagerBrowserTestBase: use fake audio layer in all tests

Move the fake audio layer command-line setup from the AudioPlayer and
VideoPlayer tests to FileManagerBrowserTestBase so it enabled for all
the FileManager browser tests (see  crbug.com/835626#c12 ).

Bug:  835626 , 835642 ,836254,835742
Change-Id: Ib4794a753a4c080fa0544a55c4ead3447c897b68
Reviewed-on: https://chromium-review.googlesource.com/1052047
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557375}
[modify] https://crrev.com/52c8de92c54cdf8f7afcc2d88ac09024dfb4fede/chrome/browser/chromeos/file_manager/audio_player_browsertest.cc
[modify] https://crrev.com/52c8de92c54cdf8f7afcc2d88ac09024dfb4fede/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
[modify] https://crrev.com/52c8de92c54cdf8f7afcc2d88ac09024dfb4fede/chrome/browser/chromeos/file_manager/video_player_browsertest.cc

Project Member

Comment 4 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

Sign in to add a comment