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

Issue 636300 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android
Pri: 2
Type: Bug



Sign in to add a comment

groupId is wrong for outputdevice on MediaDevices.enumerateDevices

Project Member Reported by guidou@chromium.org, Aug 10 2016

Issue description

Version: 54
OS: Linux

What steps will reproduce the problem?
(1) invoke mediaDevices.enumerateDevices() on a machine with at least one sound card that has input and output.

What is the expected output?
The entries in the result for the input and output device for that sound adapter should have the same group ID.

What do you see instead?
They have different group IDs.


 

Comment 1 by maxmorin@google.com, Aug 11 2016

Does this work on other platforms? It seems like the matched_output_device_id field isn't populated until the input/output is opened (by a call to getUserMedia for example), which usually hasn't been done when enumerating, so we'd fall back to using device.id when generating the groupid.
Status: Started (was: Assigned)

Comment 3 by guidou@chromium.org, Aug 19 2016

Labels: OS-Windows
This problem occurs on Windows too.
Thanks for looking into this. I split off separate CL for fixing the issue where matched_output_device_id isn't populated.
Using matched_output_device_id is a hack that isn't going to work. I'll add an actual group id to MediaStreamDevice.
Labels: -OS-Linux -OS-Windows OS-All
Summary: groupId is wrong for outputdevice on MediaDevices.enumerateDevices (was: groupId is wrong for outputdevice on MediaDevices.enumerateDevices on Linux)

Comment 7 by hta@chromium.org, Aug 29 2016

Making this more complex: It's not clear that input and output on a sound card should be linked.

If the output is connected to room speakers and input is connected to a desk microphone, it's not clear that the user will see them as associated.

If both are connected to an analog headset, the user will see them as associated.

Not sure how to solve this, but just saying "one sound card is always one group" doesn't seem to me to be the right solution.

I don't see how we could ever implement the spec to the letter, since information about what a user considers a physical device isn't actually available to the computer.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 29 2016

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

commit cff5a1f78bd76f764a70efcdb9b908ff6a87c60f
Author: maxmorin <maxmorin@chromium.org>
Date: Mon Aug 29 18:02:08 2016

Add GetAssociatedOutputDeviceID support to pulse.
We use the bus path for identification of devices. I have verified that it works with a headset connected with 3.5 mm connectors, and with a USB headset. Another nice thing with  his approach is that we should be able to easily find if a webcam has a built-in mic, since in this case the webcam and mic will typically be connected via the same USB port.

BUG=636300

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

[modify] https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f/media/audio/pulse/audio_manager_pulse.cc
[modify] https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f/media/audio/pulse/audio_manager_pulse.h
[modify] https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f/media/audio/pulse/pulse.sigs
[modify] https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f/media/audio/pulse/pulse_util.cc
[modify] https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f/media/audio/pulse/pulse_util.h

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 2 2016

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

commit f07d1047986130ef0277a87b5b0b6175c4362c98
Author: maxmorin <maxmorin@chromium.org>
Date: Fri Sep 02 00:17:27 2016

Add groupid for media devices. Group audio devices.

At present, the renderer tries to use matched_output_device
to assign groupids to audio devices. matched_output_device isn't set
before devices are sent to the renderer, and we wouldn't be able to
handle all the cases with it anyways.

BUG=636300, 627793

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

[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/browser/renderer_host/media/audio_input_device_manager.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/common/media/media_stream_messages.h
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/public/common/media_stream_request.h
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/renderer/media/mock_media_stream_dispatcher.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/audio_manager.h
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/audio_manager_base.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/audio_manager_base.h
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/audio_manager_unittest.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/mock_audio_manager.cc
[modify] https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98/media/audio/mock_audio_manager.h

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 2 2016

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

commit 0c747762e50c0452f7809fa48249a1a776ff43d2
Author: maxmorin <maxmorin@chromium.org>
Date: Fri Sep 02 09:09:46 2016

Revert of Add groupid for media devices. Group audio devices. (patchset #5 id:80001 of https://codereview.chromium.org/2273653002/ )

Reason for revert:
Breaks webrtc FYI bots on mac https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/59143. I'll have to figure out what's up with that ¯\_(ツ)_/¯

Original issue's description:
> Add groupid for media devices. Group audio devices.
>
> At present, the renderer tries to use matched_output_device
> to assign groupids to audio devices. matched_output_device isn't set
> before devices are sent to the renderer, and we wouldn't be able to
> handle all the cases with it anyways.
>
> BUG=636300, 627793
>
> Committed: https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98
> Cr-Commit-Position: refs/heads/master@{#416140}

TBR=guidou@chromium.org,tommi@chromium.org,nasko@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=636300, 627793

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

[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/browser/renderer_host/media/audio_input_device_manager.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/common/media/media_stream_messages.h
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/public/common/media_stream_request.h
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/renderer/media/mock_media_stream_dispatcher.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/audio_manager.h
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/audio_manager_base.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/audio_manager_base.h
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/audio_manager_unittest.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/mock_audio_manager.cc
[modify] https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2/media/audio/mock_audio_manager.h

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 6 2016

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

commit a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34
Author: maxmorin <maxmorin@chromium.org>
Date: Tue Sep 06 17:57:09 2016

Reland of Add groupid for media devices. Group audio devices. (patchset #1 id:1 of https://codereview.chromium.org/2296393004/ )

Reason for revert:
I'm fixing the threading issue now.

Original issue's description:
> Revert of Add groupid for media devices. Group audio devices. (patchset #5 id:80001 of https://codereview.chromium.org/2273653002/ )
>
> Reason for revert:
> Breaks webrtc FYI bots on mac https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/59143. I'll have to figure out what's up with that ¯\_(ツ)_/¯
>
> Original issue's description:
> > Add groupid for media devices. Group audio devices.
> >
> > At present, the renderer tries to use matched_output_device
> > to assign groupids to audio devices. matched_output_device isn't set
> > before devices are sent to the renderer, and we wouldn't be able to
> > handle all the cases with it anyways.
> >
> > BUG=636300, 627793
> >
> > Committed: https://crrev.com/f07d1047986130ef0277a87b5b0b6175c4362c98
> > Cr-Commit-Position: refs/heads/master@{#416140}
>
> TBR=guidou@chromium.org,tommi@chromium.org,nasko@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=636300, 627793
>
> Committed: https://crrev.com/0c747762e50c0452f7809fa48249a1a776ff43d2
> Cr-Commit-Position: refs/heads/master@{#416217}

TBR=guidou@chromium.org,tommi@chromium.org,nasko@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=636300, 627793

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

[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/browser/renderer_host/media/audio_input_device_manager.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/browser/renderer_host/media/audio_output_device_enumerator.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/browser/renderer_host/media/audio_output_device_enumerator.h
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/browser/renderer_host/media/audio_renderer_host.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/common/media/media_stream_messages.h
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/public/common/media_stream_request.h
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/renderer/media/mock_media_stream_dispatcher.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/audio_manager.h
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/audio_manager_base.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/audio_manager_base.h
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/audio_manager_unittest.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/mock_audio_manager.cc
[modify] https://crrev.com/a4ec5769ae36fdd14e49207f30e0aa8dcdc2ac34/media/audio/mock_audio_manager.h

The latest change breaks audio for me on a linux Chrome OS build. Any ideas or should the change be reverted?
dtseng: can you provide more details about the breakage you are seeing?
Reverting is OK if it fixes the problem.
media/audio/sounds/sounds_manager::PlaySound fails. In addition, sounds played from a NACL extension fail to play.

Found the cl via a bisect and reverting does get audio back though I'm seeing a likely unrelated crash on master.

To be clear, the cl was revision 415024 cff5a1f78bd76f764a70efcdb9b908ff6a87c60f

Comment 16 Deleted

Project Member

Comment 17 by bugdroid1@chromium.org, Sep 9 2016

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

commit fdaa93d6a62212b4bab1eb6709a42e9225c56fe4
Author: dtseng <dtseng@chromium.org>
Date: Fri Sep 09 06:45:19 2016

Revert of Add GetAssociatedOutputDeviceID support to pulse. (patchset #7 id:120001 of https://codereview.chromium.org/2258143002/ )

Reason for revert:
Breaks audio playback on Linux Chrome OS
target_os = "chromeos" in gn args

( SoundsManager::Play fails).

Original issue's description:
> Add GetAssociatedOutputDeviceID support to pulse.
> We use the bus path for identification of devices. I have verified that it works with a headset connected with 3.5 mm connectors, and with a USB headset. Another nice thing with  his approach is that we should be able to easily find if a webcam has a built-in mic, since in this case the webcam and mic will typically be connected via the same USB port.
>
> BUG=636300
>
> Committed: https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f
> Cr-Commit-Position: refs/heads/master@{#415024}

TBR=guidou@chromium.org,tommi@chromium.org,maxmorin@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=636300

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

[modify] https://crrev.com/fdaa93d6a62212b4bab1eb6709a42e9225c56fe4/media/audio/pulse/audio_manager_pulse.cc
[modify] https://crrev.com/fdaa93d6a62212b4bab1eb6709a42e9225c56fe4/media/audio/pulse/audio_manager_pulse.h
[modify] https://crrev.com/fdaa93d6a62212b4bab1eb6709a42e9225c56fe4/media/audio/pulse/pulse.sigs
[modify] https://crrev.com/fdaa93d6a62212b4bab1eb6709a42e9225c56fe4/media/audio/pulse/pulse_util.cc
[modify] https://crrev.com/fdaa93d6a62212b4bab1eb6709a42e9225c56fe4/media/audio/pulse/pulse_util.h

Project Member

Comment 18 by bugdroid1@chromium.org, Sep 12 2016

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

commit 50eb11b562c9d277ec416899ccb365c04ff295b4
Author: guidou <guidou@chromium.org>
Date: Mon Sep 12 12:21:02 2016

Add constructor taking a group ID to MediaStreamDevice and StreamDeviceInfo.

BUG=636300

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

[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/browser/renderer_host/media/audio_input_device_manager.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/browser/renderer_host/media/video_capture_manager.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/common/media/media_stream_options.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/common/media/media_stream_options.h
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/public/common/media_stream_request.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/public/common/media_stream_request.h
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/renderer/media/mock_media_stream_registry.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/renderer/media/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/renderer/media/webrtc/processed_local_audio_source_unittest.cc
[modify] https://crrev.com/50eb11b562c9d277ec416899ccb365c04ff295b4/content/renderer/media/webrtc/webrtc_media_stream_adapter_unittest.cc

Cc: maxmorin@chromium.org
Labels: -OS-All OS-Android OS-Chrome OS-iOS OS-Linux
Owner: ----
Status: Available (was: Started)
I've done all work I'm going to do on this.
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 19 2017

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

commit c8bc83f1143c9d5424154b875cfe753759e28176
Author: Max Morin <maxmorin@chromium.org>
Date: Wed Jul 19 10:21:25 2017

Support GetAssociatedOutputDeviceID on CrOS.

With this CL, AudioManagerCras associates devices with each other
based on their chromeos::AudioDevice::device_name, which represents
the physical hardware of a device. This will also allow Chrome to
compute group ids for audio devices.

Also fix lint errors.

BUG=636300

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
Change-Id: Idbbb4b3ad833564f603e317093108eadfbbdddf8
Reviewed-on: https://chromium-review.googlesource.com/538754
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Reviewed-by: Tommi <tommi@chromium.org>
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487814}
[modify] https://crrev.com/c8bc83f1143c9d5424154b875cfe753759e28176/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/c8bc83f1143c9d5424154b875cfe753759e28176/chromeos/audio/cras_audio_handler.h
[modify] https://crrev.com/c8bc83f1143c9d5424154b875cfe753759e28176/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/c8bc83f1143c9d5424154b875cfe753759e28176/media/audio/cras/audio_manager_cras.h

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 20 2017

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

commit 7a2d72adfce316e4b56c1218ad795a99d19e8f7a
Author: Max Morin <maxmorin@chromium.org>
Date: Thu Jul 20 19:06:25 2017

Invalidate device cache on default device change.

Changing which device is the default one will change the device
enumeration result since
https://chromium-review.googlesource.com/c/538754/. This CL makes sure
to invalidate the device cache in MediaDevicesManager when this happens.

Tested by plugging in various devices into a Pixel, verifying that input
and output from each device has the same group id, and in addition that
the "default" device has the same group id as the actual device that is
considered default. Moreover, after changing the default device in the
system UI and re-enumerating the result, the group id is updated
accordingly.

BUG=636300
R=jennyz

Change-Id: Ie847bd052a609022826f31fbc3059a98049b875d
Reviewed-on: https://chromium-review.googlesource.com/577532
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488341}
[modify] https://crrev.com/7a2d72adfce316e4b56c1218ad795a99d19e8f7a/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/7a2d72adfce316e4b56c1218ad795a99d19e8f7a/chromeos/audio/cras_audio_handler_unittest.cc

Labels: -OS-Chrome
That should be it for Chrome OS.
Project Member

Comment 23 by bugdroid1@chromium.org, Jul 25 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2da06c9098862a1e6364fca1af22c528d96dee2

commit e2da06c9098862a1e6364fca1af22c528d96dee2
Author: Qiang Xu <warx@chromium.org>
Date: Tue Jul 25 20:15:06 2017

[revert in m61] Revert "Support GetAssociatedOutputDeviceID on CrOS."

This reverts commit c8bc83f1143c9d5424154b875cfe753759e28176.

Reason for revert: this is a speculative revert in M61 in hope of
fixing  crbug.com/748267 .

TBR: jennyz@chromium.org, tommi@chromium.org, maxmorin@chromium.org
Bug:  748267 , 636300
Change-Id: I895fcdd322ccb29ca37e1dd9d06d6d8d3fe5dc0e
Reviewed-on: https://chromium-review.googlesource.com/585596
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#33}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/e2da06c9098862a1e6364fca1af22c528d96dee2/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/e2da06c9098862a1e6364fca1af22c528d96dee2/chromeos/audio/cras_audio_handler.h
[modify] https://crrev.com/e2da06c9098862a1e6364fca1af22c528d96dee2/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/e2da06c9098862a1e6364fca1af22c528d96dee2/media/audio/cras/audio_manager_cras.h

Project Member

Comment 24 by bugdroid1@chromium.org, Jul 27 2017

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

commit 82918d4ddeaf2995e1e4c8f1ce8501f326cb98de
Author: Qiang(Joe) Xu <warx@chromium.org>
Date: Thu Jul 27 22:46:09 2017

Revert "Support GetAssociatedOutputDeviceID on CrOS."

This reverts commit c8bc83f1143c9d5424154b875cfe753759e28176.

Reason for revert:  crbug.com/748267 

Original change's description:
> Support GetAssociatedOutputDeviceID on CrOS.
> 
> With this CL, AudioManagerCras associates devices with each other
> based on their chromeos::AudioDevice::device_name, which represents
> the physical hardware of a device. This will also allow Chrome to
> compute group ids for audio devices.
> 
> Also fix lint errors.
> 
> BUG=636300
> 
> 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
> Change-Id: Idbbb4b3ad833564f603e317093108eadfbbdddf8
> Reviewed-on: https://chromium-review.googlesource.com/538754
> Reviewed-by: Jenny Zhang <jennyz@chromium.org>
> Reviewed-by: Tommi <tommi@chromium.org>
> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> Commit-Queue: Max Morin <maxmorin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487814}

TBR=jennyz@chromium.org,tommi@chromium.org,warx@chromium.org,maxmorin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

TBR: jennyz@chromium.org, tommi@chromium.org, maxmorin@chromium.org
Bug: 636300, 748267 
Change-Id: I916e1075026f419beed8948312435c9d23d226cc
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
Reviewed-on: https://chromium-review.googlesource.com/585402
Commit-Queue: Qiang(Joe) Xu <warx@chromium.org>
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490031}
[modify] https://crrev.com/82918d4ddeaf2995e1e4c8f1ce8501f326cb98de/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/82918d4ddeaf2995e1e4c8f1ce8501f326cb98de/chromeos/audio/cras_audio_handler.h
[modify] https://crrev.com/82918d4ddeaf2995e1e4c8f1ce8501f326cb98de/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/82918d4ddeaf2995e1e4c8f1ce8501f326cb98de/media/audio/cras/audio_manager_cras.h

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 1 2017

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

commit 64abc8e04cf992bb93d014efcb43828dcab11298
Author: Guido Urdaneta <guidou@chromium.org>
Date: Tue Aug 01 07:42:14 2017

Reland "Support GetAssociatedOutputDeviceID on CrOS.""

This reverts commit 82918d4ddeaf2995e1e4c8f1ce8501f326cb98de.

Reason for revert: Will reland a patched version of this CL.

Original change's description:
> Revert "Support GetAssociatedOutputDeviceID on CrOS."
> 
> This reverts commit c8bc83f1143c9d5424154b875cfe753759e28176.
> 
> Reason for revert:  crbug.com/748267 
> 
> Original change's description:
> > Support GetAssociatedOutputDeviceID on CrOS.
> > 
> > With this CL, AudioManagerCras associates devices with each other
> > based on their chromeos::AudioDevice::device_name, which represents
> > the physical hardware of a device. This will also allow Chrome to
> > compute group ids for audio devices.
> > 
> > Also fix lint errors.
> > 
> > BUG=636300
> > 
> > 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
> > Change-Id: Idbbb4b3ad833564f603e317093108eadfbbdddf8
> > Reviewed-on: https://chromium-review.googlesource.com/538754
> > Reviewed-by: Jenny Zhang <jennyz@chromium.org>
> > Reviewed-by: Tommi <tommi@chromium.org>
> > Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> > Commit-Queue: Max Morin <maxmorin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#487814}
> 
> TBR=jennyz@chromium.org,tommi@chromium.org,warx@chromium.org,maxmorin@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> TBR: jennyz@chromium.org, tommi@chromium.org, maxmorin@chromium.org
> Bug: 636300, 748267 
> Change-Id: I916e1075026f419beed8948312435c9d23d226cc
> 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
> Reviewed-on: https://chromium-review.googlesource.com/585402
> Commit-Queue: Qiang(Joe) Xu <warx@chromium.org>
> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#490031}

TBR=jennyz@chromium.org,tommi@chromium.org,warx@chromium.org,maxmorin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 636300,  748267 
Change-Id: I5d8c518c5c840ea77f3f712d0b5e54e2d5aa8a75
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
Reviewed-on: https://chromium-review.googlesource.com/593687
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490905}
[modify] https://crrev.com/64abc8e04cf992bb93d014efcb43828dcab11298/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/64abc8e04cf992bb93d014efcb43828dcab11298/chromeos/audio/cras_audio_handler.h
[modify] https://crrev.com/64abc8e04cf992bb93d014efcb43828dcab11298/media/audio/audio_manager_base.cc
[modify] https://crrev.com/64abc8e04cf992bb93d014efcb43828dcab11298/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/64abc8e04cf992bb93d014efcb43828dcab11298/media/audio/cras/audio_manager_cras.h

Labels: -merge-merged-3163 Merge-Request-61
Labels: M-61
Project Member

Comment 28 by sheriffbot@chromium.org, Aug 3 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Note that the requested merge applies to r490905, which is a corrected version of the CL that was reverted in 61 and 62.
Before we approve merge for r490905, could you pls confirm this change is well baked/verified in Canary, having enough automation coverage and will be a safe merge?
This bug has been open since August 2016, can this wait until M62? 
Also which OSs this bug is most applicable too?
Since the reland, the change has been in Canary for two days. We can wait a few more days before the actual remerge.
There are tests that cover this, although in practice they are not useful in bots because they do not have real audio devices.
Before the reland there was a single issue which caused the CL to be reverted. That issue is no longer reproducible (see  bug 748267 #c40).

This bug has been open for a while, but does not apply to Windows and Mac OSX, where audio group IDs already work correctly.
Fixing it in CrOS gained higher priority recently due to a number of applications running on ChromeOS requiring this functionality. This is the reason why we would strongly prefer to have this in M61.

The bug is still open for Linux.
On Android the bug is not entirely applicable since Android audio has a different architecture and it can be argued that all microphones and speakers are part of the same device and hence should have the same group ID.
Labels: -OS-iOS
Labels: OS-Chrome
Cc: keta...@chromium.org
+ketakid@ for M61 merge review as this bug is more prominent on Chrome OS.
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 Chrome OS.
Project Member

Comment 36 by sheriffbot@chromium.org, Aug 8 2017

Cc: kbleicher@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 37 by bugdroid1@chromium.org, Aug 8 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09

commit 73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09
Author: Guido Urdaneta <guidou@chromium.org>
Date: Tue Aug 08 15:15:29 2017

Reland "Support GetAssociatedOutputDeviceID on CrOS.""

This reverts commit 82918d4ddeaf2995e1e4c8f1ce8501f326cb98de.

Reason for revert: Will reland a patched version of this CL.

Original change's description:
> Revert "Support GetAssociatedOutputDeviceID on CrOS."
>
> This reverts commit c8bc83f1143c9d5424154b875cfe753759e28176.
>
> Reason for revert:  crbug.com/748267 
>
> Original change's description:
> > Support GetAssociatedOutputDeviceID on CrOS.
> >
> > With this CL, AudioManagerCras associates devices with each other
> > based on their chromeos::AudioDevice::device_name, which represents
> > the physical hardware of a device. This will also allow Chrome to
> > compute group ids for audio devices.
> >
> > Also fix lint errors.
> >
> > BUG=636300
> >
> > 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
> > Change-Id: Idbbb4b3ad833564f603e317093108eadfbbdddf8
> > Reviewed-on: https://chromium-review.googlesource.com/538754
> > Reviewed-by: Jenny Zhang <jennyz@chromium.org>
> > Reviewed-by: Tommi <tommi@chromium.org>
> > Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> > Commit-Queue: Max Morin <maxmorin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#487814}
>
> TBR=jennyz@chromium.org,tommi@chromium.org,warx@chromium.org,maxmorin@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> TBR: jennyz@chromium.org, tommi@chromium.org, maxmorin@chromium.org
> Bug: 636300, 748267 
> Change-Id: I916e1075026f419beed8948312435c9d23d226cc
> 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
> Reviewed-on: https://chromium-review.googlesource.com/585402
> Commit-Queue: Qiang(Joe) Xu <warx@chromium.org>
> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#490031}

TBR=guidou@chromium.org, jennyz@chromium.org, maxmorin@chromium.org, tommi@chromium.org, warx@chromium.org


(cherry picked from commit 64abc8e04cf992bb93d014efcb43828dcab11298)

Bug: 636300,  748267 
Change-Id: I5d8c518c5c840ea77f3f712d0b5e54e2d5aa8a75
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
Reviewed-on: https://chromium-review.googlesource.com/593687
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490905}
Reviewed-on: https://chromium-review.googlesource.com/605950
Cr-Commit-Position: refs/branch-heads/3163@{#379}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09/chromeos/audio/cras_audio_handler.h
[modify] https://crrev.com/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09/media/audio/audio_manager_base.cc
[modify] https://crrev.com/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/73cf47c1e558ccd7e73dcf0ea2322830ed3dbb09/media/audio/cras/audio_manager_cras.h

Project Member

Comment 38 by bugdroid1@chromium.org, Sep 12 2017

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

commit a00d6fa0a60f520958721cf380dedfe7981e353c
Author: Max Morin <maxmorin@chromium.org>
Date: Tue Sep 12 12:01:56 2017

Fix group id for default devices on Chrome OS.

This CL addresses a couple of bugs related to group id computations.

First, the default output device isn't correctly identified as
GetPrimaryActiveOutputNodeOnMainThread function doesn't actually set the
output value.

Second, default input device id was assumed to correspond to the
built-in microphone in earlier versions of the code. This isn't true
(and wasn't true before either). Now, we actually look up what device is
the default input device.

BUG=636300, 764228 

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
Change-Id: Ief22bfa5dffe29279dd906dd252c4d2489f30f1b
Reviewed-on: https://chromium-review.googlesource.com/662761
Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501240}
[modify] https://crrev.com/a00d6fa0a60f520958721cf380dedfe7981e353c/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/a00d6fa0a60f520958721cf380dedfe7981e353c/media/audio/cras/audio_manager_cras.h

Project Member

Comment 39 by bugdroid1@chromium.org, Sep 15 2017

Labels: merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5947ab6d4e7e3e9f5775208b9b569b1904479b9c

commit 5947ab6d4e7e3e9f5775208b9b569b1904479b9c
Author: Max Morin <maxmorin@chromium.org>
Date: Fri Sep 15 08:24:51 2017

[M62] Fix group id for default devices on Chrome OS.

This CL addresses a couple of bugs related to group id computations.

First, the default output device isn't correctly identified as
GetPrimaryActiveOutputNodeOnMainThread function doesn't actually set the
output value.

Second, default input device id was assumed to correspond to the
built-in microphone in earlier versions of the code. This isn't true
(and wasn't true before either). Now, we actually look up what device is
the default input device.

BUG=636300, 764228 

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
Change-Id: Ief22bfa5dffe29279dd906dd252c4d2489f30f1b
Reviewed-on: https://chromium-review.googlesource.com/662761
Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501240}(cherry picked from commit a00d6fa0a60f520958721cf380dedfe7981e353c)
Reviewed-on: https://chromium-review.googlesource.com/667149
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#246}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/5947ab6d4e7e3e9f5775208b9b569b1904479b9c/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/5947ab6d4e7e3e9f5775208b9b569b1904479b9c/media/audio/cras/audio_manager_cras.h

Labels: -OS-Chrome
Project Member

Comment 41 by bugdroid1@chromium.org, May 25 2018

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

commit f84fffabdef28f8577bf4b8c5f5ec73e73991927
Author: Guido Urdaneta <guidou@chromium.org>
Date: Fri May 25 13:59:15 2018

Improve support for audio-device group IDs on Linux.

This is a reland of https://codereview.chromium.org/2258143002, originally authored by maxmorin@.
This version contains minor additions, lint fixes and restricts functionality to just non-ChromeOS
Linux.

This CL adds the following AudioManagerPulse overrides in order to better support group IDs:
  * GetDefaultInputDeviceID()
  * GetDefaultOutputDeviceID()
  * GetAssociatedOutputDeviceID()

The original CL was reverted due to regressions on ChromeOS with AudioManagerPulse. This CL keeps
behavior of AudioManagerPulse on ChromeOS unmodified.

Bug: 636300
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: Id48dc1477efd2fecd5ae111df22dfe2b744de6e0
Reviewed-on: https://chromium-review.googlesource.com/1072649
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561855}
[modify] https://crrev.com/f84fffabdef28f8577bf4b8c5f5ec73e73991927/media/audio/pulse/audio_manager_pulse.cc
[modify] https://crrev.com/f84fffabdef28f8577bf4b8c5f5ec73e73991927/media/audio/pulse/audio_manager_pulse.h
[modify] https://crrev.com/f84fffabdef28f8577bf4b8c5f5ec73e73991927/media/audio/pulse/pulse.sigs
[modify] https://crrev.com/f84fffabdef28f8577bf4b8c5f5ec73e73991927/media/audio/pulse/pulse_util.cc
[modify] https://crrev.com/f84fffabdef28f8577bf4b8c5f5ec73e73991927/media/audio/pulse/pulse_util.h

Sign in to add a comment