Link things together and enable the device selection for linux and mac.

Enable the device selection for linux and mac by passing a device unique id from renderer down to the browser.
With this patch, users should be able to create a audio stream with a non-default device, and feature is only available for low-latency audio.

TEST=media_unittests, webrtc test app
BUG=None



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111153

[no longer working on chromium, 2011-11-15 10:04:40]

This patch link thins together and allow us to open a non-default capture device
for the stream.

Henrika, Tommi, Andrew, Eugene: code style and general design.
Chris: mac part.
Brettw: owner stamp for content.

Thanks,
BR,
/SX

[tommi (sloooow) - chröme, 2011-11-15 10:29:07]

http://codereview.chromium.org/8491044/diff/11009/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/8491044/diff/11009/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:35: static
const char kInvalidDeviceUId[];
kInvalidDeviceUniqueID or perhaps just kInvalidDeviceID

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:17: static const char
kDefaultDeviceUId[] = "default";
isn't this the definition of the global constant?  Any reason for 'static'?

Also, should this be in a namespace?

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:133: std::string default_device_uid
= "";
skip the = "" part.  (and uid -> id)

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
File content/renderer/media/audio_input_device.h (right):

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.h:154: virtual void
OnDeviceReady(const std::string& device_uid);
also change foo_uid to foo_id.  ID is what we use for identifier and identifiers
tend to be unique, so no need for the extra 'u'.

[scherkus (not reviewing), 2011-11-16 01:20:40]

nice!

few nits

also where is the webrtc test app? sounds like something that should get
documented a little more clearly if other folks make changes

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:16: static const char
kDefaultDeviceUId[] = "default";
I don't see this used and it's also confusing with the code change below where
you pass in "" as default

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:374: input_channels = 2;
was this change intentional?

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:39: "surround",
??

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:271:
arraysize(kNotWantedSurroundDevices) -1))
nit space between -1

[henrika (OOO until Aug 14), 2011-11-16 13:24:02]

Done first round of comments. Did down to media/audio/audio_input_controller.cc.
Will take the rest tonight.

Need some changes, especially about callback names where Started() and Ready()
are mixed.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:114:
AudioManagerBase::kDefaultDeviceId);
We mix UId and Id here. Not sure why?

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:206: // Get
the up-to-date device enumeration list from the system and find out
I really hate fuzzy phrases like "the system" ;-(

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:207: // the
index of the device.
This comment is old.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:227: // Posts
the index to AudioInputRenderHost through the event handler.
new line?

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:287: // Post a
callback through the event handler to create an audio stream.
Please add more info here about what is the next step.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:30: // Calling
Start() with this kFakeOpenSessionId will open the default device,
This comment applies to the first const only. Could be more clear or comments
could be added for the others as well.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:72: void
StartedOnIOThread(int session_id, const std::string& device_uid);
u=unique?
Why emphasize for only one of the parameters?

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File
content/browser/renderer_host/media/audio_input_device_manager_event_handler.h
(right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager_event_handler.h:20:
const std::string& device_uid) = 0;
Not sure if we need 'u' here.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:343: // Notify
the renderer that the device has been started.
Perhaps make the comment more clear since the device is actually not really
started. Just verified.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:23: //
referenced by the session id, and a OnDeviceStarted() callback with the
an

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:24: // unique_id
of the opened device will be received later. Then it will send a
don't use variable names up here

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
File content/renderer/media/audio_input_message_filter.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
content/renderer/media/audio_input_message_filter.h:42: virtual void
OnDeviceReady(const std::string& index) = 0;
uid?
And why do you change name of callback here?

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:374: input_channels = 2;
Same question.

http://codereview.chromium.org/8491044/diff/17005/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/8491044/diff/17005/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:54: std::string default_device_uid = "";
You are mixing empty and "Default".

[no longer working on chromium, 2011-11-16 17:45:47]

Update a new version since the comments have been pending up.
Please take a second round.

Thanks,
/SX

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:114:
AudioManagerBase::kDefaultDeviceId);
On 2011/11/16 13:24:02, henrika1 wrote:
> We mix UId and Id here. Not sure why?

When it is kFakeOpenSessionId, which is 1, then we open default device
regardless if the device has been opened or not.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:206: // Get
the up-to-date device enumeration list from the system and find out
On 2011/11/16 13:24:02, henrika1 wrote:
> I really hate fuzzy phrases like "the system" ;-( 

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:207: // the
index of the device.
On 2011/11/16 13:24:02, henrika1 wrote:
> This comment is old.

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:227: // Posts
the index to AudioInputRenderHost through the event handler.
On 2011/11/16 13:24:02, henrika1 wrote:
> new line?

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.cc:287: // Post a
callback through the event handler to create an audio stream.
On 2011/11/16 13:24:02, henrika1 wrote:
> Please add more info here about what is the next step.

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:30: // Calling
Start() with this kFakeOpenSessionId will open the default device,
Add blank line, hope it is fine.

On 2011/11/16 13:24:02, henrika1 wrote:
> This comment applies to the first const only. Could be more clear or comments
> could be added for the others as well.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:72: void
StartedOnIOThread(int session_id, const std::string& device_uid);
On 2011/11/16 13:24:02, henrika1 wrote:
> u=unique?
> Why emphasize for only one of the parameters?

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:343: // Notify
the renderer that the device has been started.
On 2011/11/16 13:24:02, henrika1 wrote:
> Perhaps make the comment more clear since the device is actually not really
> started. Just verified.

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:23: //
referenced by the session id, and a OnDeviceStarted() callback with the
On 2011/11/16 13:24:02, henrika1 wrote:
> an

Done.

http://codereview.chromium.org/8491044/diff/17005/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:24: // unique_id
of the opened device will be received later. Then it will send a
On 2011/11/16 13:24:02, henrika1 wrote:
> don't use variable names up here

Done.

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:16: static const char
kDefaultDeviceUId[] = "default";
On 2011/11/16 01:20:40, scherkus wrote:
> I don't see this used and it's also confusing with the code change below where
> you pass in "" as default

Sorry, wrong code by mistake.

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
File content/renderer/media/audio_input_message_filter.h (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/audi...
content/renderer/media/audio_input_message_filter.h:42: virtual void
OnDeviceReady(const std::string& index) = 0;
On 2011/11/16 13:24:02, henrika1 wrote:
> uid?
> And why do you change name of callback here?

We can talk more on which name to use.

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:374: input_channels = 2;
On 2011/11/16 01:20:40, scherkus wrote:
> was this change intentional?

Yes. The testing shows that stereo is more widely supported than mono, and the
possibility of failing opening a non-default device in mono is quite high.

This change will be in par with windows, about 50 lines above, it has
// Use stereo recording on Windows since low-latency Core Audio (WASAPI) does
not support mono.
input_channels = 2;

http://codereview.chromium.org/8491044/diff/17005/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:374: input_channels = 2;
On 2011/11/16 13:24:02, henrika1 wrote:
> Same question.

See comment above.

http://codereview.chromium.org/8491044/diff/17005/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/8491044/diff/17005/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:54: std::string default_device_uid = "";
On 2011/11/16 13:24:02, henrika1 wrote:
> You are mixing empty and "Default".

I removed ""

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:39: "surround",
On 2011/11/16 01:20:40, scherkus wrote:
> ??

We would like to strip out all the surround devices like surround4.0,
surround5.0, ..etc.
But for other string like default, we only want to strip default device, but not
device like default-headset.
We have to distinguish two cases.

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8491044/diff/17005/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:271:
arraysize(kNotWantedSurroundDevices) -1))
On 2011/11/16 01:20:40, scherkus wrote:
> nit space between -1

Done.

[no longer working on chromium, 2011-11-16 17:47:21]

http://codereview.chromium.org/8491044/diff/11009/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/8491044/diff/11009/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager.h:35: static
const char kInvalidDeviceUId[];
On 2011/11/15 10:29:07, tommi wrote:
> kInvalidDeviceUniqueID or perhaps just kInvalidDeviceID

Done.

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:17: static const char
kDefaultDeviceUId[] = "default";
On 2011/11/15 10:29:07, tommi wrote:
> isn't this the definition of the global constant?  Any reason for 'static'?
> 
> Also, should this be in a namespace?

It has been removed.

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:133: std::string default_device_uid
= "";
On 2011/11/15 10:29:07, tommi wrote:
> skip the = "" part.  (and uid -> id)

Done.

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
File content/renderer/media/audio_input_device.h (right):

http://codereview.chromium.org/8491044/diff/11009/content/renderer/media/audi...
content/renderer/media/audio_input_device.h:154: virtual void
OnDeviceReady(const std::string& device_uid);
On 2011/11/15 10:29:07, tommi wrote:
> also change foo_uid to foo_id.  ID is what we use for identifier and
identifiers
> tend to be unique, so no need for the extra 'u'.

Done.

[scherkus (not reviewing), 2011-11-17 01:48:18]

owners LGTM on relevant folders -- will defer to henrik/tommi/etc for remainder
of review

[tommi (sloooow) - chröme, 2011-11-17 10:24:16]

http://codereview.chromium.org/8491044/diff/31001/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/31001/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:131: std::string device_id =
AudioManagerBase::kDefaultDeviceId;
Are kDefaultDeviceId and kInvalidDeviceId equal?

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:103: if (device_id ==
AudioManagerBase::kDefaultDeviceId)
When checking the device id, in some places, we use .empty(), in others we do
string compare.  Can we just consolidate that to always do .empty()?

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:269: if
(!strcmp(kInvalidAudioInputDevices[i], device_name) ||
nit: prefer strcmp() == 0 rather than !strcmp() for readability.  (the latter
reads as "not compares")

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
File media/audio/mac/audio_manager_mac.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:200: property_address.mSelector =
kAudioHardwarePropertyDefaultInputDevice;
nit: instead of if/else for this kind of initialization, just use a single
assignment and the ? : operator:

property_address.mSelector = is_input ? 
    kAudioHardwarePropertyDefaultInputDevice :
    kAudioHardwarePropertyDefaultOutputDevice;

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:223: 0, //translation_size,
are you going to remove these comments?

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:231: << " for AudioDeviceID ";
use {} when the body spans more than one line.

[henrika (OOO until Aug 14), 2011-11-17 11:26:14]

LGTM w/ nits and the following assumptions:

1) Verify WebRTC unit test for all platforms.
2) Verify essential media tests locally.
3) Add TODO to merge these parts to the WebRTC unit tests.

And update the design doc based on this CL. Thanks!

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:39: "surround",
Related to this CL?

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac_unittest.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac_unittest.cc:107:
CHANNEL_LAYOUT_STEREO, fs, 16, samples_per_packet), "default");
Use constant.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
File media/audio/mac/audio_manager_mac.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:210: } else {  // Non-default device.
comment on separate line

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:305: AudioDeviceID audio_device_id =
GetAudioDeviceIdByUId(true, device_id);
Lots of IDs here..
At lest add a comment explaining what goes on here. Unique device ID -> internal
audio device ID -> open stream for this audio device ID.

[no longer working on chromium, 2011-11-17 14:15:26]

Update the code based on the comments from Tommi and Henrik.
I will re-test the CL on Mac and windows to double check everything is working,
and updating the design doc is ongoing.

http://codereview.chromium.org/8491044/diff/31001/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/31001/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:131: std::string device_id =
AudioManagerBase::kDefaultDeviceId;
On 2011/11/17 10:24:16, tommi wrote:
> Are kDefaultDeviceId and kInvalidDeviceId equal?

No, they are not.
kDefaultDeviceId = "default".
kInvalidDeviceId = "".

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (left):

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:39: "surround",
On 2011/11/17 11:26:15, henrika1 wrote:
> Related to this CL?

Yes, I have already explained offline.

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:103: if (device_id ==
AudioManagerBase::kDefaultDeviceId)
On 2011/11/17 10:24:16, tommi wrote:
> When checking the device id, in some places, we use .empty(), in others we do
> string compare.  Can we just consolidate that to always do .empty()?

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:269: if
(!strcmp(kInvalidAudioInputDevices[i], device_name) ||
On 2011/11/17 10:24:16, tommi wrote:
> nit: prefer strcmp() == 0 rather than !strcmp() for readability.  (the latter
> reads as "not compares")

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac_unittest.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac_unittest.cc:107:
CHANNEL_LAYOUT_STEREO, fs, 16, samples_per_packet), "default");
On 2011/11/17 11:26:15, henrika1 wrote:
> Use constant.

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
File media/audio/mac/audio_manager_mac.cc (right):

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:200: property_address.mSelector =
kAudioHardwarePropertyDefaultInputDevice;
On 2011/11/17 10:24:16, tommi wrote:
> nit: instead of if/else for this kind of initialization, just use a single
> assignment and the ? : operator:
> 
> property_address.mSelector = is_input ? 
>     kAudioHardwarePropertyDefaultInputDevice :
>     kAudioHardwarePropertyDefaultOutputDevice;

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:210: } else {  // Non-default device.
On 2011/11/17 11:26:15, henrika1 wrote:
> comment on separate line

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:223: 0, //translation_size,
On 2011/11/17 10:24:16, tommi wrote:
> are you going to remove these comments?

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:231: << " for AudioDeviceID ";
On 2011/11/17 10:24:16, tommi wrote:
> use {} when the body spans more than one line.

Done.

http://codereview.chromium.org/8491044/diff/31001/media/audio/mac/audio_manag...
media/audio/mac/audio_manager_mac.cc:305: AudioDeviceID audio_device_id =
GetAudioDeviceIdByUId(true, device_id);
On 2011/11/17 11:26:15, henrika1 wrote:
> Lots of IDs here..
> At lest add a comment explaining what goes on here. Unique device ID ->
internal
> audio device ID -> open stream for this audio device ID.

Done.

[tommi (sloooow) - chröme, 2011-11-17 14:29:57]

lgtm

[jam, 2011-11-21 16:39:23]

lgtm

http://codereview.chromium.org/8491044/diff/41001/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/41001/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:16: 
nit: extra line

[no longer working on chromium, 2011-11-21 16:47:56]

Thank you for all. It is going to be landed.

BR,
-SX

http://codereview.chromium.org/8491044/diff/41001/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/8491044/diff/41001/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:16: 
On 2011/11/21 16:39:23, John Abd-El-Malek wrote:
> nit: extra line

Done.

[commit-bot: I haz the power, 2011-11-21 20:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/8491044/47001

[commit-bot: I haz the power, 2011-11-21 20:08:08]

Can't process patch for file media/audio/audio_manager.h.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-11-22 12:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/8491044/53035

[commit-bot: I haz the power, 2011-11-22 14:28:24]

Change committed as 111153

[clandestino73, 2011-11-22 14:42:04]

lgtm

[clandestino73, 2014-05-15 18:17:21]

On Tue, 22 Nov 2011 06:42:04 -0800, <clandestino73@gmail.com> wrote:

> lgtm
>
>
>
> http://codereview.chromium.org/8491044/


-- 
Using Opera's mail client: http://www.opera.com/mail/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.