Removed the dependency in GainControlImpl on the ProcessingComponent class

webrtc/modules/audio_processing/gain_control_impl.cc

Index: webrtc/modules/audio_processing/gain_control_impl.cc
diff --git a/webrtc/modules/audio_processing/gain_control_impl.cc b/webrtc/modules/audio_processing/gain_control_impl.cc
index 04a6c7ba29c264d4d8f1ccd6f7c26d29ad3e25c9..b115315a1d09cdf88371af0eb92091cbbbe2ee95 100644
--- a/webrtc/modules/audio_processing/gain_control_impl.cc
+++ b/webrtc/modules/audio_processing/gain_control_impl.cc
@@ -12,6 +12,7 @@
 
 #include <assert.h>

[the sun, 2016/03/09 09:42:11]

Remove assert() while you're at it

[peah-webrtc, 2016/03/09 12:23:39]

Done.

 
+#include "webrtc/base/constructormagic.h"

[the sun, 2016/03/09 09:42:11]

Won't be needed.

[peah-webrtc, 2016/03/09 12:23:39]

Done.

 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/agc/legacy/gain_control.h"
 
@@ -42,11 +43,38 @@ static const size_t kMaxNumFramesToBuffer = 100;
 
 }  // namespace
 
+class GainControlImpl::GainController {
+ public:
+  explicit GainController() {
+    state_ = WebRtcAgc_Create();
+    RTC_CHECK(state_);
+  }
+
+  ~GainController() {
+    RTC_DCHECK(state_);

[the sun, 2016/03/09 09:42:10]

remove this DCHECK or add one in each of the other methods.

[peah-webrtc, 2016/03/09 12:23:39]

Done.

+    WebRtcAgc_Free(state_);
+  }
+
+  Handle* state() { return state_; }
+  void Initialize(int minimum_capture_level,
+                  int maximum_capture_level,
+                  Mode mode,
+                  int sample_rate_hz) {
+    const int error =
+        WebRtcAgc_Init(state_, minimum_capture_level, maximum_capture_level,
+                       MapSetting(mode), sample_rate_hz);
+    RTC_DCHECK_EQ(0, error);
+  }
+
+ private:
+  Handle* state_;
+  RTC_DISALLOW_COPY_AND_ASSIGN(GainController);
+};
+
 GainControlImpl::GainControlImpl(const AudioProcessing* apm,
                                  rtc::CriticalSection* crit_render,
                                  rtc::CriticalSection* crit_capture)
-    : ProcessingComponent(),
-      apm_(apm),
+    : apm_(apm),
       crit_render_(crit_render),
       crit_capture_(crit_capture),
       mode_(kAdaptiveAnalog),
@@ -68,20 +96,20 @@ GainControlImpl::~GainControlImpl() {}
 
 int GainControlImpl::ProcessRenderAudio(AudioBuffer* audio) {
   rtc::CritScope cs(crit_render_);
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
-  assert(audio->num_frames_per_band() <= 160);
+  RTC_DCHECK_GE(160u, audio->num_frames_per_band());
 
   render_queue_buffer_.resize(0);
-  for (size_t i = 0; i < num_handles(); i++) {
-    Handle* my_handle = static_cast<Handle*>(handle(i));
+  for (size_t i = 0; i < num_handles_required(); i++) {
+    Handle* my_handle = gain_controllers_[i]->state();
     int err =
         WebRtcAgc_GetAddFarendError(my_handle, audio->num_frames_per_band());
 
     if (err != AudioProcessing::kNoError)
-      return GetHandleError(my_handle);
+      return AudioProcessing::kUnspecifiedError;
 
     // Buffer the samples in the render queue.
     render_queue_buffer_.insert(
@@ -106,16 +134,16 @@ int GainControlImpl::ProcessRenderAudio(AudioBuffer* audio) {
 void GainControlImpl::ReadQueuedRenderData() {
   rtc::CritScope cs(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return;
   }
 
   while (render_signal_queue_->Remove(&capture_queue_buffer_)) {
     size_t buffer_index = 0;
     const size_t num_frames_per_band =
-        capture_queue_buffer_.size() / num_handles();
-    for (size_t i = 0; i < num_handles(); i++) {
-      Handle* my_handle = static_cast<Handle*>(handle(i));
+        capture_queue_buffer_.size() / num_handles_required();
+    for (size_t i = 0; i < num_handles_required(); i++) {

[the sun, 2016/03/09 09:42:10]

for (auto& gain_controller : gain_controllers_) {

[peah-webrtc, 2016/03/09 12:23:39]

This does not work, as the number of gain controllers may be larger than
num_handles_required.

This behavior is weird, but according to how it was before. Lets refactor the
whole subcomponent-channel behavior once this is done to remedy that.

I'll keep it as it is for now. Wdyt?

[the sun, 2016/03/09 13:01:53]

That's ok, but it's quite a simple fix to resize the gain_controllers_ array
even when the number of required "handles" decrease.

[peah-webrtc, 2016/03/09 21:06:44]

Simple it indeed is but this is as it was before this CL. I.e.: cancellers were
allocated when needed but not destructed when no longer needed. There is a
slight point in doing that since if they will again be needed later on they are
already constructed and no overhead of canceller construction will be taken.

Having said that, this scheme is a thing that should be refactored, and since I
now see an opportunity to start that refactoring within the scope of this CL, I
will boldly take that opportunity :-)

+      Handle* my_handle = gain_controllers_[i]->state();
       WebRtcAgc_AddFarend(my_handle, &capture_queue_buffer_[buffer_index],
                           num_frames_per_band);
 
@@ -127,19 +155,19 @@ void GainControlImpl::ReadQueuedRenderData() {
 int GainControlImpl::AnalyzeCaptureAudio(AudioBuffer* audio) {
   rtc::CritScope cs(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
-  assert(audio->num_frames_per_band() <= 160);
-  assert(audio->num_channels() == num_handles());
+  RTC_DCHECK_GE(160u, audio->num_frames_per_band());
+  RTC_DCHECK_EQ(audio->num_channels(), num_handles_required());

[the sun, 2016/03/09 09:42:11]

num_handles_required() -> gain_controllers_.size(), here, and below methods and
loops.

[peah-webrtc, 2016/03/09 12:23:39]

Done.

 
   int err = AudioProcessing::kNoError;
 
   if (mode_ == kAdaptiveAnalog) {
-    capture_levels_.assign(num_handles(), analog_capture_level_);
-    for (size_t i = 0; i < num_handles(); i++) {
-      Handle* my_handle = static_cast<Handle*>(handle(i));
+    capture_levels_.assign(num_handles_required(), analog_capture_level_);
+    for (size_t i = 0; i < num_handles_required(); i++) {
+      Handle* my_handle = gain_controllers_[i]->state();
       err = WebRtcAgc_AddMic(
           my_handle,
           audio->split_bands(i),

[the sun, 2016/03/09 09:42:10]

Is there one split band per channel? Is the DCHECK_EQ above correct?

[peah-webrtc, 2016/03/09 12:23:39]

In analog_agc.c, the in each channel bands are processed. Therefore, at this
level, the bands are just passed to the AGC, and the AGC then received a **
(pointer to pointer).
Therefore, I think the DCHECK is fine (and it also matches what was in place
before this CL as well as before the locking scheme was introduced.

@@ -147,13 +175,12 @@ int GainControlImpl::AnalyzeCaptureAudio(AudioBuffer* audio) {
           audio->num_frames_per_band());
 
       if (err != AudioProcessing::kNoError) {
-        return GetHandleError(my_handle);
+        return AudioProcessing::kUnspecifiedError;
       }
     }
   } else if (mode_ == kAdaptiveDigital) {
-
-    for (size_t i = 0; i < num_handles(); i++) {
-      Handle* my_handle = static_cast<Handle*>(handle(i));
+    for (size_t i = 0; i < num_handles_required(); i++) {
+      Handle* my_handle = gain_controllers_[i]->state();
       int32_t capture_level_out = 0;
 
       err = WebRtcAgc_VirtualMic(
@@ -167,7 +194,7 @@ int GainControlImpl::AnalyzeCaptureAudio(AudioBuffer* audio) {
       capture_levels_[i] = capture_level_out;
 
       if (err != AudioProcessing::kNoError) {
-        return GetHandleError(my_handle);
+        return AudioProcessing::kUnspecifiedError;
       }
 
     }
@@ -179,7 +206,7 @@ int GainControlImpl::AnalyzeCaptureAudio(AudioBuffer* audio) {
 int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   rtc::CritScope cs(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
@@ -187,12 +214,12 @@ int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
     return AudioProcessing::kStreamParameterNotSetError;
   }
 
-  assert(audio->num_frames_per_band() <= 160);
-  assert(audio->num_channels() == num_handles());
+  RTC_DCHECK_GE(160u, audio->num_frames_per_band());
+  RTC_DCHECK_EQ(audio->num_channels(), num_handles_required());
 
   stream_is_saturated_ = false;
-  for (size_t i = 0; i < num_handles(); i++) {
-    Handle* my_handle = static_cast<Handle*>(handle(i));
+  for (size_t i = 0; i < num_handles_required(); i++) {
+    Handle* my_handle = gain_controllers_[i]->state();
     int32_t capture_level_out = 0;
     uint8_t saturation_warning = 0;
 
@@ -210,7 +237,7 @@ int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
         &saturation_warning);
 
     if (err != AudioProcessing::kNoError) {
-      return GetHandleError(my_handle);
+      return AudioProcessing::kUnspecifiedError;
     }
 
     capture_levels_[i] = capture_level_out;
@@ -222,11 +249,11 @@ int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   if (mode_ == kAdaptiveAnalog) {
     // Take the analog level to be the average across the handles.
     analog_capture_level_ = 0;
-    for (size_t i = 0; i < num_handles(); i++) {
+    for (size_t i = 0; i < num_handles_required(); i++) {
       analog_capture_level_ += capture_levels_[i];

[the sun, 2016/03/09 09:42:10]

Make capture_level_ a member of GainController to avoid keeping the arrays in
sync?

[peah-webrtc, 2016/03/09 12:23:39]

Awesome idea!!!
Done.

     }
 
-    analog_capture_level_ /= num_handles();
+    analog_capture_level_ /= num_handles_required();
   }
 
   was_analog_level_set_ = false;
@@ -257,12 +284,18 @@ int GainControlImpl::stream_analog_level() {
 int GainControlImpl::Enable(bool enable) {
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
-  return EnableComponent(enable);
+  if (enable && !enabled_) {
+    enabled_ = enable;  // Must be set before Initialize() is called.
+    Initialize();
+  } else {
+    enabled_ = enable;
+  }
+  return AudioProcessing::kNoError;
 }
 
 bool GainControlImpl::is_enabled() const {
   rtc::CritScope cs(crit_capture_);
-  return is_component_enabled();
+  return enabled_;
 }
 
 int GainControlImpl::set_mode(Mode mode) {
@@ -273,7 +306,8 @@ int GainControlImpl::set_mode(Mode mode) {
   }
 
   mode_ = mode;
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 GainControl::Mode GainControlImpl::mode() const {
@@ -299,7 +333,8 @@ int GainControlImpl::set_analog_level_limits(int minimum,
   minimum_capture_level_ = minimum;
   maximum_capture_level_ = maximum;
 
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 int GainControlImpl::analog_level_minimum() const {
@@ -318,12 +353,14 @@ bool GainControlImpl::stream_is_saturated() const {
 }
 
 int GainControlImpl::set_target_level_dbfs(int level) {
-  rtc::CritScope cs(crit_capture_);
-  if (level > 31 || level < 0) {
-    return AudioProcessing::kBadParameterError;
-  }
+  {
+    rtc::CritScope cs(crit_capture_);
+    if (level > 31 || level < 0) {

[the sun, 2016/03/09 09:42:11]

Verify the input parameter outside this scope, before taking the lock.

[peah-webrtc, 2016/03/09 12:23:39]

Done.

+      return AudioProcessing::kBadParameterError;
+    }
 
-  target_level_dbfs_ = level;
+    target_level_dbfs_ = level;
+  }
   return Configure();
 }
 
@@ -333,12 +370,14 @@ int GainControlImpl::target_level_dbfs() const {
 }
 
 int GainControlImpl::set_compression_gain_db(int gain) {
-  rtc::CritScope cs(crit_capture_);
-  if (gain < 0 || gain > 90) {
-    return AudioProcessing::kBadParameterError;
-  }
+  {
+    rtc::CritScope cs(crit_capture_);
+    if (gain < 0 || gain > 90) {

[the sun, 2016/03/09 09:42:11]

ditto

[peah-webrtc, 2016/03/09 12:23:39]

Done.

+      return AudioProcessing::kBadParameterError;
+    }
 
-  compression_gain_db_ = gain;
+    compression_gain_db_ = gain;
+  }
   return Configure();
 }
 
@@ -348,8 +387,10 @@ int GainControlImpl::compression_gain_db() const {
 }
 
 int GainControlImpl::enable_limiter(bool enable) {
-  rtc::CritScope cs(crit_capture_);
-  limiter_enabled_ = enable;
+  {
+    rtc::CritScope cs(crit_capture_);
+    limiter_enabled_ = enable;
+  }
   return Configure();
 }
 
@@ -358,26 +399,44 @@ bool GainControlImpl::is_limiter_enabled() const {
   return limiter_enabled_;
 }
 
-int GainControlImpl::Initialize() {
-  int err = ProcessingComponent::Initialize();
-  if (err != AudioProcessing::kNoError || !is_component_enabled()) {
-    return err;
+void GainControlImpl::Initialize() {
+  rtc::CritScope cs_render(crit_render_);
+  rtc::CritScope cs_capture(crit_capture_);
+  if (!enabled_) {
+    return;
+  }
+
+  const int sample_rate_hz = apm_->proc_sample_rate_hz();
+
+  if (num_handles_required() > gain_controllers_.size()) {
+    const size_t gain_controllers_old_size = gain_controllers_.size();

[the sun, 2016/03/09 09:42:10]

Remove const

[peah-webrtc, 2016/03/09 12:23:39]

Done.

+    gain_controllers_.resize(num_handles_required());

[the sun, 2016/03/09 13:01:53]

Like the comment for EchoControlMobile - is there any reason to keep the unused
objects around, if num_handles_required() decreases?

+
+    for (size_t i = gain_controllers_old_size; i < gain_controllers_.size();
+         ++i) {
+      gain_controllers_[i].reset(new GainController());
+    }
   }
 
+  for (auto& gain_controller : gain_controllers_) {
+    gain_controller->Initialize(minimum_capture_level_, maximum_capture_level_,
+                                mode_, sample_rate_hz);
+  }
+
+  Configure();
+
   AllocateRenderQueue();
 
-  rtc::CritScope cs_capture(crit_capture_);
-  const int n = num_handles();
+  const int n = num_handles_required();
   RTC_CHECK_GE(n, 0) << "Bad number of handles: " << n;
 
   capture_levels_.assign(n, analog_capture_level_);
-  return AudioProcessing::kNoError;
 }
 
 void GainControlImpl::AllocateRenderQueue() {
-  const size_t new_render_queue_element_max_size =
-      std::max<size_t>(static_cast<size_t>(1),
-                       kMaxAllowedValuesOfSamplesPerFrame * num_handles());
+  const size_t new_render_queue_element_max_size = std::max<size_t>(
+      static_cast<size_t>(1),
+      kMaxAllowedValuesOfSamplesPerFrame * num_handles_required());
 
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
@@ -398,26 +457,7 @@ void GainControlImpl::AllocateRenderQueue() {
   }
 }
 
-void* GainControlImpl::CreateHandle() const {
-  return WebRtcAgc_Create();
-}
-
-void GainControlImpl::DestroyHandle(void* handle) const {
-  WebRtcAgc_Free(static_cast<Handle*>(handle));
-}
-
-int GainControlImpl::InitializeHandle(void* handle) const {
-  rtc::CritScope cs_render(crit_render_);
-  rtc::CritScope cs_capture(crit_capture_);
-
-  return WebRtcAgc_Init(static_cast<Handle*>(handle),
-                          minimum_capture_level_,
-                          maximum_capture_level_,
-                          MapSetting(mode_),
-                          apm_->proc_sample_rate_hz());
-}
-
-int GainControlImpl::ConfigureHandle(void* handle) const {
+int GainControlImpl::Configure() {
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
   WebRtcAgcConfig config;
@@ -430,18 +470,19 @@ int GainControlImpl::ConfigureHandle(void* handle) const {
       static_cast<int16_t>(compression_gain_db_);
   config.limiterEnable = limiter_enabled_;
 
-  return WebRtcAgc_set_config(static_cast<Handle*>(handle), config);
+  int error = AudioProcessing::kNoError;
+  for (auto& gain_controller : gain_controllers_) {
+    const int handle_error =
+        WebRtcAgc_set_config(gain_controller->state(), config);
+    if (handle_error != AudioProcessing::kNoError) {
+      error = AudioProcessing::kNoError;

[the sun, 2016/03/09 09:42:11]

error = handle_error

[peah-webrtc, 2016/03/09 12:23:39]

Good catch!
Done.

+    }
+  }
+  return error;
 }
 
 size_t GainControlImpl::num_handles_required() const {
   // Not locked as it only relies on APM public API which is threadsafe.
   return apm_->num_proc_channels();
 }
-
-int GainControlImpl::GetHandleError(void* handle) const {
-  // The AGC has no get_error() function.
-  // (Despite listing errors in its interface...)
-  assert(handle != NULL);
-  return AudioProcessing::kUnspecifiedError;
-}
 }  // namespace webrtc