Improved AGC update scheme for the audio backend in Chrome

Improved AGC update scheme for the audio backend in Chrome.

This CL serves two purposes:

1) Improve the existing "mic-volume-checking"-scheme by ensuring that we no longer calls native audio functions from the core capture thread.
2) Prepare for adding AGC to the live-audio backend.

TBR=tommi@chromium.org
BUG=none
TEST=WebRTC loopback tests in Chrome where I monitor the microphone volume while speaking into the mic.

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

[henrika (OOO until Aug 14), 2013-05-24 13:52:33]

Tommi,

given our off-line discussions, could you take a look at this one?

Note that I've added a timer which runs on the AM thread ensure that the capture
thread only reads a cached volume value and never touches any APIs (which can
lead to audio distortions or even dead-locks).

[henrika (OOO until Aug 14), 2013-05-24 13:55:03]

I will update more platforms when you are OK with the main approach. Once landed
I can start adding support for the live-audio part as well but that can be done
in a separate CL.

[tommi (sloooow) - chröme, 2013-05-24 18:21:11]

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
File media/audio/agc_audio_stream.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:21: // implementation should derive from this
class.
maybe add a note about what interfaces can be passed as AudioInterface?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:29: normalized_volume_(0.0) {
fix indent and move : to this line

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:66: virtual void SetAutomaticGainControl(bool
enabled) {
OVERRIDE?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:70: timer_.Start(FROM_HERE,
does timer_.Start() have some requirements on the type of the current message
loop?
if so, would it make sense to dcheck on that type in the constructor of this
class?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:71:
base::TimeDelta::FromMilliseconds(kIntervalBetweenVolumeUpdatesMs),
indent

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:73: } else {
combine this else with the nested if?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:77: agc_is_enabled_ = enabled;
do we need to handle the case where agc_is_enabled_ == enabled at the top of the
function?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:122: base::RepeatingTimer<AgcAudioStream> timer_;
I would have expected this to have to be
base::RepeatingTimer<AgcAudioStream<AudioInterface> > timer_;  but if the
compiler isn't complaining, then I guess I shouldn't :)

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:132: // but not querying the capture volume for
each callback. Guarded by |lock_|.
s/but/by

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h#ne...
media/audio/audio_io.h:113: virtual void SetAutomaticGainControl(bool enabled)
{}
pure virtual?

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h#ne...
media/audio/audio_io.h:117: virtual bool GetAutomaticGainControl() { return
false; }
same here

https://codereview.chromium.org/15563004/diff/11001/media/audio/linux/alsa_in...
File media/audio/linux/alsa_input.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/linux/alsa_in...
media/audio/linux/alsa_input.h:16: #include "media/audio/agc_audio_stream.h"
is this #include needed?

https://codereview.chromium.org/15563004/diff/11001/media/audio/mac/audio_low...
File media/audio/mac/audio_low_latency_input_mac.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/mac/audio_low...
media/audio/mac/audio_low_latency_input_mac.h:45: #include
"media/audio/agc_audio_stream.h"
#include needed?

https://codereview.chromium.org/15563004/diff/11001/media/audio/pulse/pulse_i...
File media/audio/pulse/pulse_input.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/pulse/pulse_i...
media/audio/pulse/pulse_input.h:11: #include "media/audio/agc_audio_stream.h"
#include needed?

[henrika (OOO until Aug 14), 2013-05-27 12:17:43]

Thanks Tommi,

making changes as proposed. My plan was to start with refactoring input side
only first in this CL and add methods for output side (and live-audio) in
upcoming CL.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
File media/audio/agc_audio_stream.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:21: // implementation should derive from this
class.
On 2013/05/24 18:21:11, tommi wrote:
> maybe add a note about what interfaces can be passed as AudioInterface?

Done.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:29: normalized_volume_(0.0) {
On 2013/05/24 18:21:11, tommi wrote:
> fix indent and move : to this line

Done.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:66: virtual void SetAutomaticGainControl(bool
enabled) {
Nice catch!

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:70: timer_.Start(FROM_HERE,
Comments says:

// OneShotTimer and RepeatingTimer both cancel the timer when they go out of
// scope, which makes it easy to ensure that you do not get called when your
// object has gone out of scope.  Just instantiate a OneShotTimer or
// RepeatingTimer as a member variable of the class for which you wish to
// receive timer events.

and

// NOTE: These APIs are not thread safe. Always call from the same thread.

and I do ensure that we start it from the same thread as the object is created
on.
Not sure if I can do more.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:71:
base::TimeDelta::FromMilliseconds(kIntervalBetweenVolumeUpdatesMs),
On 2013/05/24 18:21:11, tommi wrote:
> indent

Done.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:73: } else {
On 2013/05/24 18:21:11, tommi wrote:
> combine this else with the nested if?

Done.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:77: agc_is_enabled_ = enabled;
Sorry; not sure if I understand that comment. What am I missing?

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:122: base::RepeatingTimer<AgcAudioStream> timer_;
Really good comment. I was a bit surprised that it passed first as well. Changed
to your style since it feels more kocher.

https://codereview.chromium.org/15563004/diff/11001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:132: // but not querying the capture volume for
each callback. Guarded by |lock_|.
On 2013/05/24 18:21:11, tommi wrote:
> s/but/by

Done.

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h#ne...
media/audio/audio_io.h:113: virtual void SetAutomaticGainControl(bool enabled)
{}
Actually the idea was to add these in a follow-up CL when refactoring was done
in the input part. Hence, I remove these for now. Hope that is OK.

https://codereview.chromium.org/15563004/diff/11001/media/audio/audio_io.h#ne...
media/audio/audio_io.h:117: virtual bool GetAutomaticGainControl() { return
false; }
see above

https://codereview.chromium.org/15563004/diff/11001/media/audio/mac/audio_low...
File media/audio/mac/audio_low_latency_input_mac.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/mac/audio_low...
media/audio/mac/audio_low_latency_input_mac.h:45: #include
"media/audio/agc_audio_stream.h"
Now it is ;-)

https://codereview.chromium.org/15563004/diff/11001/media/audio/pulse/pulse_i...
File media/audio/pulse/pulse_input.h (right):

https://codereview.chromium.org/15563004/diff/11001/media/audio/pulse/pulse_i...
media/audio/pulse/pulse_input.h:11: #include "media/audio/agc_audio_stream.h"
Now it is ;-)

[henrika (OOO until Aug 14), 2013-05-27 12:23:04]

tommi, PTAL

[tommi (sloooow) - chröme, 2013-05-27 13:54:36]

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
File media/audio/agc_audio_stream.h (right):

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:17: // TODO(henrika): add info about threading
Will you do this now?

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:36: enum { kIntervalBetweenVolumeUpdatesMs = 1000
};
I don't think Chrome in general declares constants this way. Instead declare it
as a static const int.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:83: } else if (timer_.IsRunning())
{}
(if one side of an else has a brace, the other should as well)

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:85: agc_is_enabled_ = enabled;
What I meant here is that what if these two values are already equal?  Aren't we
then going through an unnecessary amount of trouble to set the value?

I think this could be covered by a check at the top of the function:
if (agc_is_enabled_ == enabled)
  return;

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:108: {
nit: there's no need for this scope since the next two lines contain the only
objects in the current scope and they are also the two last lines in the scope.

[henrika (OOO until Aug 14), 2013-05-27 15:37:36]

Fixed compiler issues on all platforms (Windows is a bit special) and improved
class description.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
File media/audio/agc_audio_stream.h (right):

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:17: // TODO(henrika): add info about threading
Done. Sorry.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:36: enum { kIntervalBetweenVolumeUpdatesMs = 1000
};
On 2013/05/27 13:54:36, tommi wrote:
> I don't think Chrome in general declares constants this way. Instead declare
it
> as a static const int.

Done.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:83: } else if (timer_.IsRunning())
On 2013/05/27 13:54:36, tommi wrote:
> {}
> (if one side of an else has a brace, the other should as well)

Done.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:85: agc_is_enabled_ = enabled;
On 2013/05/27 13:54:36, tommi wrote:
> What I meant here is that what if these two values are already equal?  Aren't
we
> then going through an unnecessary amount of trouble to set the value?
> 
> I think this could be covered by a check at the top of the function:
> if (agc_is_enabled_ == enabled)
>   return;

Done.

https://codereview.chromium.org/15563004/diff/19001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:108: {
On 2013/05/27 13:54:36, tommi wrote:
> nit: there's no need for this scope since the next two lines contain the only
> objects in the current scope and they are also the two last lines in the
scope.

Done.

[henrika (OOO until Aug 14), 2013-05-27 15:39:07]

tommi, PTAL thanks.

https://codereview.chromium.org/15563004/diff/31001/media/audio/agc_audio_str...
File media/audio/agc_audio_stream.h (right):

https://codereview.chromium.org/15563004/diff/31001/media/audio/agc_audio_str...
media/audio/agc_audio_stream.h:122: max_volume_ =
static_cast<AudioInterface*>(this)->GetMaxVolume();
Should now work on all platforms. Only Windows compiler said OK without "the
this trick".

[henrika (OOO until Aug 14), 2013-05-27 15:46:14]

Tommi, FYI, I am also thinking about adding one additional improvement and would
like your feedback on the idea. Today, SetAgc(true) is called when the stream is
created and Start() is usually called directly after => we don't do volume
checks that are not used (between AGC starts and we have started actual
capturing).

My plan was to add an API called e.g. CaptureIsStarted(bool) which the
implementation must call in its start method. I would then avoid asking for the
mic volume unless capturing has actually started. I could also combine it with
the existing GetAutomaticGainControl() and let the timer method wait for the
first call of this method and only then asking for the mic volume (or even start
the timer).

Sound OK?

[tommi (sloooow) - chröme, 2013-05-28 07:32:38]

lgtm.

On 2013/05/27 15:46:14, henrika wrote:
> Tommi, FYI, I am also thinking about adding one additional improvement and
would
> like your feedback on the idea. Today, SetAgc(true) is called when the stream
is
> created and Start() is usually called directly after => we don't do volume
> checks that are not used (between AGC starts and we have started actual
> capturing).
> 
> My plan was to add an API called e.g. CaptureIsStarted(bool) which the
> implementation must call in its start method. I would then avoid asking for
the
> mic volume unless capturing has actually started. I could also combine it with
> the existing GetAutomaticGainControl() and let the timer method wait for the
> first call of this method and only then asking for the mic volume (or even
start
> the timer).
> 
> Sound OK?

Sounds good to me.  If it's a method that's only called once, then perhaps
calling it from the implementation's Initialize() method would be better? (since
Start() can be called multiple times)
The Agc class could also have its own Initialize method instead of
CaptureIsStarted and that way it would be clear that the class must be
initialized before use and set the agc flag there.

[henrika (OOO until Aug 14), 2013-05-28 09:23:04]

Dale, seems like I need you OK for media.gyp. Hope it is OK if I use TBR. Feel
free to comment and I can make fixes in a separate CL.

Tommi: improved the description and added two new protected members, StartAgc()
and StopAgc(). The actual audio-stream interface method
SetAutomaticGainControl() now only sets a state and prepates for AGC
functionality (called in an early stage when the stream is created).

The implementation can now use this state and start/stop the timer which touches
the mic in its Start/Stop method.

Feels like a suitable compromise. Thanks for you input.

[commit-bot: I haz the power, 2013-05-28 09:31:05]

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

[henrika (OOO until Aug 14), 2013-05-28 09:38:41]

Chris, FYI.

Started out refactoring the input part to prepare for adding AGC for live-audio
as well. This CL also removes the existing "bad design" where we allow the core
audio thread to touch the mic volume control. This is now done from a timer
running on the audio manager thread.

[tommi (sloooow) - chröme, 2013-05-28 11:01:13]

Looks like you added code after the review and then committed.  That's generally
not the way to do things since that code hasn't been reviewed, but I took a look
and don't see any issues. lgtm.

[henrika (OOO until Aug 14), 2013-05-28 11:08:59]

Sorry, bad style from my side. Mainly added comments and the described (trivial)
idea. Will not be so trigger-happy next time.

Dale, if you have any additional feedback I will deal with that in a separate
CL.

[commit-bot: I haz the power, 2013-05-28 11:39:45]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[henrika (OOO until Aug 14), 2013-05-28 11:53:11]

Committed patchset #8 manually as r202538 (presubmit successful).

[DaleCurtis, 2013-05-28 16:09:52]

lgtm