Adds Analog Gain Control (AGC) to the WebRTC client.

Adds Analog Gain Control (AGC) to the WebRTC client.

The AGC functionality is as follows. It aims at maintaining the same speech loudness level from the microphone.
This is done by both controlling the analog microphone gain and applying a digital gain.

The microphone gain on the sound card is slowly increased/decreased during speech only. By observing the microphone control slider you can see it move when you speak. If you scream, the slider moves downwards and then upwards again when you return to normal.
It is not uncommon that the slider hits the maximum. This means that the maximum analog gain is not large enough to give the desired loudness. Nevertheless, we can in general still attain the desired loudness.

If the microphone control slider is moved manually, the analog adaptation restarts and returns to roughly the same position as before the change if the circumstances are still the same.

When the input microphone signal causes saturation, the level is decreased dramatically and has to re-adapt towards the old level.

The adaptation is a slowly varying process and at the beginning of a call this is noticed by a slow increase in volume.

Smaller changes in microphone input level is leveled out by the built-in digital control. For larger differences we need to rely on the slow adaptation.

BUG=115265
TEST=content_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129400

[henrika (OOO until Aug 14), 2012-03-15 17:25:08]

Tommi and Shijing,

please take a look at this CL. It has grown quite a bit but there are no
advanced parts. The CL does currently only work on Windows (and almost on Mac).
The changes needed for Linux will more or less be cut-and-paste from the other
platforms. I was hoping for some feedback before I take the last step.

You can verify that it works on Windows using:

content_unittests --gtest_filter=WebRTC*FullDuplexAudioWithAGC*

[tommi (sloooow) - chröme, 2012-03-16 13:31:05]

http://codereview.chromium.org/9702019/diff/2002/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/9702019/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_renderer_host.cc:220: uint32
mem_size = AudioInputBuffer::kSizeOfStructMinusArr + packet_size;
nit: change kSizeOfStructMinusArr -> kSizeOfStructMinusArray.
If I wrote it as "Arr" on the white board, it was just me being lazy :)

I wonder if we can use more specific term than 'array'.  What about using the
word 'Packet' to be consistent with audio_params.GetPacketSize()?

http://codereview.chromium.org/9702019/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_renderer_host.cc:305:
NOTIMPLEMENTED();
Do we need to implement this at some point or never?
(when is it called?)

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
content/renderer/media/audio_input_device.cc:332:
reinterpret_cast<AudioInputBuffer*>(shared_memory_.memory());
indent

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
content/renderer/media/audio_input_device.cc:334: // TODO(henrika): uint32 size
= buffer->size;
ping

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
content/renderer/media/audio_input_device.cc:337: int16* memory =
reinterpret_cast<int16*>(buffer->audio);
nit: &buffer->audio[0]

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
File content/renderer/media/audio_input_device.h (right):

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/audio...
content/renderer/media/audio_input_device.h:149: // TODO(henrika): extend the
comments here...
ping (or just remove the todo)

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:805: double normalized_volume
= static_cast<double>(volume / kMaxVolumeLevel);
is the cast needed?

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:830: NOTREACHED();
A comment on why we never expect this to be called?

http://codereview.chromium.org/9702019/diff/2002/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/9702019/diff/2002/media/audio/audio_parameters...
media/audio/audio_parameters.h:12: #pragma pack(push, 1)
Can you add a note why we set the packing to 1?

Actually, since we intend to use this only for shared memory and not for network
buffers, I wonder if we can solve the problem of calculating the buffer size in
a different way.

I know that the problem is that with default 32bit packing, sizeof  the |audio|
member will be 4 (not 1) and that messes up the calculation for
kSizeOfStructMinusArr (which I think should be named kSizeOfStructMinusPacket
but maybe we don't need it...).

So, how about this approach instead:

struct MEDIA_EXPORT AudioInputBufferParameters {
  double volume;
  uint32 size;
};

struct MEDIA_EXPORT AudioInputBuffer {
  AudioInputBufferParameters params;
  int8 audio[1];
};

and then instead of kSizeOfStructMinusArr, we'll use
sizeof(AudioInputBufferParameters).  WDYT?

oh, and if we do it that way, can you add comments on why we separate the
parameters from the data based on the above.

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

http://codereview.chromium.org/9702019/diff/2002/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.cc:298: DLOG(INFO) <<
"SetVolume(volume=" << volume << ")";
ping

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

http://codereview.chromium.org/9702019/diff/2002/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:210: NULL);
indent

http://codereview.chromium.org/9702019/diff/2002/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:238: void
WASAPIAudioInputStream::SetAutomaticGainControl(bool enabled) {
seems like we have a lot of similar or exactly the same code here as for the
output stream. Can't we have that code in one place somehow?

http://codereview.chromium.org/9702019/diff/2002/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:417: if
(GetAutomaticGainControl()) {
this code looks the same too

[scherkus (not reviewing), 2012-03-20 13:49:41]

few drive by things

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/browser/rend...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/browser/rend...
content/browser/renderer_host/media/audio_input_renderer_host.cc:305:
NOTIMPLEMENTED();
On 2012/03/16 13:31:05, tommi wrote:
> Do we need to implement this at some point or never?
> (when is it called?)

AFAIK GetVolume() is completely unused

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/common/media...
File content/common/media/audio_messages.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/common/media...
content/common/media/audio_messages.h:108: bool /* atomatic_gain_control */)
typo

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.cc:51: pending_device_ready_(false) {
should you initialize agc_is_enabled_ here?

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/audio_input_device.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.h:149: // TODO(henrika): extend the
comments here...
On 2012/03/16 13:31:05, tommi wrote:
> ping (or just remove the todo)

ping again! :P

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:202: // The AGC returns a
non-zero microphone level if it has been decided
I'm confused -- isn't it the audio_transport_callback_ returning a non-zero mic
level as opposed to the AGC?

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_in...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_in...
media/audio/audio_input_controller.cc:187: DCHECK(volume >= 0 && volume <= 1.0);
nit: if you split these DCHECKs into DCHECK_GE() and DCHECK_LE() you'll get the
incorrect value reported on the console

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
media/audio/mac/audio_low_latency_input_mac.cc:476: DLOG(INFO) << "Volume
updated to: " << volume_;
logspam

[scherkus (not reviewing), 2012-03-21 09:21:21]

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

http://codereview.chromium.org/9702019/diff/2002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:699: // active.
when you say "most common usage" do you actually mean WebRTC will always set AGC
before recording starts or does it actually depend on some other factor?

I imagine we could simplify some things if we only permitted recording either
with/without AGC enabled (i.e., disallow changing on the fly)

http://codereview.chromium.org/9702019/diff/2002/media/audio/mac/audio_low_la...
File media/audio/mac/audio_low_latency_input_mac.h (right):

http://codereview.chromium.org/9702019/diff/2002/media/audio/mac/audio_low_la...
media/audio/mac/audio_low_latency_input_mac.h:158: base::subtle::Atomic32
agc_is_enabled_;
please avoid using base::subtle -- it's intended to be used to build
synchronization primitives (i.e., Lock)

there's two ways around this:
  1) disallow changing AGC on the fly (i.e., must be set prior to recording)
  2) use a lock -- there will be no contention for the lock seeing as this value
is set once

if I had to pick I'd go with (1) assuming that WebRTC knows whether it wants AGC
or not prior to recording and does not need to change it on the fly

[henrika (OOO until Aug 14), 2012-03-21 10:16:03]

Great input; thanks. I'll come back with a new version soon taking the following
things into account:

- avoid base::subtle
- add restriction on when to enable/disable AGC
- try to factor out platform independent AGC parts

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/browser/rend...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/browser/rend...
content/browser/renderer_host/media/audio_input_renderer_host.cc:220: uint32
mem_size = AudioInputBuffer::kSizeOfStructMinusArr + packet_size;
On 2012/03/16 13:31:05, tommi wrote:
> nit: change kSizeOfStructMinusArr -> kSizeOfStructMinusArray.
> If I wrote it as "Arr" on the white board, it was just me being lazy :)
> 
> I wonder if we can use more specific term than 'array'.  What about using the
> word 'Packet' to be consistent with audio_params.GetPacketSize()?

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/browser/rend...
content/browser/renderer_host/media/audio_input_renderer_host.cc:305:
NOTIMPLEMENTED();
Yeah, you are correct. I might as well drop it actually. Better to add it
if/when we need it.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/common/media...
File content/common/media/audio_messages.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/common/media...
content/common/media/audio_messages.h:108: bool /* atomatic_gain_control */)
Thanks.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.cc:51: pending_device_ready_(false) {
OOps. Thanks.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.cc:332:
reinterpret_cast<AudioInputBuffer*>(shared_memory_.memory());
On 2012/03/16 13:31:05, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.cc:334: // TODO(henrika): uint32 size
= buffer->size;
Added DCHECK which uses the size variable.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.cc:337: int16* memory =
reinterpret_cast<int16*>(buffer->audio);
On 2012/03/16 13:31:05, tommi wrote:
> nit: &buffer->audio[0]

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/audio_input_device.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/audio_input_device.h:149: // TODO(henrika): extend the
comments here...
Much better now ;-)

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:202: // The AGC returns a
non-zero microphone level if it has been decided
The last two parameters in RecordedDataIsAvailable() are:
const WebRtc_UWord32 currentMicLevel, and 
WebRtc_UWord32& newMicLevel.

We use currentMicLevel to provide input from the media layer to the AGC in
WebRTC. The AGC then determines if it is time for a volume update or not
(depends on speech or no speech, if the user has moved the volume slider
manually, level of speech etc. If a new volume shall be set, the output
reference paramterer newMicLevel is set to a non-zero value to indicate "it is
now time for a change".

Let me know if something is still not clear.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:699: // active.
I discussed it with Justin and it was OK to add such a restriction. I will
simplify and also remove usage of base::subtle in the media layer.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:805: double normalized_volume
= static_cast<double>(volume / kMaxVolumeLevel);
Good question. I don't think is required for fundamental types but implicit
casting will most likely be enough. Do you find it disturbing? I think it is OK
as is.

https://chromiumcodereview.appspot.com/9702019/diff/2002/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:830: NOTREACHED();
Good question. It is a method that have caused us many issues in the past since
very few platforms support it, and those who do does it in slightly different
ways. Basically, it is only useful for unit tests where one wants to sweep the
volume and know for sure that a change does in fact have an effect.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_in...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_in...
media/audio/audio_input_controller.cc:187: DCHECK(volume >= 0 && volume <= 1.0);
Nice. Thanks.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/audio_pa...
media/audio/audio_parameters.h:12: #pragma pack(push, 1)
On 2012/03/16 13:31:05, tommi wrote:
> Can you add a note why we set the packing to 1?
> 
> Actually, since we intend to use this only for shared memory and not for
network
> buffers, I wonder if we can solve the problem of calculating the buffer size
in
> a different way.
> 
> I know that the problem is that with default 32bit packing, sizeof  the
|audio|
> member will be 4 (not 1) and that messes up the calculation for
> kSizeOfStructMinusArr (which I think should be named kSizeOfStructMinusPacket
> but maybe we don't need it...).
> 
> So, how about this approach instead:
> 
> struct MEDIA_EXPORT AudioInputBufferParameters {
>   double volume;
>   uint32 size;
> };
> 
> struct MEDIA_EXPORT AudioInputBuffer {
>   AudioInputBufferParameters params;
>   int8 audio[1];
> };
> 
> and then instead of kSizeOfStructMinusArr, we'll use
> sizeof(AudioInputBufferParameters).  WDYT?
> 
> oh, and if we do it that way, can you add comments on why we separate the
> parameters from the data based on the above.

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
media/audio/mac/audio_low_latency_input_mac.cc:298: DLOG(INFO) <<
"SetVolume(volume=" << volume << ")";
On 2012/03/16 13:31:05, tommi wrote:
> ping

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
media/audio/mac/audio_low_latency_input_mac.cc:476: DLOG(INFO) << "Volume
updated to: " << volume_;
On 2012/03/20 13:49:41, scherkus wrote:
> logspam

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
File media/audio/mac/audio_low_latency_input_mac.h (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/mac/audi...
media/audio/mac/audio_low_latency_input_mac.h:158: base::subtle::Atomic32
agc_is_enabled_;
Good idea. I'll come back with a new version and will go for #1 as you propose.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/win/audi...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/win/audi...
media/audio/win/audio_low_latency_input_win.cc:210: NULL);
On 2012/03/16 13:31:05, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9702019/diff/2002/media/audio/win/audi...
media/audio/win/audio_low_latency_input_win.cc:238: void
WASAPIAudioInputStream::SetAutomaticGainControl(bool enabled) {
I feel the same way. Let me try to come up with something.

[henrika (OOO until Aug 14), 2012-03-22 12:51:59]

Hi guys,

added a new version where I broke out platform-independent AGC-related parts
from the AudioInputStream.

Hope you like it.

[henrika (OOO until Aug 14), 2012-03-22 13:34:09]

Seems this CL needs some serious rebasing first as well. Please hold off your
comments until that's done.

[henrika (OOO until Aug 14), 2012-03-26 11:10:41]

Should be ready for a final? round now.

Added support for Linux and Mac as well. Verified the AGC performance on both
desktop and laptop machines.

[tommi (sloooow) - chröme, 2012-03-26 15:26:38]

lgtm with a few comments.

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

http://codereview.chromium.org/9702019/diff/17032/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:52: agc_is_enabled_(false) {
indent

http://codereview.chromium.org/9702019/diff/17032/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:102: return false;
NOTREACHED?

http://codereview.chromium.org/9702019/diff/17032/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:331: 
remove empty line

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

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:51: agc_is_enabled_(false) {
indent is off.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:173: // The state of the AGC can
not be modified while capturing is active.
Are the reasons for this still valid?

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:316: 
remove this empty line

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:327: int16* memory =
reinterpret_cast<int16*>(buffer->audio);
nit: &buffer->audio[0]

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:94: //  - Regarding the AGC:
It aims at maintaining the same speech loudness level
s/It aims at maintaining/It aims to maintain

It's not very clear what "the same" refers to.  I'm assuming it means 'even out'
or normalize the volume, but there might be a clearer way to put it.

You could also just point to http://en.wikipedia.org/wiki/Automatic_gain_control
and skip to the implementation specific things.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:96: //    microphone gain and
applying a digital gain. The microphone gain on the
s/a digital gain/digital gain

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:104: //    the analog
adaptation restarts and returns to roughly the same position
s/analog adaptation/gain adaptation.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:108: //    is a slowly varying
process and at the beginning of a call this is
s/beginning of a call/beginning of capture
(there are no assumptions about a 'call' in the code afaics)

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
File media/audio/audio_input_stream_impl.h (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
media/audio/audio_input_stream_impl.h:36: 
one empty line should be enough

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/audio_low_l...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win.cc:334: double volume =
static_cast<double>(GetVolume());
is the cast necessary?

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/wavein_inpu...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/wavein_inpu...
media/audio/win/wavein_input_win.cc:274: // TODO(henrika): comment on volume =
0.0.
ping

[henrika (OOO until Aug 14), 2012-03-26 15:52:30]

Thanks Tommi, will fix the final issues tomorrow.

Andrew: it would be great if you had time to give some comments as well.

Antoine: I think I need your OK as well for one file in content/renderer/pepper.

[piman, 2012-03-26 16:44:38]

LGTM on the pepper part.
Would AGC be a good feature to expose to Pepper too?

[henrika (OOO until Aug 14), 2012-03-26 19:08:51]

The AGC is currently only available through the WebRTC client since the core
part of it is rather deeply integrated into third_party/webrtc which is exposed
to Chrome through content/renderer/media/webrtc_audio_device_impl. What this CL
does is to add the final hooks that are required to close the loop and add the
analog parts of the AGC. The digital scaling part was already in place. 

It means that it is not trivial to add the same AGC to other clients as well
unless they use the WebRTC client. 

I have forwarded your question to jtlinden@ who is PM for audio in Chrome to see
what his view is. I'll let you know when he has replied. If it is essential for
Pepper to have a full AGC, I might be able to provide some ideas on how it could
be implemented.

[scherkus (not reviewing), 2012-03-26 22:41:04]

LGTM w/ nits

http://codereview.chromium.org/9702019/diff/25002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

http://codereview.chromium.org/9702019/diff/25002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:24: double
volume) {
nit: indent to align w/ other params

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

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:174: DCHECK_EQ(0, stream_id_);
want to move that comment into a DCHECK() message?

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

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:713:
audio_input_device_->SetAutomaticGainControl(enable);
nit: instead of a setter you may consider passing in this value to Start() --
that way we don't have an API that puts restrictions on the ordering of method
calls

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
File media/audio/audio_input_stream_impl.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
media/audio/audio_input_stream_impl.cc:16:
AudioInputStreamImpl::~AudioInputStreamImpl() {
nit: {}

[henrika (OOO until Aug 14), 2012-03-27 09:20:38]

Great input. Modified accordingly.

http://codereview.chromium.org/9702019/diff/25002/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

http://codereview.chromium.org/9702019/diff/25002/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:24: double
volume) {
On 2012/03/26 22:41:04, scherkus wrote:
> nit: indent to align w/ other params

Done.

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

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:51: agc_is_enabled_(false) {
On 2012/03/26 15:26:40, tommi wrote:
> indent is off.

Done.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:173: // The state of the AGC can
not be modified while capturing is active.
Yes, I have removed locks in on the media side under the assumption that this
condition holds.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:174: DCHECK_EQ(0, stream_id_);
On 2012/03/26 22:41:04, scherkus wrote:
> want to move that comment into a DCHECK() message?

Done.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:316: 
On 2012/03/26 15:26:40, tommi wrote:
> remove this empty line

Done.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:327: int16* memory =
reinterpret_cast<int16*>(buffer->audio);
On 2012/03/26 15:26:40, tommi wrote:
> nit: &buffer->audio[0]

Done.

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

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.cc:713:
audio_input_device_->SetAutomaticGainControl(enable);
Thanks for the proposal. I'd like to keep the current design if that is OK. My
opinion is that calls like: Start(true/false) can look odd.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:94: //  - Regarding the AGC:
It aims at maintaining the same speech loudness level
Modified to "It aims to maintain a constant speech loudness level from the
microphone." and added a reference to Wikipedia.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:96: //    microphone gain and
applying a digital gain. The microphone gain on the
On 2012/03/26 15:26:40, tommi wrote:
> s/a digital gain/digital gain

Done.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:104: //    the analog
adaptation restarts and returns to roughly the same position
Actually, it is only the analog part that restarts; not the digital. Will change
as you propose.

http://codereview.chromium.org/9702019/diff/25002/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_impl.h:108: //    is a slowly varying
process and at the beginning of a call this is
On 2012/03/26 15:26:40, tommi wrote:
> s/beginning of a call/beginning of capture
> (there are no assumptions about a 'call' in the code afaics)

Done.

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
File media/audio/audio_input_stream_impl.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
media/audio/audio_input_stream_impl.cc:16:
AudioInputStreamImpl::~AudioInputStreamImpl() {
On 2012/03/26 22:41:04, scherkus wrote:
> nit: {}

Done.

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
File media/audio/audio_input_stream_impl.h (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/audio_input_str...
media/audio/audio_input_stream_impl.h:36: 
On 2012/03/26 15:26:40, tommi wrote:
> one empty line should be enough

Done.

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/audio_low_l...
File media/audio/win/audio_low_latency_input_win.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win.cc:334: double volume =
static_cast<double>(GetVolume());
It should not be needed in float->double. Removed.

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/wavein_inpu...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/9702019/diff/25002/media/audio/win/wavein_inpu...
media/audio/win/wavein_input_win.cc:274: // TODO(henrika): comment on volume =
0.0.
On 2012/03/26 15:26:40, tommi wrote:
> ping

Done.

[commit-bot: I haz the power, 2012-03-27 10:56:22]

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

[commit-bot: I haz the power, 2012-03-27 13:38:58]

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