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

Issue 768792 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Webrtc uses pinned stream while it should not

Project Member Reported by cychiang@chromium.org, Sep 26 2017

Issue description

I tried on latest image 9975.0.

Steps:

1. Plug USB headset, select USB as output if it is not.
2. Go to appr.tc and join a room.
3. Plug 3.5mm Headphone.
4. Select 3.5mm Headphone as output.
5. Play youtube.
6. Hear the WebRTC audio at USB, and youtube audio at 3.5mm Headphone. This suggests that WebRTC uses pinned stream on USB.

From the test result of audio_AudioWebRTCLoopback, https://wmatrix.googleplex.com/matrix/unfiltered?tests=audio_AudioWebRTCLoopback&days_back=20&hide_missing=True

9942.0 is good and 9945.0 is bad.

The blamelist of Chrome CLs:
https://chromium.googlesource.com/chromium/src/+log/63.0.3212.0..63.0.3216.0?pretty=fuller&n=10000

I would guess it is this one: https://chromium-review.googlesource.com/662761




 
Cc: cychiang@chromium.org
Owner: maxmorin@chromium.org
I verified that it is indeed the culprit.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc?q=GetAssociatedOutputDeviceID&sq=package:chromium&dr=C&l=274

Note that if device id is passed to CrasUnifiedStream, it means it is a pinned stream.

This is like https://chromium-review.googlesource.com/c/chromium/src/+/538754 which accidentally used pinned stream.

Max, could you please take a look?

Thanks!
Hmm, I'm getting more convinced it's a conceptual error to associate devices/assign group ids to the "default" devices, since they don't correspond to physical devices. They should probably just be treated as different from any other device.
I don't have a working Chromium checkout right now (I'm travelling, and somehow the checkout on my laptop got corrupted), but I  https://cs.chromium.org/chromium/src/media/audio/cras/audio_manager_cras.cc?type=cs&q=audiomanagercras&sq=package:chromium&l=207 should be "AudioDeviceDescription::kDefaultDeviceId" and AudioManagerCras::GetDefaultOutputDeviceID() should return "".
Sorry, line 207 should be "return AudioDeviceDescription::kDefaultDeviceId".
Great! Your suggestion fixed the problem.
Is it possible for you to make the change, or should I take it ?
Thanks!
I posted the CL: https://chromium-review.googlesource.com/#/c/chromium/src/+/688381/

Please take a look, thanks!
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 28 2017

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

commit eb5a4358523b8874ea6484796bfd31b73f83d075
Author: Cheng-Yi Chiang <cychiang@chromium.org>
Date: Thu Sep 28 10:07:01 2017

Fix audio_manager_cras GetAssociatedOutputDeviceID for default device

Fix the bug in

crrev.com/c/662761 :Fix group id for default devices on Chrome OS.

WebRTC incorrectly uses pinned stream on ChromeOS.

Default device does not correspond to physical device, so there is no
associated device for it.

BUG= 768792 
TEST=On ChromeOS, check audio output from WebRTC can be routed to
different output device.

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: I295aa4072b44d5d124d3e27c1894bce3f3085860
Reviewed-on: https://chromium-review.googlesource.com/688381
Commit-Queue: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504956}
[modify] https://crrev.com/eb5a4358523b8874ea6484796bfd31b73f83d075/media/audio/cras/audio_manager_cras.cc

Labels: Merge-Request-62
Add merge request to R62 for CL in #8.
Thanks!
Project Member

Comment 10 by sheriffbot@chromium.org, Oct 12 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: We are only 4 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: bhthompson@chromium.org
Hi Bernie,
I am sorry for the late request.
The CL https://chromium-review.googlesource.com/#/c/688381/ was simple so it is safe to get merged.
I have verified WebRTC works on 10024.0.

Thanks!
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 12 2017

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

commit ceb41ed3cea56d876f40c720c78f4f40a79bfe86
Author: Max Morin <maxmorin@chromium.org>
Date: Thu Oct 12 11:18:18 2017

(cros) Associate "default" input to "default" output.

The current implementation, associating "default" input to output based
on hardware, has unwanted consequences. Thus, we associate "default" to
"" instead. We also override "Get{In|Out}putGroupId" in
AudioManagerCras to make sure it's still based on the underlying
hardware device.

BUG= 768792 
TBR=cychiang

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: I8c7ba3b0c2ab16645352b8e32b499f71e219fcc8
Reviewed-on: https://chromium-review.googlesource.com/688797
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508311}
[modify] https://crrev.com/ceb41ed3cea56d876f40c720c78f4f40a79bfe86/media/audio/audio_manager_base.h
[modify] https://crrev.com/ceb41ed3cea56d876f40c720c78f4f40a79bfe86/media/audio/cras/audio_manager_cras.cc
[modify] https://crrev.com/ceb41ed3cea56d876f40c720c78f4f40a79bfe86/media/audio/cras/audio_manager_cras.h

Labels: -Hotlist-Merge-Review -Merge-Review-62 Merge-Approved-62
Approved for 62.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 13 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f6d967ea2c9c65fa2787463c07e4fa0031110ce

commit 1f6d967ea2c9c65fa2787463c07e4fa0031110ce
Author: Cheng-Yi Chiang <cychiang@chromium.org>
Date: Fri Oct 13 06:16:40 2017

Fix audio_manager_cras GetAssociatedOutputDeviceID for default device

Fix the bug in

crrev.com/c/662761 :Fix group id for default devices on Chrome OS.

WebRTC incorrectly uses pinned stream on ChromeOS.

Default device does not correspond to physical device, so there is no
associated device for it.

BUG= 768792 
TEST=On ChromeOS, check audio output from WebRTC can be routed to
different output device.

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: I295aa4072b44d5d124d3e27c1894bce3f3085860
Reviewed-on: https://chromium-review.googlesource.com/688381
Commit-Queue: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#504956}(cherry picked from commit eb5a4358523b8874ea6484796bfd31b73f83d075)
Reviewed-on: https://chromium-review.googlesource.com/718256
Cr-Commit-Position: refs/branch-heads/3202@{#678}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/1f6d967ea2c9c65fa2787463c07e4fa0031110ce/media/audio/cras/audio_manager_cras.cc

Status: Fixed (was: Assigned)
Thanks Bernie and Wucheng!
Issue 766782 has been merged into this issue.

Comment 18 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 19 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment