Improved AGC update scheme for the audio backend in Chrome

media/audio/agc_audio_stream.h

Index: media/audio/agc_audio_stream.h
diff --git a/media/audio/agc_audio_stream.h b/media/audio/agc_audio_stream.h
new file mode 100644
index 0000000000000000000000000000000000000000..7a719d8712277a5306a3c42fcb3610b1f3dcaf42
--- /dev/null
+++ b/media/audio/agc_audio_stream.h
@@ -0,0 +1,144 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_AUDIO_AGC_AUDIO_STREAM_H_
+#define MEDIA_AUDIO_AGC_AUDIO_STREAM_H_
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#include "base/timer.h"
+#include "media/audio/audio_io.h"
+
+namespace media {
+
+// TODO(henrika): add info about threading
+
+// The template based AgcAudioStream implements platform-independent parts
+// of the AudioInput/OutputStream interfaces. Each platform dependent
+// implementation should derive from this class.

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

maybe add a note about what interfaces can be passed as AudioInterface?

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

Done.

+template <typename AudioInterface>
+class MEDIA_EXPORT AgcAudioStream : public AudioInterface {
+ public:
+  // Time between two successive timer events.
+  enum { kIntervalBetweenVolumeUpdatesMs = 1000 };
+
+  AgcAudioStream() : agc_is_enabled_(false), max_volume_(0.0),
+    normalized_volume_(0.0) {

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

fix indent and move : to this line

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

Done.

+    DVLOG(1) << __FUNCTION__;
+  }
+
+  virtual ~AgcAudioStream() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DVLOG(1) << __FUNCTION__;
+  }
+
+ protected:
+  // Stores a new microphone volume level by checking the audio input device.
+  // Called on the audio manager thread.
+  void UpdateAgcVolume() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+
+    // We take new volume samples once every second when the AGC is enabled.
+    // To ensure that a new setting has an immediate effect, the new volume
+    // setting is cached here. It will ensure that the next OnData() callback
+    // will contain a new valid volume level. If this approach was not taken,
+    // we could report invalid volume levels to the client for a time period
+    // of up to one second.
+    QueryAndStoreNewMicrophoneVolume();
+  }
+
+  //  the latest stored volume level if AGC is enabled.
+  // Called at each capture callback on a real-time capture thread (platform
+  // dependent).
+  void GetAgcVolume(double* normalized_volume) {
+    base::AutoLock lock(lock_);
+    *normalized_volume = normalized_volume_;
+  }
+
+ private:
+  // Sets the automatic gain control (AGC) to on or off. When AGC is enabled,
+  // the microphone volume is queried periodically and the volume level can
+  // be read in each AudioInputCallback::OnData() callback and fed to the
+  // render-side AGC.
+  virtual void SetAutomaticGainControl(bool enabled) {

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

OVERRIDE?

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

Nice catch!

+    DVLOG(1) << "SetAutomaticGainControl(enabled=" << enabled << ")";
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (enabled) {
+      timer_.Start(FROM_HERE,

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

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?

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

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.

+        base::TimeDelta::FromMilliseconds(kIntervalBetweenVolumeUpdatesMs),

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

indent

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

Done.

+        this, &AgcAudioStream::OnTimer);
+    } else {

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

combine this else with the nested if?

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

Done.

+      if (timer_.IsRunning())
+        timer_.Stop();
+    }
+    agc_is_enabled_ = enabled;

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

do we need to handle the case where agc_is_enabled_ == enabled at the top of the
function?

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

Sorry; not sure if I understand that comment. What am I missing?

+  }
+
+  // Gets the current automatic gain control state.
+  virtual bool GetAutomaticGainControl() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    return agc_is_enabled_;
+  }
+
+  // Takes a new microphone volume sample and stores it in |normalized_volume_|.
+  // Range is normalized to [0.0,1.0] or [0.0, 1.5] on Linux.
+  void QueryAndStoreNewMicrophoneVolume() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(timer_.IsRunning());
+
+    // Cach the maximum volume if this is the first time we ask for it.
+    if (max_volume_ == 0.0)
+      max_volume_ = GetMaxVolume();
+
+    // Retrieve the current volume level by asking the audio hardware.
+    // Range is normalized to [0.0,1.0] or [0.0, 1.5] on Linux.
+    if (max_volume_ != 0.0) {
+      double normalized_volume = GetVolume() / max_volume_;
+      {
+        base::AutoLock auto_lock(lock_);
+        normalized_volume_ = normalized_volume;
+      }
+    }
+  }
+
+  // This method is called periodically when AGC is enabled and always on the
+  // audio manager thread. We use it to read the current microphone level and
+  // to store it so it can be read by the main capture thread. By using this
+  // approach, we can avoid accessing audio hardware from a real-time audio
+  // thread and it leads to a more stable capture performance.
+  void OnTimer() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    QueryAndStoreNewMicrophoneVolume();
+  }
+
+  // Ensures that this class is created and destroyed on the same thread.
+  base::ThreadChecker thread_checker_;
+
+  // Repeating timer which cancels itself when it goes out of scope.
+  // Used to check the microphone volume periodically.
+  base::RepeatingTimer<AgcAudioStream> timer_;

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

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 :)

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

Really good comment. I was a bit surprised that it passed first as well. Changed
to your style since it feels more kocher.

+
+  // True when automatic gain control is enabled, false otherwise.
+  bool agc_is_enabled_;
+
+  // Stores the maximum volume which is used for normalization to a volume
+  // range of [0.0, 1.0].
+  double max_volume_;
+
+  // Contains last result of internal call to GetVolume(). We save resources
+  // but not querying the capture volume for each callback. Guarded by |lock_|.

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

s/but/by

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

Done.

+  // The range is normalized to [0.0, 1.0].
+  double normalized_volume_;
+
+  // Protects |normalized_volume_| .
+  base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(AgcAudioStream<AudioInterface>);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_AUDIO_AGC_AUDIO_STREAM_H_