Add GetAssociatedOutputDeviceID support to pulse.

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}

[Max Morin, 2016-08-19 14:08:55]

Description was changed from

==========
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 this 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,627793
==========

to

==========
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 this 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,627793
==========

[Max Morin, 2016-08-19 14:08:55]

maxmorin@chromium.org changed reviewers:
+ guidou@chromium.org

[Max Morin, 2016-08-19 14:11:07]

Guido: PTAL. I will add Tommi for owners review when it looks good to you and
he's back in the office.

[Guido Urdaneta, 2016-08-19 14:26:18]

Haven't checked everything yet, but take a look at the comment about the default
device, which requires some special treatment.

https://codereview.chromium.org/2258143002/diff/20001/media/audio/pulse/audio...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2258143002/diff/20001/media/audio/pulse/audio...
media/audio/pulse/audio_manager_pulse.cc:182: if (input_device_id ==
AudioDeviceDescription::kDefaultDeviceId) {
This check is wrong. The default output device is not by definition associated
with the default input device.

The associated output device should be in the same physical device as the input
device.

[Max Morin, 2016-08-22 08:32:46]

Description was changed from

==========
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 this 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,627793
==========

to

==========
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,627793
==========

[Max Morin, 2016-08-23 13:37:53]

Description was changed from

==========
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,627793
==========

to

==========
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
==========

[Max Morin, 2016-08-24 07:39:56]

PTAL when you are back in the office.

[Guido Urdaneta, 2016-08-29 10:31:12]

lgtm % nits, but send it to a pulse owner

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio...
media/audio/pulse/audio_manager_pulse.cc:186: std::string input;
use const ref to avoid copying in the case where input is not default

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/pulse...
media/audio/pulse/pulse_util.cc:457: bool input) {
Is it possible to use an enum instead of bool?

[Max Morin, 2016-08-29 11:49:45]

maxmorin@chromium.org changed reviewers:
+ tommi@chromium.org

[Max Morin, 2016-08-29 11:54:02]

Tommi: PTAL since you are the only owner of this code.

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/audio...
media/audio/pulse/audio_manager_pulse.cc:186: std::string input;
On 2016/08/29 10:31:12, Guido Urdaneta wrote:
> use const ref to avoid copying in the case where input is not default 

Done.

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2258143002/diff/80001/media/audio/pulse/pulse...
media/audio/pulse/pulse_util.cc:457: bool input) {
On 2016/08/29 10:31:12, Guido Urdaneta wrote:
> Is it possible to use an enum instead of bool?

Done.

[tommi (sloooow) - chröme, 2016-08-29 13:02:26]

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:187: const std::string& input =
if GetRealDefaultDeviceId doesn't return a reference, just change this to be a
non-ref, since that's what it'll effectively be.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:195: if (input_bus.empty()) {
no {}

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:198: std::string s =
pulse::GetOutputCorrespondingTo(input_mainloop_,
nit: return pulse::GetOutputCorrespondingTo(...);

or even

return input_bus.empty() ? input_bus : pulse::GetOutputCorrespondingTo(...);

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
File media/audio/pulse/pulse.sigs (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:23: pa_operation*
pa_context_get_source_info_by_index(pa_context* c, uint32_t idx,
pa_source_info_cb_t cb, void* userdata)
intentionally removing the semicolon?

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:46: int pa_proplist_contains (pa_proplist* p, const
char* key);
remove space before (

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:48: const char* pa_proplist_gets (pa_proplist* p,
const char* key);
same here

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:15: int strcmp(const char* s1, const char* s2);
any reason not to #include this declaration?

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:77: pa_threaded_mainloop* loop_;
const?  (as in pa_threaded_mainloop* const loop_;)

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:137: DefaultDevicesData* data =
reinterpret_cast<DefaultDevicesData*>(userdata);
nit: these sort of casts (casting from a void* to a specific type *) are usually
static_cast in Chromium.   I know reinterpret_cast is commonly used outside of
Chromium, but for consistency, let's use static_cast (see elsewhere in this file
for example).

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:463: if (type == RequestType::INPUT) {
remove {}

could also do

return type == RequestType::INPUT ? data.input_ : data.output_;

[Max Morin, 2016-08-29 14:04:23]

PTAL.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:187: const std::string& input =
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> if GetRealDefaultDeviceId doesn't return a reference, just change this to be a
> non-ref, since that's what it'll effectively be.

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:195: if (input_bus.empty()) {
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> no {}

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:198: std::string s =
pulse::GetOutputCorrespondingTo(input_mainloop_,
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> nit: return pulse::GetOutputCorrespondingTo(...);
> 
> or even
> 
> return input_bus.empty() ? input_bus : pulse::GetOutputCorrespondingTo(...);

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
File media/audio/pulse/pulse.sigs (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:23: pa_operation*
pa_context_get_source_info_by_index(pa_context* c, uint32_t idx,
pa_source_info_cb_t cb, void* userdata)
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> intentionally removing the semicolon?

Oops.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:46: int pa_proplist_contains (pa_proplist* p, const
char* key);
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> remove space before (

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse.sigs:48: const char* pa_proplist_gets (pa_proplist* p,
const char* key);
On 2016/08/29 13:02:25, tommi (chrömium) wrote:
> same here

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:15: int strcmp(const char* s1, const char* s2);
On 2016/08/29 13:02:26, tommi (chrömium) wrote:
> any reason not to #include this declaration?

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:77: pa_threaded_mainloop* loop_;
On 2016/08/29 13:02:26, tommi (chrömium) wrote:
> const?  (as in pa_threaded_mainloop* const loop_;)

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:137: DefaultDevicesData* data =
reinterpret_cast<DefaultDevicesData*>(userdata);
On 2016/08/29 13:02:26, tommi (chrömium) wrote:
> nit: these sort of casts (casting from a void* to a specific type *) are
usually
> static_cast in Chromium.   I know reinterpret_cast is commonly used outside of
> Chromium, but for consistency, let's use static_cast (see elsewhere in this
file
> for example).

Done.

https://codereview.chromium.org/2258143002/diff/100001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:463: if (type == RequestType::INPUT) {
On 2016/08/29 13:02:26, tommi (chrömium) wrote:
> remove {}
> 
> could also do
> 
> return type == RequestType::INPUT ? data.input_ : data.output_;

Done.

[tommi (sloooow) - chröme, 2016-08-29 16:08:16]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2016-08-29 16:08:17]

lgtm

[tommi (sloooow) - chröme, 2016-08-29 16:08:17]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2258143002/#ps120001
(title: "Tommis comments.")

[commit-bot: I haz the power, 2016-08-29 16:09:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 18:04:33]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-08-29 18:04:34]

Patchset 7 (id:??) landed as
https://crrev.com/cff5a1f78bd76f764a70efcdb9b908ff6a87c60f
Cr-Commit-Position: refs/heads/master@{#415024}

[David Tseng, 2016-09-08 22:23:02]

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)..