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

webrtc/modules/audio_processing/echo_cancellation_impl.h

Index: webrtc/modules/audio_processing/echo_cancellation_impl.h
diff --git a/webrtc/modules/audio_processing/echo_cancellation_impl.h b/webrtc/modules/audio_processing/echo_cancellation_impl.h
index 070dcabc5d6df0447dcaaa65dfe5c205031993ac..d17b248c12f3ec5e9de03c407a3d604dff338eb0 100644
--- a/webrtc/modules/audio_processing/echo_cancellation_impl.h
+++ b/webrtc/modules/audio_processing/echo_cancellation_impl.h
@@ -11,11 +11,34 @@
 #ifndef WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_CANCELLATION_IMPL_H_
 #define WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_CANCELLATION_IMPL_H_
 
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/common_audio/swap_queue.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/audio_processing/processing_component.h"
 
 namespace webrtc {
 
+namespace {
+// Functor to use when supplying a verifier function for the queue item
+// verifcation.
+class AecRenderQueueItemVerifier {
+ public:
+  explicit AecRenderQueueItemVerifier(size_t allowed_size_1,
+                                      size_t allowed_size_2)
+      : allowed_size_1_(allowed_size_1), allowed_size_2_(allowed_size_2) {}
+
+  bool operator()(const std::vector<float>& v) const {
+    return (((v.size() == allowed_size_1_) || (v.size() == allowed_size_2_)) &&
+            (v.capacity() >= std::max(allowed_size_1_, allowed_size_2_)));

[kwiberg-webrtc, 2015/10/27 10:43:07]

Why test size in addition to capacity? The latter is sufficient to ensure there
will be no reallocations.

[peah-webrtc, 2015/10/29 11:36:56]

Done.

+  }
+
+ private:
+  size_t allowed_size_1_;
+  size_t allowed_size_2_;
+};
+
+}  // namespace anonymous
+
 class AudioBuffer;
 class CriticalSectionWrapper;
 
@@ -42,7 +65,16 @@ class EchoCancellationImpl : public EchoCancellation,
   bool is_delay_agnostic_enabled() const;
   bool is_extended_filter_enabled() const;
 
+  // Reads render side data that has been queued on the render call.
+  void ReadQueuedRenderData();
+
  private:
+  static const size_t kAllowedValuesOfSamplesPerFrame1 = 80;
+  static const size_t kAllowedValuesOfSamplesPerFrame2 = 160;
+  // TODO(peah): Decrease this once we properly handle hugely unbalanced
+  // reverse and forward call numbers.
+  static const size_t kMaxNumFramesToBuffer = 100;
+
   // EchoCancellation implementation.
   int Enable(bool enable) override;
   int enable_drift_compensation(bool enable) override;
@@ -68,6 +100,8 @@ class EchoCancellationImpl : public EchoCancellation,
   int num_handles_required() const override;
   int GetHandleError(void* handle) const override;
 
+  void AllocateRenderQueue();
+
   const AudioProcessing* apm_;
   CriticalSectionWrapper* crit_;
   bool drift_compensation_enabled_;
@@ -79,6 +113,12 @@ class EchoCancellationImpl : public EchoCancellation,
   bool delay_logging_enabled_;
   bool extended_filter_enabled_;
   bool delay_agnostic_enabled_;
+
+  size_t render_queue_element_max_size_;
+  std::vector<float> render_queue_buffer_;
+  std::vector<float> capture_queue_buffer_;
+  rtc::scoped_ptr<SwapQueue<std::vector<float>, AecRenderQueueItemVerifier> >
+      render_signal_queue_;

[kwiberg-webrtc, 2015/10/27 10:43:07]

As of C++11, you can write >> instead of > >.

Also, why make an indirection with a scoped_ptr instead of holding the SwapQueue
directly?

[peah-webrtc, 2015/10/29 11:36:56]

The thinking was that since the queue needs to be reconstructed during runtime,
it is better to do it this way as it is cheaper to new it up dynamically, than
to create it and assign to a queue state variable.

Would that make sense, or is there another way to do this?

[kwiberg-webrtc, 2015/10/29 12:30:22]

Well... Why do you need to recreate it? Isn't this what the Clean
method is for?

[peah-webrtc, 2015/10/29 13:07:36]

I need to recreate it because the maximum item size has changed, and there is
currently no functionality to enforce a new container size on the queue. 

This cannot be known in advance since the number of channels are only known
during runtime and can change during reinitialization. Therefore queue
reallocation may be needed.

I realized that the reinitialization should be conditional though, so I added
that in the latest update.

[kwiberg-webrtc, 2015/10/29 15:04:50]

OK. So it isn't just a performance thing; you actually *need* the indirection,
since SwapQueue doesn't support assignment.

Then I'd argue that SwapQueue should support assignment. Move assignment should
even be more efficient than the indirection you have now.

[peah-webrtc, 2015/11/05 11:45:10]

According to discussions in another CL, I found the conclusion to be that until
move assignment is allowed, the current implementation simplifies the use of the
SwapQueue while having the same complexity as if a copy assignment/construction
is implemented.

Therefore I keep the current implementation. Please say if you don't agree!

 };
 
 }  // namespace webrtc