Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516813002:
Enable pinning stream output routing for webapp request on ChromeOS (Closed)
|
Created:
4 years, 1 month ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable pinning stream output routing for webapp request on ChromeOS
This CL fixes two bugs.
For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE.
For crbug.com/658048, using "Headset/Front Mic" input to represent internal_mic and 3.5mm micjack, and "HP/Speaker" output to represent internal_speaker and 3.5mm headphone.
The "/" on the webpage means routing then depends on chromeos system UI for that label.
BUG=666202
BUG=658048
TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device."
Except "HP/Speaker" and "Headset/Front Mic" then needs to depend on chromeos system UI selecting.
Test relies on the chrome://flags #enumerate-audio-devices enabled.
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
Patch Set 2 : fix compiler error #Patch Set 3 : new labels instead of Internal #
Total comments: 7
Patch Set 4 : comment #
Total comments: 2
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Enable pinning stream output routing for webapp request on ChromeOS BUG=666202 BUG=658048 ========== to ========== Enable pinning stream output routing for webapp request on ChromeOS BUG=666202 BUG=658048 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 ========== Enable pinning stream output routing for webapp request on ChromeOS BUG=666202 BUG=658048 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 ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 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 ==========
warx@chromium.org changed reviewers: + dgreid@chromium.org, hychao@chromium.org, tommi@chromium.org
Description was changed from ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 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 ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device." Except "Internal" depends on chromeos system UI selecting. 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 ==========
Hi all, ptal, thanks! dgreid@, hychao@ for bugs related code changes. tommi@ for owner review. https://codereview.chromium.org/2516813002/diff/1/media/audio/cras/audio_mana... File media/audio/cras/audio_manager_cras.cc (left): https://codereview.chromium.org/2516813002/diff/1/media/audio/cras/audio_mana... media/audio/cras/audio_manager_cras.cc:242: DLOG_IF(ERROR, !device_id.empty()) << "Not implemented!"; it is now supported https://codereview.chromium.org/2516813002/diff/1/media/audio/cras/audio_mana... media/audio/cras/audio_manager_cras.cc:268: DLOG_IF(ERROR, !output_device_id.empty()) << "Not implemented!"; it is now supported https://codereview.chromium.org/2516813002/diff/1/media/audio/cras/audio_mana... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2516813002/diff/1/media/audio/cras/audio_mana... media/audio/cras/audio_manager_cras.cc:264: // (warx): pinning stream is not supported for MakeLinearOutputStream. shall we open a new bug to track this? I don't have much context with this...@tommi for more thoughts.
Description was changed from ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device." Except "Internal" depends on chromeos system UI selecting. 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 ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device." Except "Internal" depends on chromeos system UI selecting. Test relies on the chrome://flags #enumerate-audio-devices enabled. 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 ==========
The CQ bit was checked by warx@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: 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-...)
The CQ bit was checked by warx@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: This issue passed the CQ dry run.
Description was changed from ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Internal" label to represent internal device, suggested by hychao@. That is "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. Selecting "Internal" on the webpage, the routing then depends on chromeos system UI. BUG=666202 BUG=658048 TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device." Except "Internal" depends on chromeos system UI selecting. Test relies on the chrome://flags #enumerate-audio-devices enabled. 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 ========== Enable pinning stream output routing for webapp request on ChromeOS This CL fixes two bugs. For crbug.com/666202, support pinning stream for output, the behavior reverts to default if pin_device_ is NO_DEVICE. For crbug.com/658048, using "Headset/Front Mic" input to represent internal_mic and 3.5mm micjack, and "HP/Speaker" output to represent internal_speaker and 3.5mm headphone. The "/" on the webpage means routing then depends on chromeos system UI for that label. BUG=666202 BUG=658048 TEST=manual test and expect the results: "The drop down for audio input and output from a web page should show a list of available devices plus "default". Choosing one from a web page will route only that page's audio to/from the selected device." Except "HP/Speaker" and "Headset/Front Mic" then needs to depend on chromeos system UI selecting. Test relies on the chrome://flags #enumerate-audio-devices enabled. 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 ==========
Hi all, the new change is to replace "Internal" with "HP/Speaker" and "Headset/Front Mic" based on Dylan's suggestion. It is still messy here but I think we cannot do anything more right now.
On 2016/11/21 22:39:15, Qiang (Joe) Xu wrote: > Hi all, the new change is to replace "Internal" with "HP/Speaker" and > "Headset/Front Mic" based on Dylan's suggestion. It is still messy here but I > think we cannot do anything more right now. Hi all, despite the pending agreement on the label name on crbug.com/658048, could we start the review on this? Thanks!
I'll be happy to rubberstamp once reviewed by someone more familiar with the cras code.
https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:177: else if (device.type == chromeos::AUDIO_TYPE_INTERNAL_SPEAKER) How does this work on devices without a speaker node? Chromebit and some boxes don't have a speaker but do have a headset. Same applies for internal mic, it might not exist for some form factors. https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:201: } Can you summarize what this code does? Is "Replace the internal and external device names with a default string and skip duplicate nodes on internal devices." correct? Would this also ignore an HDMI node if it was on the default device?
Hi Dylan, here is my reply and some puzzles, thanks! https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:177: else if (device.type == chromeos::AUDIO_TYPE_INTERNAL_SPEAKER) On 2016/11/29 19:38:44, dgreid wrote: > How does this work on devices without a speaker node? Chromebit and some boxes > don't have a speaker but do have a headset. > > Same applies for internal mic, it might not exist for some form factors. For devices without a speaker node or internal mic node, internal_output_dev_index or internal_input_dev_index will be zero. In this case, the headset will be labeled as Headphone or MicJack, instead of HP/Speaker or Headset/Front Mic. Then it should not cause any problem, right? https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:201: } On 2016/11/29 19:38:44, dgreid wrote: > Can you summarize what this code does? > > Is "Replace the internal and external device names with a default string and > skip duplicate nodes on internal devices." correct? > > Would this also ignore an HDMI node if it was on the default device? "Replace the internal and external device names with a default string and skip duplicate nodes on internal devices." --> correct "Would this also ignore an HDMI node if it was on the default device?" What does this mean? The default label string works only when there are two nodes pair existing (one is internal mic or one is internal speaker). For other cases, the behavior should restore to the version before this change. Do you mean HDMI node can be one of the nodes pair with internal speaker? Is that possible?
https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:177: else if (device.type == chromeos::AUDIO_TYPE_INTERNAL_SPEAKER) On 2016/11/29 21:57:47, Qiang (Joe) Xu wrote: > On 2016/11/29 19:38:44, dgreid wrote: > > How does this work on devices without a speaker node? Chromebit and some > boxes > > don't have a speaker but do have a headset. > > > > Same applies for internal mic, it might not exist for some form factors. > > For devices without a speaker node or internal mic node, > internal_output_dev_index or internal_input_dev_index will be zero. In this > case, the headset will be labeled as Headphone or MicJack, instead of HP/Speaker > or Headset/Front Mic. Then it should not cause any problem, right? Got it. Yes, I hadn't fully understood the code here, that should be OK. https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:201: } On 2016/11/29 21:57:47, Qiang (Joe) Xu wrote: > On 2016/11/29 19:38:44, dgreid wrote: > > Can you summarize what this code does? > > > > Is "Replace the internal and external device names with a default string and > > skip duplicate nodes on internal devices." correct? > > > > Would this also ignore an HDMI node if it was on the default device? > > "Replace the internal and external device names with a default string and skip > duplicate nodes on internal devices." --> correct > > "Would this also ignore an HDMI node if it was on the default device?" What does > this mean? The default label string works only when there are two nodes pair > existing (one is internal mic or one is internal speaker). For other cases, the > behavior should restore to the version before this change. > > Do you mean HDMI node can be one of the nodes pair with internal speaker? Is > that possible? OK, can you add a comment with a summary? It was hard to determine what the code was trying to do. HDMI can be a node on the main device for some intel platforms, but that is an exception and if it only affects the device pinning path, then it might be OK.
Hi all, I added a comment based on suggestion. Please see how it sounds to you. Thanks! https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... File media/audio/cras/audio_manager_cras.cc (right): https://codereview.chromium.org/2516813002/diff/40001/media/audio/cras/audio_... media/audio/cras/audio_manager_cras.cc:201: } On 2016/11/29 23:23:18, dgreid wrote: > On 2016/11/29 21:57:47, Qiang (Joe) Xu wrote: > > On 2016/11/29 19:38:44, dgreid wrote: > > > Can you summarize what this code does? > > > > > > Is "Replace the internal and external device names with a default string and > > > skip duplicate nodes on internal devices." correct? > > > > > > Would this also ignore an HDMI node if it was on the default device? > > > > "Replace the internal and external device names with a default string and skip > > duplicate nodes on internal devices." --> correct > > > > "Would this also ignore an HDMI node if it was on the default device?" What > does > > this mean? The default label string works only when there are two nodes pair > > existing (one is internal mic or one is internal speaker). For other cases, > the > > behavior should restore to the version before this change. > > > > Do you mean HDMI node can be one of the nodes pair with internal speaker? Is > > that possible? > > OK, can you add a comment with a summary? It was hard to determine what the > code was trying to do. > > HDMI can be a node on the main device for some intel platforms, but that is an > exception and if it only affects the device pinning path, then it might be OK. Done.
https://codereview.chromium.org/2516813002/diff/60001/media/audio/cras/cras_u... File media/audio/cras/cras_unified.cc (right): https://codereview.chromium.org/2516813002/diff/60001/media/audio/cras/cras_u... media/audio/cras/cras_unified.cc:209: LOG(WARNING) << "Failed to add the stream."; How does this work when the Chrome OS UI switches the device? What about when a device is unplugged? I was expecting this to set a pinned device when needed, but add a default stream otherwise.
Hi Dylan, here is my reply, thanks! https://codereview.chromium.org/2516813002/diff/60001/media/audio/cras/cras_u... File media/audio/cras/cras_unified.cc (right): https://codereview.chromium.org/2516813002/diff/60001/media/audio/cras/cras_u... media/audio/cras/cras_unified.cc:209: LOG(WARNING) << "Failed to add the stream."; On 2016/11/30 01:45:08, dgreid wrote: > How does this work when the Chrome OS UI switches the device? What about when a > device is unplugged? > > I was expecting this to set a pinned device when needed, but add a default > stream otherwise. The expectation can be satisfied. When GetUserMedia is called, audio manager (AudioManagerCras here) MakeOutputStream, depending on the device_id (Default choice or device that wants to be pinned to), pin_device_ will be initialized in line 72-76. If 'Default' label is chosen, pin_device_ == NO_DEVICE, which will be the default stream, otherwise pin_device_ is a dev_idx. cras_client_add_pinned_stream(client_, NO_DEVICE, &stream_id_, stream_params) is equal to cras_client_add_stream(client_, &stream_id_, stream_params) When ChromeOS UI switches the device, it matters when the choice is 'Default', but it doesn't matter for other choices. When a device is unplugged, all I can think of is if this device is doing pinning stream, it can no longer fill or read audio datas anymore... my cl doesn't handle this. It will possibly notify stream error, and stop or destroy the stream? If this device is not doing pinning stream, I think it is fine. The next mediaDevices.enumerateDevices call will not show the unplugged device.
Thanks makes sense, thanks! This change lgtm, can you test what happens when you unplug a USB device for example?
On 2016/11/30 02:14:29, dgreid wrote: > Thanks makes sense, thanks! > > This change lgtm, can you test what happens when you unplug a USB device for > example? I tested with https://webrtc.github.io/samples/src/content/getusermedia/source/, selected usb-headset for both input source and output source and then unplugged the usb-headset. Audio stopped after unplugging. Web-UI still displayed the usb-headset selection. Refresh the page. Usb-headset options no longer existed. I didn't see the errors in the js developer console. So it should be fine here I think.
lgtm
Message was sent while issue was closed.
On 2016/12/02 03:03:13, hychao wrote: > lgtm Since https://codereview.chromium.org/2651073004/ is landed, I will close this one. If audio label for internal_speaker/headphone, internal_mic/micjack is what we want to do, I will open a new code review. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
