Removing dependency of the EchoControlMobileImpl class on ProcessingComponent.

webrtc/modules/audio_processing/echo_control_mobile_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.
  */
 
@@ skipping 46 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
 
 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),
       comfort_noise_enabled_(true),
       external_echo_path_(NULL),
       render_queue_element_max_size_(0) {
   RTC_DCHECK(apm);
   RTC_DCHECK(crit_render);
   RTC_DCHECK(crit_capture);
 }
 
 EchoControlMobileImpl::~EchoControlMobileImpl() {
     if (external_echo_path_ != NULL) {
       delete [] external_echo_path_;
       external_echo_path_ = NULL;
     }
 }
 
 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.
   size_t handle_index = 0;
   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());
 
       if (err != AudioProcessing::kNoError)
         return MapError(err);  // TODO(ajm): warning possible?);
 
       // Buffer the samples in the render queue.
       render_queue_buffer_.insert(render_queue_buffer_.end(),
                                   audio->split_bands_const(j)[kBand0To8kHz],
                                   (audio->split_bands_const(j)[kBand0To8kHz] +
                                    audio->num_frames_per_band()));
-
-      handle_index++;
     }
   }
 
   // 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 AEC.
 void EchoControlMobileImpl::ReadQueuedRenderData() {
   rtc::CritScope cs_capture(crit_capture_);
 
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return;
   }
 
   while (render_signal_queue_->Remove(&capture_queue_buffer_)) {
     size_t handle_index = 0;
     size_t buffer_index = 0;
     const size_t num_frames_per_band =
         capture_queue_buffer_.size() /
         (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++;
       }
     }
   }
 }
 
 int EchoControlMobileImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   rtc::CritScope cs_capture(crit_capture_);
-
-  if (!is_component_enabled()) {
+  if (!enabled_) {
     return AudioProcessing::kNoError;
   }
 
   if (!apm_->was_stream_delay_set()) {
     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;
 
   // The ordering convention must be followed to pass to the correct AECM.
   size_t handle_index = 0;
   for (size_t i = 0; i < audio->num_channels(); i++) {
     // TODO(ajm): improve how this works, possibly inside AECM.
     //            This is kind of hacked up.
     const int16_t* noisy = audio->low_pass_reference(i);
     const int16_t* clean = audio->split_bands_const(i)[kBand0To8kHz];
     if (noisy == NULL) {
       noisy = clean;
       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,
           clean,
           audio->split_bands(i)[kBand0To8kHz],
           audio->num_frames_per_band(),
           apm_->stream_delay_ms());
 
       if (err != AudioProcessing::kNoError)
         return MapError(err);
-
-      handle_index++;
     }
   }
 
   return AudioProcessing::kNoError;
 }
 
 int EchoControlMobileImpl::Enable(bool enable) {
   // Ensure AEC and AECM are not both enabled.
   rtc::CritScope cs_render(crit_render_);
   rtc::CritScope cs_capture(crit_capture_);
   // The is_enabled call is safe from a deadlock perspective
   // as both locks are allready held in the correct order.
   if (enable && apm_->echo_cancellation()->is_enabled()) {
     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) {
   if (MapSetting(mode) == -1) {
     return AudioProcessing::kBadParameterError;
   }
 
   {
     rtc::CritScope cs(crit_capture_);
     routing_mode_ = mode;
@@ skipping 32 matching lines @@
       // Size mismatch
       return AudioProcessing::kBadParameterError;
     }
 
     if (external_echo_path_ == NULL) {
       external_echo_path_ = new unsigned char[size_bytes];
     }
     memcpy(external_echo_path_, echo_path, size_bytes);
   }
 
-  return Initialize();
+  Initialize();
+  return AudioProcessing::kNoError;
 }
 
 int EchoControlMobileImpl::GetEchoPath(void* echo_path,
                                        size_t size_bytes) const {
   rtc::CritScope cs(crit_capture_);
   if (echo_path == NULL) {
     return AudioProcessing::kNullPointerError;
   }
   if (size_bytes != echo_path_size_bytes()) {
     // 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());
+    }
   }
 
+  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());
+  }
+
+  Configure();
+
   AllocateRenderQueue();
-
-  return AudioProcessing::kNoError;
 }
 
 void EchoControlMobileImpl::AllocateRenderQueue() {
   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_);
 
   // Reallocate the queue if the queue item size is too small to fit the
   // data to put in the queue.
   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* 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