Lock scheme #5: Applied the render queueing to the agc

webrtc/modules/audio_processing/gain_control_impl.cc

Index: webrtc/modules/audio_processing/gain_control_impl.cc
diff --git a/webrtc/modules/audio_processing/gain_control_impl.cc b/webrtc/modules/audio_processing/gain_control_impl.cc
index 595596b5598260c04b891362a4849fcb93ddd705..4d84b2416ab6c3b6f8767f2cd4e58b3f315fd1cf 100644
--- a/webrtc/modules/audio_processing/gain_control_impl.cc
+++ b/webrtc/modules/audio_processing/gain_control_impl.cc
@@ -35,20 +35,26 @@ int16_t MapSetting(GainControl::Mode mode) {
 }
 }  // namespace
 
+const size_t GainControlImpl::kAllowedValuesOfSamplesPerFrame1;

[The Sun (google.com), 2015/11/17 09:34:13]

Why did you need to declare these in the .cc but not kMaxNumFramesToBuffer ?

[peah-webrtc, 2015/11/17 10:18:23]

(I think) This is needed because the constants are used in a call to a function
where the inputs are reference declared, therefore these needs to be declared in
the .cc. That is not the case for the kMaxNumFramesToBuffer constant.

I order to avoid this inconsistency, I now instead static_cast the constants
before applying them which apparently is a workaround for this issue:
  const size_t max_frame_size = std::max<size_t>(
      static_cast<size_t>(kAllowedValuesOfSamplesPerFrame1),
      static_cast<size_t>(kAllowedValuesOfSamplesPerFrame2));

Then I can remove these declarations.

Please let me know if you think it is ok!
Done.

+const size_t GainControlImpl::kAllowedValuesOfSamplesPerFrame2;
+
 GainControlImpl::GainControlImpl(const AudioProcessing* apm,
                                  CriticalSectionWrapper* crit)
-  : ProcessingComponent(),
-    apm_(apm),
-    crit_(crit),
-    mode_(kAdaptiveAnalog),
-    minimum_capture_level_(0),
-    maximum_capture_level_(255),
-    limiter_enabled_(true),
-    target_level_dbfs_(3),
-    compression_gain_db_(9),
-    analog_capture_level_(0),
-    was_analog_level_set_(false),
-    stream_is_saturated_(false) {}
+    : ProcessingComponent(),
+      apm_(apm),
+      crit_(crit),
+      mode_(kAdaptiveAnalog),
+      minimum_capture_level_(0),
+      maximum_capture_level_(255),
+      limiter_enabled_(true),
+      target_level_dbfs_(3),
+      compression_gain_db_(9),
+      analog_capture_level_(0),
+      was_analog_level_set_(false),
+      stream_is_saturated_(false),
+      render_queue_element_max_size_(0) {
+  AllocateRenderQueue();

[The Sun (google.com), 2015/11/17 09:34:13]

Isn't Initialize() always called for processing components? Why do you
specifically need to allocate the queue here? 

[peah-webrtc, 2015/11/17 10:18:23]

Good point!
Done.

+}
 
 GainControlImpl::~GainControlImpl() {}
 
@@ -59,21 +65,53 @@ int GainControlImpl::ProcessRenderAudio(AudioBuffer* audio) {
 
   assert(audio->num_frames_per_band() <= 160);
 
+  render_queue_buffer_.resize(0);
   for (int i = 0; i < num_handles(); i++) {
     Handle* my_handle = static_cast<Handle*>(handle(i));
-    int err = WebRtcAgc_AddFarend(
-        my_handle,
-        audio->mixed_low_pass_data(),
-        audio->num_frames_per_band());
+    int err =
+        WebRtcAgc_GetAddFarendError(my_handle, audio->num_frames_per_band());
 
-    if (err != apm_->kNoError) {
+    if (err != apm_->kNoError)
       return GetHandleError(my_handle);
-    }
+
+    // Buffer the samples in the render queue.
+    render_queue_buffer_.insert(
+        render_queue_buffer_.end(), audio->mixed_low_pass_data(),
+        (audio->mixed_low_pass_data() + audio->num_frames_per_band()));
+  }
+
+  // Insert the samples into the queue.
+  if (!render_signal_queue_->Insert(&render_queue_buffer_)) {
+    ReadQueuedRenderData();
+
+    // Retry the insert (should always work).
+    RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true);
   }
 
   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 AGC.
+void GainControlImpl::ReadQueuedRenderData() {
+  if (!is_component_enabled()) {
+    return;
+  }
+
+  while (render_signal_queue_->Remove(&capture_queue_buffer_)) {
+    int buffer_index = 0;
+    const int num_frames_per_band =
+        capture_queue_buffer_.size() / num_handles();
+    for (int i = 0; i < num_handles(); i++) {
+      Handle* my_handle = static_cast<Handle*>(handle(i));
+      WebRtcAgc_AddFarend(my_handle, &capture_queue_buffer_[buffer_index],
+                          num_frames_per_band);
+
+      buffer_index += num_frames_per_band;
+    }
+  }
+}
+
 int GainControlImpl::AnalyzeCaptureAudio(AudioBuffer* audio) {
   if (!is_component_enabled()) {
     return apm_->kNoError;
@@ -179,6 +217,12 @@ int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
 
 // TODO(ajm): ensure this is called under kAdaptiveAnalog.
 int GainControlImpl::set_stream_analog_level(int level) {
+  // TODO(peah): Verify that this is really needed to do the reading
+  // here as well as in ProcessStream. It works since these functions
+  // are called from the same thread, but it is not nice to do it in two
+  // places if not needed.
+  ReadQueuedRenderData();

[The Sun (google.com), 2015/11/17 09:34:13]

You may want to add a comment here as to why you're doing this. It seems to me
like a very strange side effect of a function that looks like a simple setter.

[peah-webrtc, 2015/11/17 10:18:23]

Removed it.
Done.

+
   CriticalSectionScoped crit_scoped(crit_);
   was_analog_level_set_ = true;
   if (level < minimum_capture_level_ || level > maximum_capture_level_) {
@@ -296,12 +340,36 @@ int GainControlImpl::Initialize() {
     return err;
   }
 
+  AllocateRenderQueue();
+
   const int n = num_handles();
   RTC_CHECK_GE(n, 0) << "Bad number of handles: " << n;
   capture_levels_.assign(n, analog_capture_level_);
   return apm_->kNoError;
 }
 
+void GainControlImpl::AllocateRenderQueue() {
+  const size_t max_frame_size = std::max<size_t>(

[The Sun (google.com), 2015/11/17 09:34:13]

Template type should be deduced by the compiler. No need to write <size_t>

[peah-webrtc, 2015/11/17 10:18:23]

Done.

+      kAllowedValuesOfSamplesPerFrame1, kAllowedValuesOfSamplesPerFrame2);
+
+  const size_t new_render_queue_element_max_size = std::max<size_t>(

[The Sun (google.com), 2015/11/17 09:34:12]

is this the queue size, or size of an element in the queue?

[peah-webrtc, 2015/11/17 10:18:23]

This is the size of an element in the queue. The queue size is specified by
kMaxNumFramesToBuffer

+      static_cast<size_t>(1), (max_frame_size * num_handles()));
+
+  if (new_render_queue_element_max_size > render_queue_element_max_size_) {
+    std::vector<int16_t> template_queue_element(render_queue_element_max_size_);

[The Sun (google.com), 2015/11/17 09:34:13]

you're using what looks like the old item size, and never set that to the new
calculated size. so you will always go this branch?

[peah-webrtc, 2015/11/17 10:18:23]

Fully true, and it worked since the verification broke due to this bug, which
caused all items to be reallocated until the queue item size was achieved. Good
find!

Done.

+
+    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_)));
+  } else {
+    render_signal_queue_->Clear();
+  }
+
+  render_queue_buffer_.resize(new_render_queue_element_max_size);
+  capture_queue_buffer_.resize(new_render_queue_element_max_size);
+}
+
 void* GainControlImpl::CreateHandle() const {
   return WebRtcAgc_Create();
 }