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

Issue 764228 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Default device lookup broken on Chrome OS

Project Member Reported by maxmorin@chromium.org, Sep 12 2017

Issue description

The new function GetPrimaryActiveOutputNodeOnMainThread from https://chromium-review.googlesource.com/c/chromium/src/+/637512 doesn't actually assign to the output pointer. This causes computations of the group id property to fail for the default output device.
 
What's the manual test step to show this bug?
Connect some device (like a headset), make sure it is selected as default in the system UI, and issue a device enumeration (for example on https://guidou.github.io/enumdemo3.html). In this case, the "default" device should have the same group id as the headset devices which doesn't have the "default" device id (on the test page, the group id is the long hex string in the last column). Instead, the "default" device now gets a different group id, since the audio manager doesn't identify which is the default device correctly.
Does audio_manager_unittest.cc have a test for this? Even if there's a test, there is no ChromeOS bot in chrome CQ.
Project Member

Comment 4 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

Components: OS>Kernel>Audio Internals>Media>Audio
Labels: Merge-Request-62
I filed issue 764291 about adding tests, but as you said tests couldn't currently run on the CQ anyways.

Requesting merge to M62: Not sure when the next beta release is, I'll let this bake in canary for a couple of days if release owner prefers.
Labels: -Merge-Request-62 Merge-Approved-62
Consider this merge approved, but please ensure a build of Chrome with this change makes it through the PFQ on ToT first, as the branch builders have no PFQ protection.
Project Member

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

Labels: -merge-approved-62 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

Status: Fixed (was: Started)

Sign in to add a comment