Adding microphone volume support to chrome.

Adding microphone volume support to chrome in linux. Totally there are 3 APIs are added: Get/SetMicVolume() and GetMaxMicVolume(). And a new AudioInputVolumeTest is also added to media_unittests.




BUG=115087
TEST=media_unittests

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

[no longer working on chromium, 2012-02-21 10:26:44]

Please review, and feel free to add more reviewers if you think they should be
involved.

Thanks.

BR,
/SX

[tommi (sloooow) - chröme, 2012-02-21 14:03:24]

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input.cc
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:71: pcm_format, latency_us);
instead of assigning to device_name and copy paste the code, use a data driven
approach:

const char* device_names[] = { kDefaultDevice1, kDefaultDevice2 };
for (size_t i = 0; i < arraysize(device_names); ++i) {
  device_handle_ = alsa_util::OpenCaptureDevice(
      wrapper_, device_names[i], ...);
  if (device_handle_) {
    device_name_ = device_names[i];
    break;
  }
}

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:279: DLOG(WARNING) << "Unable to set volume for
" << device_name_;
do we never intend to support error notifications for these apis?

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:296: *volume =
static_cast<double>(current_volume);
this should be inside an else {}.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:313: mixer_element_handle_, &min, &max)) {
indentation feels off to me.  Looks like this:

Foo(
x, y, z)

and not
Foo(
    x, y, z)

Does the first parameter (at least) not fit on the line above?

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util.cc
File media/audio/linux/alsa_util.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:36: if (alsa_util::CloseDevice(wrapper, handle))
{
should this be
if (alsa_util::CloseDevice(...) < 0) {
?

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:56: size_t pos2 = device_name.find(",");
nit: when searching for a single character, use ' and not ".  The difference is
that you go through the substring search algorithm instead of character search -
the latter being much simpler.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:162: LOG(WARNING) << "MixerClose: " <<
device_name << ", "
There's a lot of release logging code in this file.  Do we need it?

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:176: snd_mixer_elem_t* micElem = NULL;
mic_elem

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:183: const char *selemName =
wrapper->MixerSelemName(elem);
const char* selemName

actually... it should be selem_name or better yet elem_name... what's a 'selem'
anyway? :)

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:190: }
should this also assign 0.0 to volume? (as GetMaxMicVolume does?)

[henrika (OOO until Aug 14), 2012-02-21 14:03:47]

First round. Did not check the Linux part this time.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:36: double max_volume = 0.0;
We could use bool return value as indicator here as well.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:46:
(media::GetAudioInputHardwareChannelCount() == 1)?
add space after )

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:111: << ", does not have volume
control";
End with period I guess.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:120: // Gets the max volume.
Redundant comment.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:124: // Notes that |original_volume|
can be higher than |max_volume| on Linux.
Not sure if I understand this comment.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:145: double max_step_size =
max_volume / 8;
We discussed this part and the code is not OK here. Many things can go wrong.
Please modify.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_io.h#newco...
media/audio/audio_io.h:165: virtual void GetMaxMicVolume(double* max_volume) =
0;
What about moving this API up to make it more clear what max_volume is? Also, we
could use bool here instead.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_input_...
File media/audio/mac/audio_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_input_...
media/audio/mac/audio_input_mac.cc:117: NOTREACHED();
Add comment here perhaps? Or rather NOTREACHED() << "Only supported for
low-latency."

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:215: number_of_channels_in_frame_
= GetNumberOfChannelsFromStream();
Why adding "FromStream()"?

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:297: // There is no master volume
control, try to set volume for each channel.
Can there be one master volume and channel volumes?

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:350: int success = 0;
Not clear that this is a counter.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:576: UInt32
AUAudioInputStream::GetNumberOfChannelsFromStream() {
Why not the same APIs as in the audio manager for Mac?

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:614: return (result == noErr)?
is_settable : false;
Same nit again. Please use ") ? is_"

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:190: }
Add TODO.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:193: *max_volume = 0;
Add TODO.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:195: }
Add TODO.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:198: *max_volume = 0;
Add TODO.

http://codereview.chromium.org/9418042/diff/9001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/9418042/diff/9001/media/media.gyp#newcode571
media/media.gyp:571: 'audio/audio_input_volume_unittest.cc',
wrong order

[no longer working on chromium, 2012-02-21 18:01:38]

I have addressed all the comments, please take another round.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:36: double max_volume = 0.0;
On 2012/02/21 14:03:47, henrika wrote:
> We could use bool return value as indicator here as well.

Sure. Another alternative is to have one more API call bool HasVolumeControl()
I am open to the ideas, maybe we can discuss with Tommi about it.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:46:
(media::GetAudioInputHardwareChannelCount() == 1)?
On 2012/02/21 14:03:47, henrika wrote:
> add space after )

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:111: << ", does not have volume
control";
On 2012/02/21 14:03:47, henrika wrote:
> End with period I guess.

I searched the code, and found out that we don't do that in chrome.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:120: // Gets the max volume.
On 2012/02/21 14:03:47, henrika wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:124: // Notes that |original_volume|
can be higher than |max_volume| on Linux.
On 2012/02/21 14:03:47, henrika wrote:
> Not sure if I understand this comment.

I try to explain why I put EXPECT_LE(original_volume, max_volume) under #if
defined(OS_WIN) || defined(OS_MACOSX) 
It is because users can set the volume above 100% in Linux.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:145: double max_step_size =
max_volume / 8;
On 2012/02/21 14:03:47, henrika wrote:
> We discussed this part and the code is not OK here. Many things can go wrong.
> Please modify.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/audio_io.h#newco...
media/audio/audio_io.h:165: virtual void GetMaxMicVolume(double* max_volume) =
0;
On 2012/02/21 14:03:47, henrika wrote:
> What about moving this API up to make it more clear what max_volume is? Also,
we
> could use bool here instead.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input.cc
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:71: pcm_format, latency_us);
On 2012/02/21 14:03:25, tommi wrote:
> instead of assigning to device_name and copy paste the code, use a data driven
> approach:
> 
> const char* device_names[] = { kDefaultDevice1, kDefaultDevice2 };
> for (size_t i = 0; i < arraysize(device_names); ++i) {
>   device_handle_ = alsa_util::OpenCaptureDevice(
>       wrapper_, device_names[i], ...);
>   if (device_handle_) {
>     device_name_ = device_names[i];
>     break;
>   }
> }

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:279: DLOG(WARNING) << "Unable to set volume for
" << device_name_;
On 2012/02/21 14:03:25, tommi wrote:
> do we never intend to support error notifications for these apis?

My concern is that we may need to use IPC message to Get/Set() volume, so we may
not want returned value for those API. And failure in the volume APIs may not be
considered as critical, so in the future if we want to handle the error, we can
add something like HandleWarning().

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:296: *volume =
static_cast<double>(current_volume);
On 2012/02/21 14:03:25, tommi wrote:
> this should be inside an else {}.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:313: mixer_element_handle_, &min, &max)) {
On 2012/02/21 14:03:25, tommi wrote:
> indentation feels off to me.  Looks like this:
> 
> Foo(
> x, y, z)
> 
> and not
> Foo(
>     x, y, z)
> 
> Does the first parameter (at least) not fit on the line above?

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util.cc
File media/audio/linux/alsa_util.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:36: if (alsa_util::CloseDevice(wrapper, handle))
{
On 2012/02/21 14:03:25, tommi wrote:
> should this be
> if (alsa_util::CloseDevice(...) < 0) {
> ?

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:56: size_t pos2 = device_name.find(",");
On 2012/02/21 14:03:25, tommi wrote:
> nit: when searching for a single character, use ' and not ".  The difference
is
> that you go through the substring search algorithm instead of character search
-
> the latter being much simpler.

Good tips, thanks.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:162: LOG(WARNING) << "MixerClose: " <<
device_name << ", "
On 2012/02/21 14:03:25, tommi wrote:
> There's a lot of release logging code in this file.  Do we need it?

We can discuss it more. These ALSA functions are only called when creating the
stream, and it has possibility to fail since they are dealing with hardware. I
just think it might be good to know what happen if the users failed to open the
soundcard or the mixer.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:176: snd_mixer_elem_t* micElem = NULL;
On 2012/02/21 14:03:25, tommi wrote:
> mic_elem

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/linux/alsa_util....
media/audio/linux/alsa_util.cc:183: const char *selemName =
wrapper->MixerSelemName(elem);
On 2012/02/21 14:03:25, tommi wrote:
> const char* selemName
> 
> actually... it should be selem_name or better yet elem_name... what's a
'selem'
> anyway? :)

Thanks, I used elem_name then. It stands for "simple element"

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_input_...
File media/audio/mac/audio_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_input_...
media/audio/mac/audio_input_mac.cc:117: NOTREACHED();
On 2012/02/21 14:03:47, henrika wrote:
> Add comment here perhaps? Or rather NOTREACHED() << "Only supported for
> low-latency."

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:215: number_of_channels_in_frame_
= GetNumberOfChannelsFromStream();
On 2012/02/21 14:03:47, henrika wrote:
> Why adding "FromStream()"?

I want to make it more specific to differentiate this
number_of_channels_in_frame_ from the channel number in AudioParams, because
they can be different. For example, if we try to open the device in mono mode,
we can the number_of_channels_in_frame_ can still be 2.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:297: // There is no master volume
control, try to set volume for each channel.
On 2012/02/21 14:03:47, henrika wrote:
> Can there be one master volume and channel volumes?

If it supports master volume, we use master volume.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:350: int success = 0;
On 2012/02/21 14:03:47, henrika wrote:
> Not clear that this is a counter.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:576: UInt32
AUAudioInputStream::GetNumberOfChannelsFromStream() {
On 2012/02/21 14:03:47, henrika wrote:
> Why not the same APIs as in the audio manager for Mac?

I explored a bit on the apple documentation. It seems the code in audio manager
is to get the channel numbers in the device, and the code here is getting the
channel number of a specific stream.
But it still not sure if if this kAudioDevicePropertyStreamFormat is obsolete or
not.

http://codereview.chromium.org/9418042/diff/9001/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:614: return (result == noErr)?
is_settable : false;
On 2012/02/21 14:03:47, henrika wrote:
> Same nit again. Please use ") ? is_"

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:190: }
On 2012/02/21 14:03:25, tommi wrote:
> should this also assign 0.0 to volume? (as GetMaxMicVolume does?)

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:190: }
On 2012/02/21 14:03:47, henrika wrote:
> Add TODO.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:193: *max_volume = 0;
On 2012/02/21 14:03:47, henrika wrote:
> Add TODO.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:195: }
On 2012/02/21 14:03:47, henrika wrote:
> Add TODO.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:198: *max_volume = 0;
On 2012/02/21 14:03:47, henrika wrote:
> Add TODO.

Done.

http://codereview.chromium.org/9418042/diff/9001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/9418042/diff/9001/media/media.gyp#newcode571
media/media.gyp:571: 'audio/audio_input_volume_unittest.cc',
On 2012/02/21 14:03:47, henrika wrote:
> wrong order

Done.

[scherkus (not reviewing), 2012-02-21 19:20:01]

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newc...
media/audio/audio_io.h:159: virtual void GetMaxMicVolume(double* max_volume) =
0;
do we really want to introduce "Mic" into our naming scheme?

the class in question is already an AudioInputStream -- would dropping "Mic"
from these method names be sufficient?

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newc...
media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) = 0;
any reason why these getters need to use out-params? I wouldn't copy
AudioOutputStream -- it should also return a double instead of using an
out-param

it looks like this was changed a long time ago when Get/SetVolume used to have
separate left/right volumes:
http://src.chromium.org/viewvc/chrome?view=rev&revision=31018

[henrika (OOO until Aug 14), 2012-02-21 22:19:40]

Andrew,

one comment regarding adding "Mic" to the names. The idea here was to
separate it from a digital volume scaling and emphasize that the API does
in fact affect the analog level. i.e., the slider of any selected
microphone device will be affected.

Does it make sense?

On Tue, Feb 21, 2012 at 8:20 PM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.**org/9418042/diff/10004/media/**
>
audio/audio_io.h<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h>
> File media/audio/audio_io.h (right):
>
> http://codereview.chromium.**org/9418042/diff/10004/media/**
>
audio/audio_io.h#newcode159<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode159>
> media/audio/audio_io.h:159: virtual void GetMaxMicVolume(double*
> max_volume) = 0;
> do we really want to introduce "Mic" into our naming scheme?
>
> the class in question is already an AudioInputStream -- would dropping
> "Mic" from these method names be sufficient?
>
> http://codereview.chromium.**org/9418042/diff/10004/media/**
>
audio/audio_io.h#newcode165<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode165>
> media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) =
> 0;
> any reason why these getters need to use out-params? I wouldn't copy
> AudioOutputStream -- it should also return a double instead of using an
> out-param
>
> it looks like this was changed a long time ago when Get/SetVolume used
> to have separate left/right volumes:
>
http://src.chromium.org/**viewvc/chrome?view=rev&**revision=31018<http://src....
>
>
http://codereview.chromium.**org/9418042/<http://codereview.chromium.org/9418...
>

[tommi (sloooow) - chröme, 2012-02-21 22:26:52]

It's not specific to a mic (as opposed to other input devices) is it?
Would 'gain' be a better word than 'volume' in this context? As in input
gain.
On Feb 21, 2012 11:19 PM, "Henrik Andreasson" <henrika@chromium.org> wrote:

> Andrew,
>
> one comment regarding adding "Mic" to the names. The idea here was to
> separate it from a digital volume scaling and emphasize that the API does
> in fact affect the analog level. i.e., the slider of any selected
> microphone device will be affected.
>
> Does it make sense?
>
> On Tue, Feb 21, 2012 at 8:20 PM, <scherkus@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h>
>> File media/audio/audio_io.h (right):
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h#newcode159<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode159>
>> media/audio/audio_io.h:159: virtual void GetMaxMicVolume(double*
>> max_volume) = 0;
>> do we really want to introduce "Mic" into our naming scheme?
>>
>> the class in question is already an AudioInputStream -- would dropping
>> "Mic" from these method names be sufficient?
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h#newcode165<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode165>
>> media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) =
>> 0;
>> any reason why these getters need to use out-params? I wouldn't copy
>> AudioOutputStream -- it should also return a double instead of using an
>> out-param
>>
>> it looks like this was changed a long time ago when Get/SetVolume used
>> to have separate left/right volumes:
>>
http://src.chromium.org/**viewvc/chrome?view=rev&**revision=31018<http://src....
>>
>>
http://codereview.chromium.**org/9418042/<http://codereview.chromium.org/9418...
>>
>
>

[no longer working on chromium, 2012-02-21 22:27:47]

I was about to discuss with you guys on the names. It looks to me that we
may deprecate those digital scaling output volume functions, or even change
them to use analog volume instead. If it is true, we'd better remove the
"Mic".
On Feb 21, 2012 11:19 PM, "Henrik Andreasson" <henrika@chromium.org> wrote:

> Andrew,
>
> one comment regarding adding "Mic" to the names. The idea here was to
> separate it from a digital volume scaling and emphasize that the API does
> in fact affect the analog level. i.e., the slider of any selected
> microphone device will be affected.
>
> Does it make sense?
>
> On Tue, Feb 21, 2012 at 8:20 PM, <scherkus@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h>
>> File media/audio/audio_io.h (right):
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h#newcode159<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode159>
>> media/audio/audio_io.h:159: virtual void GetMaxMicVolume(double*
>> max_volume) = 0;
>> do we really want to introduce "Mic" into our naming scheme?
>>
>> the class in question is already an AudioInputStream -- would dropping
>> "Mic" from these method names be sufficient?
>>
>> http://codereview.chromium.**org/9418042/diff/10004/media/**
>>
audio/audio_io.h#newcode165<http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newcode165>
>> media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) =
>> 0;
>> any reason why these getters need to use out-params? I wouldn't copy
>> AudioOutputStream -- it should also return a double instead of using an
>> out-param
>>
>> it looks like this was changed a long time ago when Get/SetVolume used
>> to have separate left/right volumes:
>>
http://src.chromium.org/**viewvc/chrome?view=rev&**revision=31018<http://src....
>>
>>
http://codereview.chromium.**org/9418042/<http://codereview.chromium.org/9418...
>>
>
>

[no longer working on chromium, 2012-02-22 17:02:23]

Changed the volume interface based on the comments from Andrew.

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newc...
media/audio/audio_io.h:159: virtual void GetMaxMicVolume(double* max_volume) =
0;
On 2012/02/21 19:20:01, scherkus wrote:
> do we really want to introduce "Mic" into our naming scheme?
> 
> the class in question is already an AudioInputStream -- would dropping "Mic"
> from these method names be sufficient?

Done, removed the "Mic".

[no longer working on chromium, 2012-02-22 17:27:24]

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newc...
media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) = 0;
On 2012/02/21 19:20:01, scherkus wrote:
> any reason why these getters need to use out-params? I wouldn't copy
> AudioOutputStream -- it should also return a double instead of using an
> out-param
> 
> it looks like this was changed a long time ago when Get/SetVolume used to have
> separate left/right volumes:
> http://src.chromium.org/viewvc/chrome?view=rev&revision=31018

Thanks for pointing out, I was assuming the code in AudioOutputStream should be
all right.

[scherkus (not reviewing), 2012-02-23 05:48:59]

On 2012/02/22 17:27:24, xians1 wrote:
> http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h
> File media/audio/audio_io.h (right):
> 
>
http://codereview.chromium.org/9418042/diff/10004/media/audio/audio_io.h#newc...
> media/audio/audio_io.h:165: virtual void GetMicVolume(double* volume) = 0;
> On 2012/02/21 19:20:01, scherkus wrote:
> > any reason why these getters need to use out-params? I wouldn't copy
> > AudioOutputStream -- it should also return a double instead of using an
> > out-param
> > 
> > it looks like this was changed a long time ago when Get/SetVolume used to
have
> > separate left/right volumes:
> > http://src.chromium.org/viewvc/chrome?view=rev&revision=31018
> 
> Thanks for pointing out, I was assuming the code in AudioOutputStream should
be
> all right.

I believe it's always healthy to question the code you're hacking on :)

Thanks for making the changes!

[henrika (OOO until Aug 14), 2012-02-23 11:50:41]

Almost there. Please sort out the stream latency parts for Mac  first. Should
not be in this CL I guess.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:123: // Tries to set the volume to
the |max_volume|.
nit, I would skip second "the"

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:128: // Tries to set the volume to 0.
I would use zero or mute instead of 0.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:134: // Depending on the step size of
the volume control, the value that we get
This comment doesn't really describe the test.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_io.h#newc...
media/audio/audio_io.h:158: // Returns the maximum microphone analog volume.
Perhaps add a comment about return value <=> error?

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:275: // If the volume is
settable, the  valid volume range is [0.0, 1.0].
How do you know that this is a true statement?

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:385: DLOG(WARNING) <<
"AudioObjectGetPropertyData failed not get volume";
Clean up the comment. "failed not get volume" ...

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:514: // Get the stream latency.
I don't understand where this part comes from. Not related to this CL.

[no longer working on chromium, 2012-02-24 09:27:48]

Changed the code based on the comments from Henrik.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:123: // Tries to set the volume to
the |max_volume|.
On 2012/02/23 11:50:41, henrika wrote:
> nit, I would skip second "the"

Done.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:128: // Tries to set the volume to 0.
On 2012/02/23 11:50:41, henrika wrote:
> I would use zero or mute instead of 0.

Done.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:134: // Depending on the step size of
the volume control, the value that we get
On 2012/02/23 11:50:41, henrika wrote:
> This comment doesn't really describe the test.

Done.

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_io.h
File media/audio/audio_io.h (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/audio_io.h#newc...
media/audio/audio_io.h:158: // Returns the maximum microphone analog volume.
On 2012/02/23 11:50:41, henrika wrote:
> Perhaps add a comment about return value <=> error?

Done.

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:275: // If the volume is
settable, the  valid volume range is [0.0, 1.0].
On 2012/02/23 11:50:41, henrika wrote:
> How do you know that this is a true statement?

From Apple document:
kAudioDevicePropertyVolumeScalar
A Float32 that represents the value of the volume control. The range is between
0.0 and 1.0 (inclusive). This property is implemented by an AudioControl object
that is a subclass of AudioVolumeControl.

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:385: DLOG(WARNING) <<
"AudioObjectGetPropertyData failed not get volume";
On 2012/02/23 11:50:41, henrika wrote:
> Clean up the comment. "failed not get volume" ...

Done.

http://codereview.chromium.org/9418042/diff/19003/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:514: // Get the stream latency.
On 2012/02/23 11:50:41, henrika wrote:
> I don't understand where this part comes from. Not related to this CL.

Sorry, merge error.

[tommi (sloooow) - chröme, 2012-02-24 12:07:33]

http://codereview.chromium.org/9418042/diff/26001/media/audio/audio_input_vol...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:63: #endif
we also have code for bsd although we don't maintain it ourselves.  Perhaps it's
worth adding an #else to these checks and either put a NOTREACHED() there or
even an #error?  Just so that the guys that do maintain that code will get a
heads up.

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_util.cc
File media/audio/linux/alsa_util.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_util...
media/audio/linux/alsa_util.cc:188: const char *elem_name =
wrapper->MixerSelemName(elem);
const char* elem_name

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
File media/audio/linux/alsa_wrapper.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
media/audio/linux/alsa_wrapper.cc:191: int AlsaWrapper::MixerLoad(snd_mixer_t
*mixer) {
please fix all  "foo *bar" and convert them to "foo* bar"

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
media/audio/linux/alsa_wrapper.cc:208: return snd_mixer_selem_get_name(elem);
just checking, but the returned string must not be deallocated, correct?

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
++i

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
is the condition correct?  Shouldn't it be 'i < number_of_channels_in_frame_'?

As is, if number_of_channels_in_frame_ is 0, then we will run this once and not
zero times.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
Please clean up all the unnecessary UInt32 (and possibly other types such as
Float32 if you don't need it) use in this file.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:289: return;
NOTREACHED?

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:291: Float32 volume_float32 =
static_cast<Float32> (volume);
no space between >(

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:300: UInt32 size =
sizeof(volume_float32);
nit: this variable isn't necessary. Just pass sizeof() directly to the function.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:308: DLOG(WARNING) << "Failed to
set volume to " << volume_float32;
use DLOG_IF instead

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:362: int success_on_channel = 0;
successful_channels?

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:363: for (UInt32 i = 1; i <=
number_of_channels_in_frame_; i++) {
++i
please fix throughout

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:383: return static_cast<double>
(volume_float32 / success_on_channel);
no space between >(
please fix throughout

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:386: DLOG(WARNING) << "Failed not
get volume";
-> "Failed to get volume"

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.h (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.h:96: UInt32
GetNumberOfChannelsFromStream();
It looks like you've taken the UInt32 type from a single struct inside this
function and spread it.  This should be int size_t.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.h:141: UInt32
number_of_channels_in_frame_;
is 32 bit necessary?  int or size_t.

[no longer working on chromium, 2012-02-24 15:05:55]

Changed the code based on the comments from Tommi.

http://codereview.chromium.org/9418042/diff/26001/media/audio/audio_input_vol...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:63: #endif
On 2012/02/24 12:07:34, tommi wrote:
> we also have code for bsd although we don't maintain it ourselves.  Perhaps
it's
> worth adding an #else to these checks and either put a NOTREACHED() there or
> even an #error?  Just so that the guys that do maintain that code will get a
> heads up.

I have || defined(OS_OPENBSD) above, but I agree that it is better to put an
#error on #else

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_util.cc
File media/audio/linux/alsa_util.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_util...
media/audio/linux/alsa_util.cc:188: const char *elem_name =
wrapper->MixerSelemName(elem);
On 2012/02/24 12:07:34, tommi wrote:
> const char* elem_name

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
File media/audio/linux/alsa_wrapper.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
media/audio/linux/alsa_wrapper.cc:191: int AlsaWrapper::MixerLoad(snd_mixer_t
*mixer) {
On 2012/02/24 12:07:34, tommi wrote:
> please fix all  "foo *bar" and convert them to "foo* bar"

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/linux/alsa_wrap...
media/audio/linux/alsa_wrapper.cc:208: return snd_mixer_selem_get_name(elem);
On 2012/02/24 12:07:34, tommi wrote:
> just checking, but the returned string must not be deallocated, correct?

Really good question. Since the returned value is const char*, and ALSA document
does not specify we need to free the memory after using it, so I suppose ALSA
has the ownership of the memory, I will double check it by using valgrind.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
On 2012/02/24 12:07:34, tommi wrote:
> is the condition correct?  Shouldn't it be 'i < number_of_channels_in_frame_'?
> 
> As is, if number_of_channels_in_frame_ is 0, then we will run this once and
not
> zero times.

I think it is correct. For example number_of_channels_in_frame_ = 2, we should
test channel 0, 1, 2. channel 0 means kAudioObjectPropertyElementMaster. In case
number_of_channels_in_frame_ = 0, but we should still try to find out if the
master channel has volume control or not.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
On 2012/02/24 12:07:34, tommi wrote:
> ++i

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:274: for (UInt32 i = 0; i <=
number_of_channels_in_frame_; i++) {
On 2012/02/24 12:07:34, tommi wrote:
> Please clean up all the unnecessary UInt32 (and possibly other types such as
> Float32 if you don't need it) use in this file.

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:289: return;
On 2012/02/24 12:07:34, tommi wrote:
> NOTREACHED?

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:291: Float32 volume_float32 =
static_cast<Float32> (volume);
On 2012/02/24 12:07:34, tommi wrote:
> no space between >(

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:300: UInt32 size =
sizeof(volume_float32);
On 2012/02/24 12:07:34, tommi wrote:
> nit: this variable isn't necessary. Just pass sizeof() directly to the
function.

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:308: DLOG(WARNING) << "Failed to
set volume to " << volume_float32;
On 2012/02/24 12:07:34, tommi wrote:
> use DLOG_IF instead

As discussed, keep it to get rid of the "unused variable" warning

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:362: int success_on_channel = 0;
On 2012/02/24 12:07:34, tommi wrote:
> successful_channels?

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:363: for (UInt32 i = 1; i <=
number_of_channels_in_frame_; i++) {
On 2012/02/24 12:07:34, tommi wrote:
> ++i
> please fix throughout

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:383: return static_cast<double>
(volume_float32 / success_on_channel);
On 2012/02/24 12:07:34, tommi wrote:
> no space between >(
> please fix throughout

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:386: DLOG(WARNING) << "Failed not
get volume";
On 2012/02/24 12:07:34, tommi wrote:
> -> "Failed to get volume"

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.h (right):

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.h:96: UInt32
GetNumberOfChannelsFromStream();
On 2012/02/24 12:07:34, tommi wrote:
> It looks like you've taken the UInt32 type from a single struct inside this
> function and spread it.  This should be int size_t.

Done.

http://codereview.chromium.org/9418042/diff/26001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.h:141: UInt32
number_of_channels_in_frame_;
On 2012/02/24 12:07:34, tommi wrote:
> is 32 bit necessary?  int or size_t.

Done.

[tommi (sloooow) - chröme, 2012-02-27 14:47:35]

lgtm.  please add a little note for the number_of_channels_ that explains that
e.g. a value of 2 means  1 (master) + 2 more channels.  I.e. that 3 channels
need to be checked.

[no longer working on chromium, 2012-02-27 16:34:41]

On 2012/02/27 14:47:35, tommi wrote:
> lgtm.  please add a little note for the number_of_channels_ that explains that
> e.g. a value of 2 means  1 (master) + 2 more channels.  I.e. that 3 channels
> need to be checked.

Done. The master channel is not counted in the number_of_channels_in_frame_,
which tells only left and right channels.

[commit-bot: I haz the power, 2012-02-27 23:26:50]

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

[commit-bot: I haz the power, 2012-02-28 05:10:00]

Try job failure for 9418042-37002 (retry) on win_rel for steps "unit_tests,
browser_tests, ui_tests".
It's a second try, previously, steps "unit_tests, browser_tests, ui_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-28 09:58:38]

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

[commit-bot: I haz the power, 2012-02-28 11:23:19]

Change committed as 123959

[henrika (OOO until Aug 14), 2012-02-28 16:02:04]

LGTM.

[nilesh, 2012-02-28 19:24:23]

http://codereview.chromium.org/9418042/diff/37002/media/audio/audio_input_vol...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9418042/diff/37002/media/audio/audio_input_vol...
media/audio/audio_input_volume_unittest.cc:64: #error Unsupported platform
I am going to exclude this test from the android build. Let me know if this is
not the right thing to do.