Moved the AECM render sample queue into the audio processing module

webrtc/modules/audio_processing/audio_processing_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 690 matching lines @@
                                        float* const* dest) {
   TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_StreamConfig");
   ProcessingConfig processing_config;
   bool reinitialization_required = false;
   {
     // Acquire the capture lock in order to safely call the function
     // that retrieves the render side data. This function accesses apm
     // getters that need the capture lock held when being called.
     rtc::CritScope cs_capture(&crit_capture_);
     EmptyQueuedRenderAudio();
-    public_submodules_->echo_control_mobile->ReadQueuedRenderData();
     public_submodules_->gain_control->ReadQueuedRenderData();
 
     if (!src || !dest) {
       return kNullPointerError;
     }
 
     processing_config = formats_.api_format;
     reinitialization_required = UpdateActiveSubmoduleStates();
   }
 
@@ skipping 41 matching lines @@
                                           &crit_debug_, &debug_dump_.capture));
   }
 #endif
 
   return kNoError;
 }
 
 void AudioProcessingImpl::QueueRenderAudio(const AudioBuffer* audio) {
   EchoCancellationImpl::PackRenderAudioBuffer(audio, num_output_channels(),
                                               num_reverse_channels(),
-                                              &render_queue_buffer_);
+                                              &float_render_queue_buffer_);
 
   RTC_DCHECK_GE(160u, audio->num_frames_per_band());
 
   // Insert the samples into the queue.
-  if (!render_signal_queue_->Insert(&render_queue_buffer_)) {
+  if (!float_render_signal_queue_->Insert(&float_render_queue_buffer_)) {
     // The data queue is full and needs to be emptied.
     EmptyQueuedRenderAudio();
 
     // Retry the insert (should always work).
-    bool result = render_signal_queue_->Insert(&render_queue_buffer_);
+    bool result =
+        float_render_signal_queue_->Insert(&float_render_queue_buffer_);
+    RTC_DCHECK(result);
+  }
+
+  EchoControlMobileImpl::PackRenderAudioBuffer(audio, num_output_channels(),
+                                               num_reverse_channels(),
+                                               &int16_render_queue_buffer_);
+
+  // Insert the samples into the queue.
+  if (!int16_render_signal_queue_->Insert(&int16_render_queue_buffer_)) {
+    // The data queue is full and needs to be emptied.
+    EmptyQueuedRenderAudio();
+
+    // Retry the insert (should always work).
+    bool result =
+        int16_render_signal_queue_->Insert(&int16_render_queue_buffer_);
     RTC_DCHECK(result);
   }
 }
 
 void AudioProcessingImpl::AllocateRenderQueue() {
-  const size_t new_render_queue_element_max_size =
+  const size_t new_float_render_queue_element_max_size =
       std::max(static_cast<size_t>(1),
                kMaxAllowedValuesOfSamplesPerFrame *
                    EchoCancellationImpl::NumCancellersRequired(
                        num_output_channels(), num_reverse_channels()));
 
-  // 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;
+  const size_t new_int16_render_queue_element_max_size =

[ivoc, 2016/10/25 08:57:00]

Would this ever be different from the size of the float buffer in practice?

[peah-webrtc, 2016/10/25 09:48:33]

In practice it should not. However, currently these lengths implicitly depend on
how the submodules are initialized. In practice, it should not be a problem
since they are not used at the same time but until we do further refactoring I'd
prefer to have these lengths separate.

WDYT?

[ivoc, 2016/10/25 10:39:46]

Ok, that sounds fine.

+      std::max(static_cast<size_t>(1),
+               kMaxAllowedValuesOfSamplesPerFrame *
+                   EchoControlMobileImpl::NumCancellersRequired(
+                       num_output_channels(), num_reverse_channels()));
 
-    std::vector<float> template_queue_element(render_queue_element_max_size_);
+  // Reallocate the queues if the queue item sizes are too small to fit the
+  // data to put in the queues.
+  if (float_render_queue_element_max_size_ <
+      new_float_render_queue_element_max_size) {
+    float_render_queue_element_max_size_ =
+        new_float_render_queue_element_max_size;
 
-    render_signal_queue_.reset(
+    std::vector<float> template_queue_element(
+        float_render_queue_element_max_size_);
+
+    float_render_signal_queue_.reset(
         new SwapQueue<std::vector<float>, RenderQueueItemVerifier<float>>(
             kMaxNumFramesToBuffer, template_queue_element,
-            RenderQueueItemVerifier<float>(render_queue_element_max_size_)));
+            RenderQueueItemVerifier<float>(
+                float_render_queue_element_max_size_)));
 
-    render_queue_buffer_.resize(render_queue_element_max_size_);
-    capture_queue_buffer_.resize(render_queue_element_max_size_);
+    float_render_queue_buffer_.resize(float_render_queue_element_max_size_);
+    float_capture_queue_buffer_.resize(float_render_queue_element_max_size_);
   } else {
-    render_signal_queue_->Clear();
+    float_render_signal_queue_->Clear();
+  }
+
+  if (int16_render_queue_element_max_size_ <
+      new_int16_render_queue_element_max_size) {
+    int16_render_queue_element_max_size_ =
+        new_int16_render_queue_element_max_size;
+
+    std::vector<int16_t> template_queue_element(
+        int16_render_queue_element_max_size_);
+
+    int16_render_signal_queue_.reset(
+        new SwapQueue<std::vector<int16_t>, RenderQueueItemVerifier<int16_t>>(
+            kMaxNumFramesToBuffer, template_queue_element,
+            RenderQueueItemVerifier<int16_t>(
+                int16_render_queue_element_max_size_)));
+
+    int16_render_queue_buffer_.resize(int16_render_queue_element_max_size_);
+    int16_capture_queue_buffer_.resize(int16_render_queue_element_max_size_);
+  } else {
+    int16_render_signal_queue_->Clear();
   }
 }
 
 void AudioProcessingImpl::EmptyQueuedRenderAudio() {
   rtc::CritScope cs_capture(&crit_capture_);
-  while (render_signal_queue_->Remove(&capture_queue_buffer_)) {
+  while (float_render_signal_queue_->Remove(&float_capture_queue_buffer_)) {
     public_submodules_->echo_cancellation->ProcessRenderAudio(
-        capture_queue_buffer_);
+        float_capture_queue_buffer_);
+  }
+
+  while (int16_render_signal_queue_->Remove(&int16_capture_queue_buffer_)) {

[ivoc, 2016/10/25 08:57:00]

Shouldn't this be split into separate functions? This function is called when
the float buffer is full or when the int16 buffer is full. If the sizes of these
buffers are potentially different (and it looks from the code that they could
be), then the buffers can be full at different times. Shouldn't we have a
separate EmptyQueuedFloatRenderAudio() and EmptyQueuedInt16RenderAudio()?

[peah-webrtc, 2016/10/25 09:48:33]

Yes, that could make sense.
This method is, however, called in two situations
1) Before a capture call to ensure that all render data has been processed.
2) When the render buffer becomes full to ensure that there is space in the
render buffer for more data.

Bullet 1) is part of the standard operation and for that I think that it is not
necessary to split this method.

Bullet 2) is very costly as it forces the APM to run single-threadedly. This we
want to do as seldom as possible and therefore I think it is good to take the
opportunity to empty all the render buffers to avoid that happening soon again.

Finally, note that all the render buffers store kMaxNumFramesToBuffer so they
should have the same lengths all the time.

WDYT? Can we keep it as it is?

[ivoc, 2016/10/25 10:39:46]

Right, that does make sense, let's keep it like this.

+    public_submodules_->echo_control_mobile->ProcessRenderAudio(
+        int16_capture_queue_buffer_);
   }
 }
 
 int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
   TRACE_EVENT0("webrtc", "AudioProcessing::ProcessStream_AudioFrame");
   {
     // Acquire the capture lock in order to safely call the function
     // that retrieves the render side data. This function accesses apm
     // getters that need the capture lock held when being called.
     // The lock needs to be released as
     // public_submodules_->echo_control_mobile->is_enabled() aquires this lock
     // as well.
     rtc::CritScope cs_capture(&crit_capture_);
     EmptyQueuedRenderAudio();
-    public_submodules_->echo_control_mobile->ReadQueuedRenderData();
     public_submodules_->gain_control->ReadQueuedRenderData();
   }
 
   if (!frame) {
     return kNullPointerError;
   }
   // Must be a native rate.
   if (frame->sample_rate_hz_ != kSampleRate8kHz &&
       frame->sample_rate_hz_ != kSampleRate16kHz &&
       frame->sample_rate_hz_ != kSampleRate32kHz &&
@@ skipping 342 matching lines @@
   }
 
 #if WEBRTC_INTELLIGIBILITY_ENHANCER
   if (capture_nonlocked_.intelligibility_enabled) {
     public_submodules_->intelligibility_enhancer->ProcessRenderAudio(
         render_buffer);
   }
 #endif
 
   QueueRenderAudio(render_buffer);
-  RETURN_ON_ERR(public_submodules_->echo_control_mobile->ProcessRenderAudio(
-      render_buffer));
   if (!constants_.use_experimental_agc) {
     RETURN_ON_ERR(
         public_submodules_->gain_control->ProcessRenderAudio(render_buffer));
   }
 
   if (submodule_states_.RenderMultiBandProcessingActive() &&
       SampleRateSupportsMultiBand(
           formats_.render_processing_format.sample_rate_hz())) {
     render_buffer->MergeFrequencyBands();
   }
@@ skipping 447 matching lines @@
       capture_processing_format(kSampleRate16kHz),
       split_rate(kSampleRate16kHz) {}
 
 AudioProcessingImpl::ApmCaptureState::~ApmCaptureState() = default;
 
 AudioProcessingImpl::ApmRenderState::ApmRenderState() = default;
 
 AudioProcessingImpl::ApmRenderState::~ApmRenderState() = default;
 
 }  // namespace webrtc