Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632733002:
media: Use fake audio manager in headless mode (Closed)
Descriptionmedia: Use fake audio manager in headless mode
When Chrome is started in headless mode, fall back to the fake audio
manager instead of trying to use an actual backend like ALSA or
PulseAudio. This avoids the need to have actual audio support in the
headless runtime environment.
BUG=678948
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (8 generated)
Description was changed from ========== media: Use fake audio manager in headless mode When Chrome is started in headless mode, fall back to the fake audio manager instead of trying to use an actual backend like ALSA or PulseAudio. This avoids the need to have actual audio support in the headless runtime environment. BUG=678948 ========== to ========== media: Use fake audio manager in headless mode When Chrome is started in headless mode, fall back to the fake audio manager instead of trying to use an actual backend like ALSA or PulseAudio. This avoids the need to have actual audio support in the headless runtime environment. BUG=678948 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== media: Use fake audio manager in headless mode When Chrome is started in headless mode, fall back to the fake audio manager instead of trying to use an actual backend like ALSA or PulseAudio. This avoids the need to have actual audio support in the headless runtime environment. BUG=678948 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Use fake audio manager in headless mode When Chrome is started in headless mode, fall back to the fake audio manager instead of trying to use an actual backend like ALSA or PulseAudio. This avoids the need to have actual audio support in the headless runtime environment. BUG=678948 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
skyostil@chromium.org changed reviewers: + tommi@chromium.org
The CQ bit was checked by skyostil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:34: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { This is bringing in unit test code into production code. Instead, we should just use the ForTesting factory methods in tests that require it. As is, this would change all tests running in headless mode to use the FakeAudioManager and we'd lose testing of the actual code that works with the audio hardware.
https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:34: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { On 2017/01/13 18:02:27, tommi (chrømium) wrote: > This is bringing in unit test code into production code. Instead, we should > just use the ForTesting factory methods in tests that require it. As is, this > would change all tests running in headless mode to use the FakeAudioManager and > we'd lose testing of the actual code that works with the audio hardware. Sorry, which tests are you referring to? The --headless switch is used by Chrome users who wish to run the browser in headless mode for automation purposes. Is there another way we could avoid requiring audio hardware in such a setup (e.g., a server environment)?
https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:34: if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { On 2017/01/13 18:38:06, Sami wrote: > On 2017/01/13 18:02:27, tommi (chrømium) wrote: > > This is bringing in unit test code into production code. Instead, we should > > just use the ForTesting factory methods in tests that require it. As is, this > > would change all tests running in headless mode to use the FakeAudioManager > and > > we'd lose testing of the actual code that works with the audio hardware. > > Sorry, which tests are you referring to? The --headless switch is used by Chrome > users who wish to run the browser in headless mode for automation purposes. Is > there another way we could avoid requiring audio hardware in such a setup (e.g., > a server environment)? FWIW there's some more background about Headless Chrome here: https://chromium.googlesource.com/chromium/src/+/master/headless/README.md
On 2017/01/13 18:38:06, Sami wrote: > https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... > File media/audio/linux/audio_manager_linux.cc (right): > > https://codereview.chromium.org/2632733002/diff/1/media/audio/linux/audio_man... > media/audio/linux/audio_manager_linux.cc:34: if > (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kHeadless)) { > On 2017/01/13 18:02:27, tommi (chrømium) wrote: > > This is bringing in unit test code into production code. Instead, we should > > just use the ForTesting factory methods in tests that require it. As is, this > > would change all tests running in headless mode to use the FakeAudioManager > and > > we'd lose testing of the actual code that works with the audio hardware. > > Sorry, which tests are you referring to? The --headless switch is used by Chrome > users who wish to run the browser in headless mode for automation purposes. Is > there another way we could avoid requiring audio hardware in such a setup (e.g., > a server environment)? Hmm... are you saying the switch is not for testing but for chrome users? Actually, it would be helpful if you could tell me a little more about what specific problem you want to address. As is, it doesn't feel to me that 'headless' automatically maps to 'no audio hardware'. Running in headless mode can still require audio hardware for some tests (e.g. media_unittests, webrtc and webaudio tests). This particular place to make this check also seems like the wrong layer to be doing this at, especially since this is the linux specific audio manager and not the audio manager in general. Can you perhaps tell me a little more about what specific problem you want to address?
On 2017/01/13 18:58:55, tommi (chrømium) wrote: > Hmm... are you saying the switch is not for testing but for chrome users? > Actually, it would be helpful if you could tell me a little more about what > specific problem you want to address. > > As is, it doesn't feel to me that 'headless' automatically maps to 'no audio > hardware'. Running in headless mode can still require audio hardware for some > tests (e.g. media_unittests, webrtc and webaudio tests). This particular place > to make this check also seems like the wrong layer to be doing this at, > especially since this is the linux specific audio manager and not the audio > manager in general. > > Can you perhaps tell me a little more about what specific problem you want to > address? Sure, here's the overview of the Headless Chrome project again if you happened to miss it: https://chromium.googlesource.com/chromium/src/+/master/headless/README.md Basically folks are using headless for things like building services that generate screenshots for a given URL, running test suites for continuous integration, extracting data from web sites, etc. Usually these are intended to run in a server environment, which generally doesn't have graphics or audio hardware. We've solved the graphics part by switching to offscreen software rendering, and I'm hoping to do something similar for audio. This specific bug[1] came up when someone tried to run headless in a Docker image which I think didn't have ALSA or PulseAudio libraries installed. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=678948
tommi@chromium.org changed reviewers: + dalecurtis@chromium.org
Thanks. I know about headless mode from the testing perspective but wasn't aware of this usage of it. In theory - could someone want to use --headless to automate rendering of audio or provide audio via a capture device to web pages? If so, and it doesn't seem completely crazy to me granted that an example usage scenario is capturing bitmaps, then I think that something like --disable-audio might be a better way to go and then disable it at the AudioManager level. However, I'm adding Dale to the conversation as he might have a different view.
ALSA and Pulse are not dynamically linked so the binary would fail to even load if neither was present. I think instead there's a bug in handling when these pieces are not correctly configured, so I don't think this change is needed.
Thanks for the comments. I finally managed to replicate the bug reporter's environment and turns out the audio errors were just a red herring here. The actual crash was caused by the power save blocker not being able to talk to X11. I don't think we need any changes to the media code after all. Tommi's point about using headless to capture rendered audio is an interesting one. We haven't had anyone request that yet, but I think it might be useful. I guess you could kind of already do that by forwarding either ALSA or PulseAudio output to a file, but it still might make sense for us to expose more information about the audio stream to the embedder. In the meantime, let me close this patch -- and sorry for the noise.
Message was sent while issue was closed.
Glad to hear. We have a loopback device id which is used to do this: https://cs.chromium.org/search/?q=kLoopbackInputDeviceId It's only implemented on ChromeOS and Windows though AFAICT. Presumably embedders could open an input device with this ID for this purpose. Caveat: I've never tried this, but I think it's used for testing so should work (/famous_last_words) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
