Adds input device selection on Windows.

Adds input device selection on Windows.

This CL does not change anything for any other audio client but the WebRTC clients.
All other clients can ignore this new functionality and will still use the default
input device as before.

BUG=none
TEST=special version of content_unittests --gtest_filter=WebRTCAudioDeviceTest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113173

[henrika (OOO until Aug 14), 2011-12-01 13:35:38]

Expectations on reviewers:

xians: architecture and functionality
tommi: C++ guru
sherkus: owner in media and final judgement

FYI - we are working on more advanced tests to verify the input device
selection. Currently, hand-made versions of unit tests in media and
content_renderer have been utilized for tests.

[tommi (sloooow) - chröme, 2011-12-01 14:11:03]

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.cc:383: HRESULT
WASAPIAudioInputStream::SetCaptureDevice(const std::string& device_id) {
Shouldn't we store this device_id in this function?
I know it's called elsewhere and the argument that's passed to it is device_id_,
but can't this function be called with a different value?
If not, is there a reason for having the argument at all? (shouldn't we just use
device_id_?)

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
File media/audio/win/audio_low_latency_input_win.h (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.h:106: HRESULT
SetCaptureDevice(const std::string& device_id);
if the device_id has already been handed to the class via the constructor,
shouldn't we skip passing it as a parameter here?
This is a bit confusing in the cc file.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:211: if
(!GetInputDeviceNamesWinXP(&device_names)) {
nit: no need for {} (like the the next statement)

[no longer working on chromium, 2011-12-01 15:28:03]

Looks really good.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.cc:83: // device with the specified
unique identifier or role.
it only takes unique id. remove "or role".

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:223: while (it != device_names.end()) {
what happen if none of the device in the list matches the device_id_? Shall you
return false instead?

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:224: if (it->unique_id.compare(device_id_)
== 0) {
we can directly use device_id_ == it->unique_id

[henrika (OOO until Aug 14), 2011-12-01 16:02:38]

Changed according to input from reviewers.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.cc:83: // device with the specified
unique identifier or role.
But one of the IDs is "default" and that is a role.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.cc:383: HRESULT
WASAPIAudioInputStream::SetCaptureDevice(const std::string& device_id) {
On 2011/12/01 14:11:03, tommi wrote:
> Shouldn't we store this device_id in this function?
> I know it's called elsewhere and the argument that's passed to it is
device_id_,
> but can't this function be called with a different value?
> If not, is there a reason for having the argument at all? (shouldn't we just
use
> device_id_?)

Done.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
File media/audio/win/audio_low_latency_input_win.h (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_laten...
media/audio/win/audio_low_latency_input_win.h:106: HRESULT
SetCaptureDevice(const std::string& device_id);
On 2011/12/01 14:11:03, tommi wrote:
> if the device_id has already been handed to the class via the constructor,
> shouldn't we skip passing it as a parameter here?
> This is a bit confusing in the cc file.

Done.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:211: if
(!GetInputDeviceNamesWinXP(&device_names)) {
On 2011/12/01 14:11:03, tommi wrote:
> nit: no need for {} (like the the next statement)

Done.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:223: while (it != device_names.end()) {
Done.

http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_wi...
media/audio/win/wavein_input_win.cc:224: if (it->unique_id.compare(device_id_)
== 0) {
I actually prefer the compare() method for std::sting. Hope that is OK.

[no longer working on chromium, 2011-12-01 16:15:22]

lgtm

[tommi (sloooow) - chröme, 2011-12-01 17:28:15]

Lgtm
On Dec 1, 2011 5:02 PM, <henrika@chromium.org> wrote:

> Changed according to input from reviewers.
>
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/audio_low_latency_**input_win.cc<http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_latency_input_win.cc>
> File media/audio/win/audio_low_**latency_input_win.cc (right):
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/audio_low_latency_**input_win.cc#newcode83<http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_latency_input_win.cc#newcode83>
> media/audio/win/audio_low_**latency_input_win.cc:83: // device with the
> specified unique identifier or role.
> But one of the IDs is "default" and that is a role.
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/audio_low_latency_**input_win.cc#newcode383<http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_latency_input_win.cc#newcode383>
> media/audio/win/audio_low_**latency_input_win.cc:383: HRESULT
> WASAPIAudioInputStream::**SetCaptureDevice(const std::string& device_id) {
> On 2011/12/01 14:11:03, tommi wrote:
>
>> Shouldn't we store this device_id in this function?
>> I know it's called elsewhere and the argument that's passed to it is
>>
> device_id_,
>
>> but can't this function be called with a different value?
>> If not, is there a reason for having the argument at all? (shouldn't
>>
> we just use
>
>> device_id_?)
>>
>
> Done.
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/audio_low_latency_**input_win.h<http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_latency_input_win.h>
> File media/audio/win/audio_low_**latency_input_win.h (right):
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/audio_low_latency_**input_win.h#newcode106<http://codereview.chromium.org/8770005/diff/1/media/audio/win/audio_low_latency_input_win.h#newcode106>
> media/audio/win/audio_low_**latency_input_win.h:106: HRESULT
> SetCaptureDevice(const std::string& device_id);
> On 2011/12/01 14:11:03, tommi wrote:
>
>> if the device_id has already been handed to the class via the
>>
> constructor,
>
>> shouldn't we skip passing it as a parameter here?
>> This is a bit confusing in the cc file.
>>
>
> Done.
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/wavein_input_win.cc<http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_win.cc>
> File media/audio/win/wavein_input_**win.cc (right):
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/wavein_input_win.cc#**newcode211<http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_win.cc#newcode211>
> media/audio/win/wavein_input_**win.cc:211: if
> (!GetInputDeviceNamesWinXP(&**device_names)) {
> On 2011/12/01 14:11:03, tommi wrote:
>
>> nit: no need for {} (like the the next statement)
>>
>
> Done.
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/wavein_input_win.cc#**newcode223<http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_win.cc#newcode223>
> media/audio/win/wavein_input_**win.cc:223: while (it !=
> device_names.end()) {
> Done.
>
> http://codereview.chromium.**org/8770005/diff/1/media/**
>
audio/win/wavein_input_win.cc#**newcode224<http://codereview.chromium.org/8770005/diff/1/media/audio/win/wavein_input_win.cc#newcode224>
> media/audio/win/wavein_input_**win.cc:224: if
> (it->unique_id.compare(device_**id_) == 0) {
> I actually prefer the compare() method for std::sting. Hope that is OK.
>
>
http://codereview.chromium.**org/8770005/<http://codereview.chromium.org/8770...
>

[scherkus (not reviewing), 2011-12-06 02:38:05]

lgtm

lgtm

[commit-bot: I haz the power, 2011-12-06 02:38:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/henrika@chromium.org/8770005/4003

[commit-bot: I haz the power, 2011-12-06 02:38:21]

Can't apply patch for file media/audio/win/audio_manager_win.cc.
While running patch -p0 --forward --force;
patching file media/audio/win/audio_manager_win.cc
Hunk #1 FAILED at 165.
1 out of 1 hunk FAILED -- saving rejects to file
media/audio/win/audio_manager_win.cc.rej