Lock scheme #4: Introduced the render sample queue for the aec and aecm

webrtc/modules/audio_processing/echo_cancellation_impl.cc

Index: webrtc/modules/audio_processing/echo_cancellation_impl.cc
diff --git a/webrtc/modules/audio_processing/echo_cancellation_impl.cc b/webrtc/modules/audio_processing/echo_cancellation_impl.cc
index bdcfa2ad075877d6c772b2b6492c781d151150b9..05f53f836e3b49112d1d742153a2fb6b42bcf174 100644
--- a/webrtc/modules/audio_processing/echo_cancellation_impl.cc
+++ b/webrtc/modules/audio_processing/echo_cancellation_impl.cc
@@ -68,7 +68,15 @@ EchoCancellationImpl::EchoCancellationImpl(const AudioProcessing* apm,
       stream_has_echo_(false),
       delay_logging_enabled_(false),
       extended_filter_enabled_(false),
-      delay_agnostic_enabled_(false) {
+      delay_agnostic_enabled_(false),
+      render_queue_buffer_(kMaxNumSamplesPerFrameToBuffer),
+      capture_queue_buffer_(kMaxNumSamplesPerFrameToBuffer) {
+  std::vector<float> template_queue_element(kMaxNumSamplesPerFrameToBuffer);
+
+  render_signal_queue_.reset(
+      new SwapQueue<std::vector<float>, &RenderQueueItemVerifier>(
+          (kMaxNumFramesToBuffer * kMaxNumChannelsPerFrameToBuffer),
+          template_queue_element));
 }
 
 EchoCancellationImpl::~EchoCancellationImpl() {}
@@ -88,15 +96,27 @@ int EchoCancellationImpl::ProcessRenderAudio(const AudioBuffer* audio) {
   for (int i = 0; i < apm_->num_output_channels(); i++) {
     for (int j = 0; j < audio->num_channels(); j++) {
       Handle* my_handle = static_cast<Handle*>(handle(handle_index));
-      err = WebRtcAec_BufferFarend(
-          my_handle,
-          audio->split_bands_const_f(j)[kBand0To8kHz],
+      // Retrieve any error code produced by the buffering of the farend
+      // signal
+      err = WebRtcAec_GetBufferFarendError(
+          my_handle, audio->split_bands_const_f(j)[kBand0To8kHz],
           audio->num_frames_per_band());
 
       if (err != apm_->kNoError) {
         return MapError(err);  // TODO(ajm): warning possible?
       }
 
+      // Buffer the samples in the render queue.
+      memcpy(&render_queue_buffer_[0],
+             audio->split_bands_const_f(j)[kBand0To8kHz],
+             (audio->num_frames_per_band() *
+              sizeof(*audio->split_bands_const_f(j)[kBand0To8kHz])));

[kwiberg-webrtc, 2015/10/17 07:50:07]

Maybe I just haven't stared long enough, but where do you ensure that you don't
fill render_queue_buffer_ with more data than it can hold?

std::vector::clear followed by std::vector::insert (the one that takes an start
and an end iterator) is a good (and safe!) way to copy an array of PODs into a
vector. But you'll probably also want to DCHECK that the capacity is sufficient
so that no reallocation will take place.

[peah-webrtc, 2015/10/26 09:03:48]

I rewrote this quite a lot. Could you please let me know what you think of the
new code!

+      render_queue_buffer_.resize(audio->num_frames_per_band());

[kwiberg-webrtc, 2015/10/17 07:50:07]

Oh, here. This is illegal if it ends up increasing the size, since that means
the previous statement wrote past the end of the vector.

[peah-webrtc, 2015/10/26 09:03:48]

Agree. It is now rewritten. Could you please recheck!

+      // TODO(peah): Refactor so that it is possible to check the
+      // return value of Insert. Currently, that is not possible
+      // due to the code design when the capture thread is never
+      // started.

[kwiberg-webrtc, 2015/10/17 07:50:07]

Yeah, I guess that has to be done before this CL can land? Otherwise we risk
dropping data.

[peah-webrtc, 2015/10/26 09:03:48]

This is also now redesigned. It should be fine now.

+      render_signal_queue_->Insert(&render_queue_buffer_);
       handle_index++;
     }
   }
@@ -104,6 +124,25 @@ int EchoCancellationImpl::ProcessRenderAudio(const AudioBuffer* audio) {
   return apm_->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 EchoCancellationImpl::ReadQueuedRenderData() {
+  if (!is_component_enabled()) {
+    return;
+  }
+
+  while (render_signal_queue_->Remove(&capture_queue_buffer_)) {
+    size_t handle_index = 0;
+    for (int i = 0; i < apm_->num_output_channels(); i++) {
+      for (int j = 0; j < apm_->num_reverse_channels(); j++) {
+        Handle* my_handle = static_cast<Handle*>(handle(handle_index));
+        (void)WebRtcAec_BufferFarend(my_handle, &capture_queue_buffer_[0],
+                                     capture_queue_buffer_.size());
+      }
+    }
+  }
+}
+
 int EchoCancellationImpl::ProcessCaptureAudio(AudioBuffer* audio) {
   if (!is_component_enabled()) {
     return apm_->kNoError;
@@ -328,6 +367,7 @@ struct AecCore* EchoCancellationImpl::aec_core() const {
 }
 
 int EchoCancellationImpl::Initialize() {
+  render_signal_queue_->Clear();
   int err = ProcessingComponent::Initialize();
   if (err != apm_->kNoError || !is_component_enabled()) {
     return err;
@@ -368,7 +408,6 @@ int EchoCancellationImpl::ConfigureHandle(void* handle) const {
   config.nlpMode = MapSetting(suppression_level_);
   config.skewMode = drift_compensation_enabled_;
   config.delay_logging = delay_logging_enabled_;
-
   WebRtcAec_enable_extended_filter(
       WebRtcAec_aec_core(static_cast<Handle*>(handle)),
       extended_filter_enabled_ ? 1 : 0);
@@ -387,4 +426,11 @@ int EchoCancellationImpl::GetHandleError(void* handle) const {
   assert(handle != NULL);
   return AudioProcessing::kUnspecifiedError;
 }
+
+bool EchoCancellationImpl::RenderQueueItemVerifier(
+    const std::vector<float>& v) {
+  return ((v.size() == kMinNumSamplesPerFrameToBuffer) ||
+          (v.size() == kMaxNumSamplesPerFrameToBuffer));

[kwiberg-webrtc, 2015/10/17 07:50:07]

From the names of these constants, I would've expected any value in [min, max]
to be allowed.

Also, I don't see how verifying this property will help ensure there are no
run-time reallocations. What if the vector to be filled has the smaller size,
but we want to fill it with the larger number of elements?

[peah-webrtc, 2015/10/26 09:03:48]

I totally agree. 
Regarding the names I renamed them.

Regarding the run-time reallocations avoidance, with the new functor verifier
this is now properly verified. Could you please recheck!

+}
+
 }  // namespace webrtc