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..0f8cdefdd70c0157aa1c4386649ebd2d39d261d7 100644
--- a/webrtc/modules/audio_processing/echo_control_mobile_impl.cc
+++ b/webrtc/modules/audio_processing/echo_control_mobile_impl.cc
@@ -64,14 +64,43 @@ 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_DCHECK(state_);

[the sun, 2016/03/07 14:43:12]

RTC_CHECK

[peah-webrtc, 2016/03/08 07:53:32]

Done.

+  }
+
+  ~Canceller() {
+    RTC_CHECK(state_);

[the sun, 2016/03/07 14:43:12]

RTC_DCHECK, or just don't check - as per the ctor it's part of the Canceller
invariant that state_ != nullptr

[peah-webrtc, 2016/03/08 07:53:32]

Done.

+    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);

[the sun, 2016/03/07 14:43:12]

DCHECK or CHECK? Can this ever fail? To we reallocate internal buffers or
anything?

[peah-webrtc, 2016/03/08 07:53:32]

Afaics, there is only buffer creation inside the WebRtcAecm_Create method so I
think a DCHECK should be used.
However, there are lots of checks being done so the method can fail (but not by
failing to allocate memory).

+    if (external_echo_path != NULL) {
+      error = WebRtcAecm_InitEchoPath(state_, external_echo_path,
+                                      echo_path_size_bytes);
+      RTC_DCHECK_EQ(AudioProcessing::kNoError, error);

[the sun, 2016/03/07 14:43:12]

Likewise - DCHECK or CHECK?

[peah-webrtc, 2016/03/08 07:53:32]

I'd say DCHECK, since no memory allocation takes place.

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

Same as above, runtime or logic? If the arguments we supply should never ever
cause an error to be returned, go with DCHECK.

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

Acknowledged.

+    }
+  }
+
+ private:
+  Handle* state_;

[the sun, 2016/03/07 14:43:13]

RTC_DISALLOW_COPY_AND_ASSIGN(EchoControlMobileImpl::Canceller);

[peah-webrtc, 2016/03/08 07:53:32]

Done.

+};
+
 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 +121,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 +136,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();

[the sun, 2016/03/07 14:43:13]

fyi: while neat, doing multiple things in the same line actually makes it harder
to read and to step through in a debugger

[peah-webrtc, 2016/03/08 07:53:32]

True, but I think it is kind of an edge case since this counter is only used for
the purpose of indexing the cancellers_ vector.

Would it be better to do the increment of handle_index inside the 
for (size_t j = 0; j < audio->num_channels(); j++) increment statement?

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

Never mind.

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

Acknowledged.

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

:-)

       err = WebRtcAecm_GetBufferFarendError(
           my_handle, audio->split_bands_const(j)[kBand0To8kHz],
           audio->num_frames_per_band());
@@ -119,8 +149,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 +169,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 +181,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 +193,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 +201,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 +218,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 +229,6 @@ int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
 
       if (err != AudioProcessing::kNoError)
         return MapError(err);
-
-      handle_index++;
     }
   }
 
@@ -221,12 +245,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 +309,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 +323,52 @@ 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 =

[the sun, 2016/03/07 14:43:13]

I don't think you should const declare this local. You'll have a LOT of work to
do if you're to do that consistently (and you already missed one in this
method).

[peah-webrtc, 2016/03/08 07:53:32]

Not really sure what you mean here. Even though not done consistently in the
code, I think that whenever possible it makes sense to const declare (Effective
C++ Item #3).

Not sure what variable you are referring to that has been missed to const
declare in this method? Do you mean size_bytes? 

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

Discussed offline.

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

Acknowledged.

+      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;
+    RTC_DCHECK_GE(AudioProcessing::kSampleRate16kHz,

[the sun, 2016/03/07 14:43:13]

What is the DCHECK for? You're already logging. Besides, looks to me like the
DCHECK condition is wrong and will always trigger if we get inside this
conditional, so perhaps there's a test case missing too?

[peah-webrtc, 2016/03/08 07:53:32]

You are correct in that the condition is wrong. I'll remove the DCHECK.

+                  apm_->proc_sample_rate_hz());
   }
 
-  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 (size_t i = 0; i < cancellers_.size(); ++i) {

[the sun, 2016/03/07 14:43:12]

nit:
for (auto canceller : cancellers_) {
  ...

[peah-webrtc, 2016/03/08 07:53:32]

Done.

+    cancellers_[i]->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 (size_t i = 0; i < cancellers_.size(); i++) {

[the sun, 2016/03/07 14:43:13]

ditto

[peah-webrtc, 2016/03/08 07:53:32]

Done.

+    Handle* my_handle = cancellers_[i]->state();
+    const int handle_error = WebRtcAecm_set_config(my_handle, config);
+    if (handle_error != AudioProcessing::kNoError) {
+      error = AudioProcessing::kNoError;
+    }
+  }
+  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