Removed the dependency in GainControlImpl on the ProcessingComponent class

webrtc/modules/audio_processing/gain_control_impl.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/audio_processing/gain_control_impl.h"
 
 #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"
 
 namespace webrtc {
 
 typedef void Handle;
 
 namespace {
 int16_t MapSetting(GainControl::Mode mode) {
   switch (mode) {
@@ skipping 10 matching lines @@
 
 // Maximum length that a frame of samples can have.
 static const size_t kMaxAllowedValuesOfSamplesPerFrame = 160;
 // Maximum number of frames to buffer in the render queue.
 // TODO(peah): Decrease this once we properly handle hugely unbalanced
 // reverse and forward call numbers.
 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),
       minimum_capture_level_(0),
       maximum_capture_level_(255),
       limiter_enabled_(true),
       target_level_dbfs_(3),
       compression_gain_db_(9),
       analog_capture_level_(0),
       was_analog_level_set_(false),
       stream_is_saturated_(false),
       render_queue_element_max_size_(0) {
   RTC_DCHECK(apm);
   RTC_DCHECK(crit_render);
   RTC_DCHECK(crit_capture);
 }
 
 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(
         render_queue_buffer_.end(), audio->mixed_low_pass_data(),
         (audio->mixed_low_pass_data() + audio->num_frames_per_band()));
   }
 
   // Insert the samples into the queue.
   if (!render_signal_queue_->Insert(&render_queue_buffer_)) {
     // The data queue is full and needs to be emptied.
     ReadQueuedRenderData();
 
     // Retry the insert (should always work).
     RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true);
   }
 
   return AudioProcessing::kNoError;
 }
 
 // Read chunks of data that were received and queued on the render side from
 // a queue. All the data chunks are buffered into the farend signal of the AGC.
 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);
 
       buffer_index += num_frames_per_band;
     }
   }
 }
 
 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.

           audio->num_bands(),
           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(
           my_handle,
           audio->split_bands(i),
           audio->num_bands(),
           audio->num_frames_per_band(),
           analog_capture_level_,
           &capture_level_out);
 
       capture_levels_[i] = capture_level_out;
 
       if (err != AudioProcessing::kNoError) {
-        return GetHandleError(my_handle);
+        return AudioProcessing::kUnspecifiedError;
       }
 
     }
   }
 
   return AudioProcessing::kNoError;
 }
 
 int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   rtc::CritScope cs(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
   if (mode_ == kAdaptiveAnalog && !was_analog_level_set_) {
     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;
 
     // The call to stream_has_echo() is ok from a deadlock perspective
     // as the capture lock is allready held.
     int err = WebRtcAgc_Process(
         my_handle,
         audio->split_bands_const(i),
         audio->num_bands(),
         audio->num_frames_per_band(),
         audio->split_bands(i),
         capture_levels_[i],
         &capture_level_out,
         apm_->echo_cancellation()->stream_has_echo(),
         &saturation_warning);
 
     if (err != AudioProcessing::kNoError) {
-      return GetHandleError(my_handle);
+      return AudioProcessing::kUnspecifiedError;
     }
 
     capture_levels_[i] = capture_level_out;
     if (saturation_warning == 1) {
       stream_is_saturated_ = true;
     }
   }
 
   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;
   return AudioProcessing::kNoError;
 }
 
 // TODO(ajm): ensure this is called under kAdaptiveAnalog.
 int GainControlImpl::set_stream_analog_level(int level) {
   rtc::CritScope cs(crit_capture_);
 
@@ skipping 10 matching lines @@
   rtc::CritScope cs(crit_capture_);
   // TODO(ajm): enable this assertion?
   //assert(mode_ == kAdaptiveAnalog);
 
   return analog_capture_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) {
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
   if (MapSetting(mode) == -1) {
     return AudioProcessing::kBadParameterError;
   }
 
   mode_ = mode;
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 GainControl::Mode GainControlImpl::mode() const {
   rtc::CritScope cs(crit_capture_);
   return mode_;
 }
 
 int GainControlImpl::set_analog_level_limits(int minimum,
                                              int maximum) {
   rtc::CritScope cs(crit_capture_);
   if (minimum < 0) {
     return AudioProcessing::kBadParameterError;
   }
 
   if (maximum > 65535) {
     return AudioProcessing::kBadParameterError;
   }
 
   if (maximum < minimum) {
     return AudioProcessing::kBadParameterError;
   }
 
   minimum_capture_level_ = minimum;
   maximum_capture_level_ = maximum;
 
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 int GainControlImpl::analog_level_minimum() const {
   rtc::CritScope cs(crit_capture_);
   return minimum_capture_level_;
 }
 
 int GainControlImpl::analog_level_maximum() const {
   rtc::CritScope cs(crit_capture_);
   return maximum_capture_level_;
 }
 
 bool GainControlImpl::stream_is_saturated() const {
   rtc::CritScope cs(crit_capture_);
   return stream_is_saturated_;
 }
 
 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();
 }
 
 int GainControlImpl::target_level_dbfs() const {
   rtc::CritScope cs(crit_capture_);
   return target_level_dbfs_;
 }
 
 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();
 }
 
 int GainControlImpl::compression_gain_db() const {
   rtc::CritScope cs(crit_capture_);
   return compression_gain_db_;
 }
 
 int GainControlImpl::enable_limiter(bool enable) {
-  rtc::CritScope cs(crit_capture_);
-  limiter_enabled_ = enable;
+  {
+    rtc::CritScope cs(crit_capture_);
+    limiter_enabled_ = enable;
+  }
   return Configure();
 }
 
 bool GainControlImpl::is_limiter_enabled() const {
   rtc::CritScope cs(crit_capture_);
   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_);
 
   if (render_queue_element_max_size_ < new_render_queue_element_max_size) {
     render_queue_element_max_size_ = new_render_queue_element_max_size;
     std::vector<int16_t> template_queue_element(render_queue_element_max_size_);
 
     render_signal_queue_.reset(
         new SwapQueue<std::vector<int16_t>, RenderQueueItemVerifier<int16_t>>(
             kMaxNumFramesToBuffer, template_queue_element,
             RenderQueueItemVerifier<int16_t>(render_queue_element_max_size_)));
 
     render_queue_buffer_.resize(render_queue_element_max_size_);
     capture_queue_buffer_.resize(render_queue_element_max_size_);
   } else {
     render_signal_queue_->Clear();
   }
 }
 
-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;
   // TODO(ajm): Flip the sign here (since AGC expects a positive value) if we
   //            change the interface.
   //assert(target_level_dbfs_ <= 0);
   //config.targetLevelDbfs = static_cast<int16_t>(-target_level_dbfs_);
   config.targetLevelDbfs = static_cast<int16_t>(target_level_dbfs_);
   config.compressionGaindB =
       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