Removing dependency of the EchoControlMobileImpl class on ProcessingComponent.

webrtc/modules/audio_processing/echo_control_mobile_impl.cc

Index: webrtc/modules/audio_processing/echo_control_mobile_impl.cc
diff --git a/webrtc/modules/audio_processing/echo_control_mobile_impl.cc b/webrtc/modules/audio_processing/echo_control_mobile_impl.cc
index f2df5f79849f7fb98a85022506c4cceb26423588..c7d7588b14c2c727b44c694ddce816863e463131 100644
--- a/webrtc/modules/audio_processing/echo_control_mobile_impl.cc
+++ b/webrtc/modules/audio_processing/echo_control_mobile_impl.cc
@@ -13,6 +13,7 @@
 #include <assert.h>
 #include <string.h>
 
+#include "webrtc/base/constructormagic.h"

[the sun, 2016/03/09 09:23:14]

Not needed.

[peah-webrtc, 2016/03/09 12:34:53]

Done.

 #include "webrtc/modules/audio_processing/aecm/echo_control_mobile.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/system_wrappers/include/logging.h"
@@ -64,14 +65,44 @@ static const size_t kMaxNumFramesToBuffer = 100;
 }  // namespace
 
 size_t EchoControlMobile::echo_path_size_bytes() {
-    return WebRtcAecm_echo_path_size_bytes();
+  return WebRtcAecm_echo_path_size_bytes();
 }
 
+class EchoControlMobileImpl::Canceller {
+ public:
+  Canceller() {
+    state_ = WebRtcAecm_Create();
+    RTC_CHECK(state_);
+  }
+
+  ~Canceller() {
+    RTC_DCHECK(state_);
+    WebRtcAecm_Free(state_);
+  }
+
+  Handle* state() { return state_; }
+
+  void Initialize(int sample_rate_hz,
+                  unsigned char* external_echo_path,
+                  size_t echo_path_size_bytes) {
+    int error = WebRtcAecm_Init(state_, sample_rate_hz);
+    RTC_DCHECK_EQ(AudioProcessing::kNoError, error);
+    if (external_echo_path != NULL) {
+      error = WebRtcAecm_InitEchoPath(state_, external_echo_path,

[the sun, 2016/03/09 09:23:14]

nit: since you're DCHECKing in the dtor, you should DCHECK here as well, for
consistency. Or don't DCHECK in the dtor. state_!=0 is part of the class
invariant according to ctor.

[peah-webrtc, 2016/03/09 12:34:53]

This makes a lot of sense. Since the class is so small, I'll add DCHECKS for all
the methods apart from the ctor (which includes a CHECK as memory is allocated)
and dtor.

Done.

[the sun, 2016/03/09 12:46:11]

I don't understand. Why did you remove the DCHECK in the dtor? I think you
should be consistently DCHECKing whenever state_ is referenced, or rely on the
class invariant (which is what I would do).

[peah-webrtc, 2016/03/09 14:29:52]

Sorry, I misunderstood you. Now I get it :-)
Done.

+                                      echo_path_size_bytes);
+      RTC_DCHECK_EQ(AudioProcessing::kNoError, error);
+    }
+  }
+
+ private:
+  Handle* state_;
+  RTC_DISALLOW_COPY_AND_ASSIGN(Canceller);
+};
+
 EchoControlMobileImpl::EchoControlMobileImpl(const AudioProcessing* apm,
                                              rtc::CriticalSection* crit_render,
                                              rtc::CriticalSection* crit_capture)
-    : ProcessingComponent(),
-      apm_(apm),
+    : apm_(apm),
       crit_render_(crit_render),
       crit_capture_(crit_capture),
       routing_mode_(kSpeakerphone),
@@ -92,13 +123,14 @@ EchoControlMobileImpl::~EchoControlMobileImpl() {
 
 int EchoControlMobileImpl::ProcessRenderAudio(const AudioBuffer* audio) {
   rtc::CritScope cs_render(crit_render_);
-
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
-  assert(audio->num_frames_per_band() <= 160);
-  assert(audio->num_channels() == apm_->num_reverse_channels());
+  RTC_DCHECK_GE(160u, audio->num_frames_per_band());
+  RTC_DCHECK_EQ(audio->num_channels(), apm_->num_reverse_channels());
+  RTC_DCHECK_GE(cancellers_.size(),
+                apm_->num_output_channels() * audio->num_channels());
 
   int err = AudioProcessing::kNoError;
   // The ordering convention must be followed to pass to the correct AECM.
@@ -106,7 +138,7 @@ int EchoControlMobileImpl::ProcessRenderAudio(const AudioBuffer* audio) {
   render_queue_buffer_.clear();
   for (size_t i = 0; i < apm_->num_output_channels(); i++) {
     for (size_t j = 0; j < audio->num_channels(); j++) {
-      Handle* my_handle = static_cast<Handle*>(handle(handle_index));
+      Handle* my_handle = cancellers_[handle_index++]->state();
       err = WebRtcAecm_GetBufferFarendError(
           my_handle, audio->split_bands_const(j)[kBand0To8kHz],
           audio->num_frames_per_band());
@@ -119,8 +151,6 @@ int EchoControlMobileImpl::ProcessRenderAudio(const AudioBuffer* audio) {
                                   audio->split_bands_const(j)[kBand0To8kHz],
                                   (audio->split_bands_const(j)[kBand0To8kHz] +
                                    audio->num_frames_per_band()));
-
-      handle_index++;
     }
   }
 
@@ -141,7 +171,7 @@ int EchoControlMobileImpl::ProcessRenderAudio(const AudioBuffer* audio) {
 void EchoControlMobileImpl::ReadQueuedRenderData() {
   rtc::CritScope cs_capture(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return;
   }
 
@@ -153,12 +183,11 @@ void EchoControlMobileImpl::ReadQueuedRenderData() {
         (apm_->num_output_channels() * apm_->num_reverse_channels());
     for (size_t i = 0; i < apm_->num_output_channels(); i++) {
       for (size_t j = 0; j < apm_->num_reverse_channels(); j++) {
-        Handle* my_handle = static_cast<Handle*>(handle(handle_index));
+        Handle* my_handle = cancellers_[handle_index++]->state();
         WebRtcAecm_BufferFarend(my_handle, &capture_queue_buffer_[buffer_index],
                                 num_frames_per_band);
 
         buffer_index += num_frames_per_band;
-        handle_index++;
       }
     }
   }
@@ -166,8 +195,7 @@ void EchoControlMobileImpl::ReadQueuedRenderData() {
 
 int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   rtc::CritScope cs_capture(crit_capture_);
-
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
@@ -175,8 +203,8 @@ int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
     return AudioProcessing::kStreamParameterNotSetError;
   }
 
-  assert(audio->num_frames_per_band() <= 160);
-  assert(audio->num_channels() == apm_->num_output_channels());
+  RTC_DCHECK_GE(160u, audio->num_frames_per_band());
+  RTC_DCHECK_EQ(audio->num_channels(), apm_->num_output_channels());
 
   int err = AudioProcessing::kNoError;
 
@@ -192,7 +220,7 @@ int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
       clean = NULL;
     }
     for (size_t j = 0; j < apm_->num_reverse_channels(); j++) {
-      Handle* my_handle = static_cast<Handle*>(handle(handle_index));
+      Handle* my_handle = cancellers_[handle_index++]->state();
       err = WebRtcAecm_Process(
           my_handle,
           noisy,
@@ -203,8 +231,6 @@ int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
 
       if (err != AudioProcessing::kNoError)
         return MapError(err);
-
-      handle_index++;
     }
   }
 
@@ -221,12 +247,18 @@ int EchoControlMobileImpl::Enable(bool enable) {
     return AudioProcessing::kBadParameterError;
   }
 
-  return EnableComponent(enable);
+  if (enable && !enabled_) {
+    enabled_ = enable;  // Must be set before Initialize() is called.
+    Initialize();
+  } else {
+    enabled_ = enable;
+  }
+  return AudioProcessing::kNoError;
 }
 
 bool EchoControlMobileImpl::is_enabled() const {
   rtc::CritScope cs(crit_capture_);
-  return is_component_enabled();
+  return enabled_;
 }
 
 int EchoControlMobileImpl::set_routing_mode(RoutingMode mode) {
@@ -279,7 +311,8 @@ int EchoControlMobileImpl::SetEchoPath(const void* echo_path,
     memcpy(external_echo_path_, echo_path, size_bytes);
   }
 
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 int EchoControlMobileImpl::GetEchoPath(void* echo_path,
@@ -292,40 +325,50 @@ int EchoControlMobileImpl::GetEchoPath(void* echo_path,
     // Size mismatch
     return AudioProcessing::kBadParameterError;
   }
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNotEnabledError;
   }
 
   // Get the echo path from the first channel
-  Handle* my_handle = static_cast<Handle*>(handle(0));
-  int32_t err = WebRtcAecm_GetEchoPath(my_handle, echo_path, size_bytes);
-  if (err != 0)
+  const int32_t err =
+      WebRtcAecm_GetEchoPath(cancellers_[0]->state(), echo_path, size_bytes);
+  if (err != 0) {
     return MapError(err);
+  }
 
   return AudioProcessing::kNoError;
 }
 
-int EchoControlMobileImpl::Initialize() {
-  {
-    rtc::CritScope cs_capture(crit_capture_);
-    if (!is_component_enabled()) {
-      return AudioProcessing::kNoError;
-    }
+void EchoControlMobileImpl::Initialize() {
+  rtc::CritScope cs_render(crit_render_);
+  rtc::CritScope cs_capture(crit_capture_);
+  if (!enabled_) {
+    return;
   }
 
   if (apm_->proc_sample_rate_hz() > AudioProcessing::kSampleRate16kHz) {
     LOG(LS_ERROR) << "AECM only supports 16 kHz or lower sample rates";
-    return AudioProcessing::kBadSampleRateError;
   }
 
-  int err = ProcessingComponent::Initialize();
-  if (err != AudioProcessing::kNoError) {
-    return err;
+  const int sample_rate_hz = apm_->proc_sample_rate_hz();
+
+  if (num_handles_required() > cancellers_.size()) {
+    const size_t cancellers_old_size = cancellers_.size();
+    cancellers_.resize(num_handles_required());
+
+    for (size_t i = cancellers_old_size; i < cancellers_.size(); ++i) {
+      cancellers_[i].reset(new Canceller());
+    }
   }
 
-  AllocateRenderQueue();
+  for (auto& canceller : cancellers_) {
+    canceller->Initialize(sample_rate_hz, external_echo_path_,
+                          echo_path_size_bytes());
+  }
 
-  return AudioProcessing::kNoError;
+  Configure();
+
+  AllocateRenderQueue();
 }
 
 void EchoControlMobileImpl::AllocateRenderQueue() {
@@ -355,53 +398,25 @@ void EchoControlMobileImpl::AllocateRenderQueue() {
   }
 }
 
-void* EchoControlMobileImpl::CreateHandle() const {
-  return WebRtcAecm_Create();
-}
-
-void EchoControlMobileImpl::DestroyHandle(void* handle) const {
-  // This method is only called in a non-concurrent manner during APM
-  // destruction.
-  WebRtcAecm_Free(static_cast<Handle*>(handle));
-}
-
-int EchoControlMobileImpl::InitializeHandle(void* handle) const {
-  rtc::CritScope cs_render(crit_render_);
-  rtc::CritScope cs_capture(crit_capture_);
-  assert(handle != NULL);
-  Handle* my_handle = static_cast<Handle*>(handle);
-  if (WebRtcAecm_Init(my_handle, apm_->proc_sample_rate_hz()) != 0) {
-    return GetHandleError(my_handle);
-  }
-  if (external_echo_path_ != NULL) {
-    if (WebRtcAecm_InitEchoPath(my_handle,
-                                external_echo_path_,
-                                echo_path_size_bytes()) != 0) {
-      return GetHandleError(my_handle);
-    }
-  }
-
-  return AudioProcessing::kNoError;
-}
-
-int EchoControlMobileImpl::ConfigureHandle(void* handle) const {
+int EchoControlMobileImpl::Configure() {
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
   AecmConfig config;
   config.cngMode = comfort_noise_enabled_;
   config.echoMode = MapSetting(routing_mode_);
-
-  return WebRtcAecm_set_config(static_cast<Handle*>(handle), config);
+  int error = AudioProcessing::kNoError;
+  for (auto& canceller : cancellers_) {
+    Handle* my_handle = canceller->state();
+    const int handle_error = WebRtcAecm_set_config(my_handle, config);

[the sun, 2016/03/09 09:23:14]

There's that const again

[peah-webrtc, 2016/03/09 12:34:53]

Done.

+    if (handle_error != AudioProcessing::kNoError) {
+      error = AudioProcessing::kNoError;

[the sun, 2016/03/09 09:23:14]

Not nit:
= handle_error ?

[peah-webrtc, 2016/03/09 12:34:53]

Excellent find!

Done.

+    }
+  }
+  return error;
 }
 
 size_t EchoControlMobileImpl::num_handles_required() const {
   // Not locked as it only relies on APM public API which is threadsafe.
   return apm_->num_output_channels() * apm_->num_reverse_channels();
 }
-
-int EchoControlMobileImpl::GetHandleError(void* handle) const {
-  // Not locked as it does not rely on anything in the state.
-  assert(handle != NULL);
-  return AudioProcessing::kUnspecifiedError;
-}
 }  // namespace webrtc