Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2258143002: Add GetAssociatedOutputDeviceID support to pulse. (Closed)

Created:
3 years, 3 months ago by Max Morin
Modified:
3 years, 3 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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}

Patch Set 1 #

Patch Set 2 : Remove unneeded pulse signatures. #

Total comments: 1

Patch Set 3 : Split off Issue 2265883002. #

Patch Set 4 : Fix handling of default device. #

Patch Set 5 : Fix handling of default device. #

Total comments: 4

Patch Set 6 : Guidos comments. #

Total comments: 20

Patch Set 7 : Tommis comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -2 lines) Patch
M media/audio/pulse/audio_manager_pulse.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M media/audio/pulse/audio_manager_pulse.cc View 1 2 3 4 5 6 1 chunk +23 lines, -0 lines 0 comments Download
M media/audio/pulse/pulse.sigs View 1 2 3 4 5 6 3 chunks +4 lines, -2 lines 0 comments Download
M media/audio/pulse/pulse_util.h View 1 2 3 4 5 2 chunks +12 lines, -0 lines 0 comments Download
M media/audio/pulse/pulse_util.cc View 1 2 3 4 5 6 3 chunks +107 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (8 generated)
Max Morin
Guido: PTAL. I will add Tommi for owners review when it looks good to you ...
3 years, 3 months ago (2016-08-19 14:11:07 UTC) #3
Guido Urdaneta
Haven't checked everything yet, but take a look at the comment about the default device, ...
3 years, 3 months ago (2016-08-19 14:26:18 UTC) #4
Max Morin
PTAL when you are back in the office.
3 years, 3 months ago (2016-08-24 07:39:56 UTC) #7
Guido Urdaneta
lgtm % nits, but send it to a pulse owner https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio_manager_pulse.cc File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio_manager_pulse.cc#newcode186 ...
3 years, 3 months ago (2016-08-29 10:31:12 UTC) #8
Max Morin
Tommi: PTAL since you are the only owner of this code. https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio_manager_pulse.cc File media/audio/pulse/audio_manager_pulse.cc (right): ...
3 years, 3 months ago (2016-08-29 11:54:02 UTC) #10
tommi (sloooow) - chröme
https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audio_manager_pulse.cc File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audio_manager_pulse.cc#newcode187 media/audio/pulse/audio_manager_pulse.cc:187: const std::string& input = if GetRealDefaultDeviceId doesn't return a ...
3 years, 3 months ago (2016-08-29 13:02:26 UTC) #11
Max Morin
PTAL. https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audio_manager_pulse.cc File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audio_manager_pulse.cc#newcode187 media/audio/pulse/audio_manager_pulse.cc:187: const std::string& input = On 2016/08/29 13:02:25, tommi ...
3 years, 3 months ago (2016-08-29 14:04:23 UTC) #12
tommi (sloooow) - chröme
lgtm
3 years, 3 months ago (2016-08-29 16:08:17 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2258143002/120001
3 years, 3 months ago (2016-08-29 16:09:33 UTC) #16
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f Cr-Commit-Position: refs/heads/master@{#415024}
3 years, 3 months ago (2016-08-29 18:04:34 UTC) #18
David Tseng
3 years, 3 months ago (2016-09-08 22:23:02 UTC) #19
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2322083003/ by dtseng@chromium.org.

The reason for reverting is: Breaks audio playback on Linux Chrome OS
target_os = "chromeos" in gn args

( SoundsManager::Play fails)..

Powered by Google App Engine
This is Rietveld 408576698