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

Issue 658048 link

Starred by 4 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 700247

Blocking:
issue 651512



Sign in to add a comment

Eng work for getting pinned stream from headset micjack, actually getting from internal mic

Project Member Reported by warx@chromium.org, Oct 20 2016

Issue description

Version: tot
OS: ChromeOS

bug described in crbug.com/651512

On tot or M54 (54.0.2840.68), open chrome://flags, and turn on #enumerate-audio-devices flag. Then repeat the repro steps on 651512.

Comment 10 and 11 in crbug.com/651512 has logs and some information. I agreed with dgreid@ that we should expose more device settings to the api. What about the last 32 bits of cras_node_id_t?

Firstly assigned to hychao@.
 

Comment 1 by hychao@chromium.org, Oct 27 2016

Hi,
I have read issue 651512 and have a few questions.
You mentioned unable to distinguish mic and micjack because they share the same dev idx. But your log seems having full node id: 0x600000001 and 0x600000000, so why cannot you access the last 32 bits of node id?
Can you explain what is the 'api' you're referring to?

Comment 2 by warx@chromium.org, Nov 1 2016

The api means https://cs.chromium.org/chromium/src/media/audio/cras/cras_input.cc?q=add_pinned&sq=package:chromium&l=224, cras_client_add_pinned_stream, which is defined here: https://cs.corp.google.com/chromeos_public/src/third_party/adhd/cras/src/libcras/cras_client.c?type=cs&q=cras_client_add_pinned_stream&l=2353,
It takes uint32_t dev_idx. I don't see there is a way that I can pass the full node id to differentiate it.
Correct, streams get pinned to devices not nodes.  This is because nodes on the same device often can't play different streams simultaneously.
Thanks for the hint, Dylan. Now I understand the problem.

Pinned stream has limitation when system UI selects to the other node (on the same device) for playback/recording. Examples of pairs: speaker <-> headphone, mic jack <-> internal mic.

Given this limitation, maybe we should enumerate only the devices but not detailed to nodes. Like expose one entry for speaker & headphone labeled by 'device_name' of AudioDevice.
It's messy because the ui needs to be able to select nodes so the user can choose headphone or speaker, but the pinned streams will play to both if they are pinned to either.

Comment 6 by hychao@chromium.org, Nov 16 2016

Cc: hychao@chromium.org
Owner: warx@chromium.org
Back to this issue.
We cannot fix the hardware limitation. And I personally think the best thing to do is modify naming to make this feature less confusing.

Maybe for #enumerate-audio-devices feature we put INTERNAL_SPEAKER and INTERNAL_MIC behind one entry named "Internal". Since only the internal device could have more than one node.  That results the messy behavior as Dylan described in #5. And user will eventually realize that whenever pinned audio to this 'internal' device, it routes to headphone or speaker depends on system UI's selection.

Example:
 - Output nodes exposed to Chrome UI: [ speaker(3:0), headphone(3:1), USB1(4:0), BT-headset(5:0)]
 - Output nodes exposed to mediaDevices.enumerateDevices() layer: [ internal(3), USB1(4), BT-headset(5) ]

Assign back to warx@ to see if my suggestion is feasible.

Comment 7 by warx@chromium.org, Nov 17 2016

Cc: -warx@chromium.org
Wait, I started to realize that the #enumerate-audio-devices feature is only half done. I was thinking we only needed to deal with audio input devices and forgot to test audio output devices. The current behavior I implemented still relies on CRAS default output routing even selecting output destination device from drop down list.

That is to say we got pinning stream from input on the web page, but still 'default' routing to output.
The work should start with passing device_id to https://cs.chromium.org/chromium/src/media/audio/cras/audio_manager_cras.cc?sq=package:chromium&l=292.

My apologize, will open another bug to track.

Comment 8 by warx@chromium.org, Nov 17 2016

Blocking: 651512

Comment 9 by warx@chromium.org, Nov 19 2016

Status: Started (was: Assigned)
I create a cl to fix this and 666202: https://codereview.chromium.org/2516813002/
So per comment 6, I am worried about user experience here. If we "Internal" input to represent internal_mic and 3.5mm micjack, and "Internal" output to represent internal_speaker and 3.5mm headphone. User will really get confused at first because routing depends on chromeos system UI's selection.

On chromeos system tray UI: I can see Speaker(Internal), Microphone(Internal), but not Headphone (Internal) and Mic Jack (Internal).

I don't think we can assume that user knows that headphone is using internal node, if user has multiple outputs plugged. User has to try each one to find this.
I'd agree that it doesn't make too much sense to display "internal"  Maybe a combined "HP/Speaker"?

Comment 11 by warx@chromium.org, Nov 19 2016

"HP/Speaker" sounds better to me than "internal", how about input? Is "Mic/Mic Jack" a professional way to do this?
Maybe Headset/Front Mic.  I'm not sure on that one and it's going to get really messy as we add new mic locations.

Comment 13 by warx@chromium.org, Nov 21 2016

Yes, that is because we allow playing back to internal speaker with headphone plugged. 
If we are going to make this change, how could we let user know that this choice depends on system UI's selection?

Another option may be to call cras_client_select_node API to activate the node if selection is HP/Speaker or HeadSet/Front Mic? But it may need more work and probably is dangerous since javascript could change system settings.

Btw, can we get a PM to confirm the behavior here?
Cc: tbuck...@chromium.org
That approach sounds reasonable to me.

1) Headset/Speakers
2) What about "Internal Mic/External Mic" ?

+Tom for his input too

Comment 15 by warx@chromium.org, Nov 30 2016

For the ease of the PM's input, I will make a summary here:

For the device with internal mic, we will show the label like "Headset/Front Mic". The actual recording depends on:
1) if there is a mic jack plugged, recording from mic jack or internal mic depends on chromeos system UI selection.
2) if there is no mic jack plugged, recording is from internal mic.

For the device with internal speaker, we will show the label like "HP/Speaker". The actual playing-back depends on:
1) if there is a headphone plugged, playing back to headphone or internal speaker depends on chromeos system UI selection.
2) if there is no headphone plugged, playing back is to internal speaker.

We need to make agreement on the label names for this : )

Comment 16 by warx@chromium.org, Dec 2 2016

Btw, if device has 3.5mm headset plugged, usb-headset plugged, on webpage we select "HP/Speaker", but chromeos system UI we select usb-headset output. Audio on the webpage is playbacked to internal speaker.

so, if chromeos system UI doesn't select one of the "slash selections", internal mic and internal speaker is the default choice for routing.
At least on Windows PCs, there is only one "device" for anything hooked up to the internal sound card, so builtin speakers and headphones through the jack, or built-in mic and mic through jack are the same entry in device selection.

I expect users to be used to this, and be fine with the sound to seamlessly switch between built-in mic/speakers and headset when the jack is inserted.

What you're saying is it's possible on chrome os to use the internal mic/speakers even when a headset jack is inserted?
Is there a reason to expose this to the user?

In particular, I'm a little worried about #c16
While it's most likely an unlikely scenario for RTC, as a user if I had the configuration described I would expect audio to be routed to the jack, not to the internal mic/speaker.

If the system is able to differentiate between internal mic/speaker and headphones, why exactly can't this be exposed to pages?

IMO, you either need to make this distinction available to pages, or you need to remove the distinction from the system UI, automatically using the headset if a jack is inserted.

Comment 18 by warx@chromium.org, Mar 10 2017

Blockedon: 700247
Project Member

Comment 19 by bugdroid1@chromium.org, Apr 4 2017

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

commit 188483ae0dc7746fd2bc8d8ebdc2a57b33769007
Author: warx <warx@chromium.org>
Date: Tue Apr 04 21:51:08 2017

cros: Using virtual audio device label for internal device

Changes:
Now that cras could remember last active audio node for pinning stream.  crbug.com/658048  could be properly handled if we use a virtual device name for internal device to represent the internal device with last active internal node.

After discussion with UX writer and reviewer:
Decided using "Built-in-mic" for input device, and "Built-in-speaker" for output device.

BUG= 658048 
TEST=Test on Chrome OS 9426.0.0. The bug in  crbug.com/658048  is handled, if GetUserMedia selects internal device, the source/destination depends on last active internal node or current system activated internal node.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2795933002
Cr-Commit-Position: refs/heads/master@{#461854}

[modify] https://crrev.com/188483ae0dc7746fd2bc8d8ebdc2a57b33769007/media/audio/cras/audio_manager_cras.cc

Comment 20 by warx@chromium.org, Apr 4 2017

Status: Fixed (was: Started)
Project Member

Comment 21 by bugdroid1@chromium.org, Apr 29 2017

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

commit 934e8926ee432fe5f5e6b2b9a1f196455ede77df
Author: warx <warx@chromium.org>
Date: Sat Apr 29 20:13:43 2017

cros: Update virtual audio label name to Built-in mic/speaker

Changes:
As from discussion with UX writer, updating the label name here.

BUG= 658048 
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2847173003
Cr-Commit-Position: refs/heads/master@{#468248}

[modify] https://crrev.com/934e8926ee432fe5f5e6b2b9a1f196455ede77df/media/audio/cras/audio_manager_cras.cc

Comment 22 by warx@chromium.org, Apr 29 2017

Labels: Merge-Request-59
Status: Assigned (was: Fixed)
#21 -> request merge to M59
Project Member

Comment 23 by sheriffbot@chromium.org, Apr 30 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, May 1 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58a885b06af608a8dd427e81a490ef5fb8cc0e2d

commit 58a885b06af608a8dd427e81a490ef5fb8cc0e2d
Author: Qiang Xu <warx@chromium.org>
Date: Mon May 01 00:32:35 2017

[Merge to M59] cros: Update virtual audio label name to Built-in mic/speaker

Changes:
As from discussion with UX writer, updating the label name here.

TBR=tommi@chromium.org
BUG= 658048 
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2847173003
Cr-Commit-Position: refs/heads/master@{#468248}
(cherry picked from commit 934e8926ee432fe5f5e6b2b9a1f196455ede77df)

Review-Url: https://codereview.chromium.org/2856493002 .
Cr-Commit-Position: refs/branch-heads/3071@{#314}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/58a885b06af608a8dd427e81a490ef5fb8cc0e2d/media/audio/cras/audio_manager_cras.cc

Comment 25 by warx@chromium.org, May 1 2017

Status: Fixed (was: Assigned)
Labels: VerifyIn-61
Status: Verified (was: Fixed)

Sign in to add a comment