audio_processing/aec: make delay estimator aware of starving farend buffer

webrtc/modules/audio_processing/aec/aec_core.c

 /*
  *  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 839 matching lines @@
   freq_data[1][0] = 0;
   freq_data[1][PART_LEN] = 0;
   freq_data[0][0] = time_data[0];
   freq_data[0][PART_LEN] = time_data[1];
   for (i = 1; i < PART_LEN; i++) {
     freq_data[0][i] = time_data[2 * i];
     freq_data[1][i] = time_data[2 * i + 1];
   }
 }
 
+static int MoveFarReadPtrWithoutSystemDelayUpdate(AecCore* self, int elements) {
+  WebRtc_MoveReadPtr(self->far_buf_windowed, elements);

[hlundin-webrtc, 2015/06/16 20:14:38]

If I understand it correctly, WebRtc_MoveReadPtr returns the number of elements
the pointer was moved, which may differ from the input |elements|. Is there any
point in verifying that all returned the same value?

[bjornv1, 2015/06/16 20:30:05]

Yes, you're right, though all these buffers are constructed with the same size,
hence if one of them "breaks", they all do. We could add asserts for sure to
catch such an event. I'll commit without assert and add it in a second CL if you
insist.

[hlundin-webrtc, 2015/06/16 20:32:30]

I think it is fine the way it is. Don't bother with the asserts.

+#ifdef WEBRTC_AEC_DEBUG_DUMP
+  WebRtc_MoveReadPtr(self->far_time_buf, elements);
+#endif
+  return WebRtc_MoveReadPtr(self->far_buf, elements);
+}
+
 static int SignalBasedDelayCorrection(AecCore* self) {
   int delay_correction = 0;
   int last_delay = -2;
   assert(self != NULL);
 #if !defined(WEBRTC_ANDROID)
   // On desktops, turn on correction after |kDelayCorrectionStart| frames.  This
   // is to let the delay estimation get a chance to converge.  Also, if the
   // playout audio volume is low (or even muted) the delay estimation can return
   // a very large delay, which will break the AEC if it is applied.
   if (self->frame_count < kDelayCorrectionStart) {
@@ skipping 20 matching lines @@
     int delay = last_delay - WebRtc_lookahead(self->delay_estimator);
     // Allow for a slack in the actual delay, defined by a |lower_bound| and an
     // |upper_bound|.  The adaptive echo cancellation filter is currently
     // |num_partitions| (of 64 samples) long.  If the delay estimate is negative
     // or at least 3/4 of the filter length we open up for correction.
     const int lower_bound = 0;
     const int upper_bound = self->num_partitions * 3 / 4;
     const int do_correction = delay <= lower_bound || delay > upper_bound;
     if (do_correction == 1) {
       int available_read = (int)WebRtc_available_read(self->far_buf);
-      // Adjust w.r.t. a |shift_offset| to account for not as reliable estimates
-      // in the beginning, hence we are more conservative.
-      delay_correction = -(delay - self->shift_offset);
+      // With |shift_offset| we gradually rely on the delay estimates.  For
+      // positive delays we reduce the correction by |shift_offset| to lower the
+      // risk of pushing the AEC into a non causal state.  For negative delays
+      // we rely on the values up to a rounding error, hence compensate by 1
+      // element to make sure to push the delay into the causal region.
+      delay_correction = -delay;
+      delay_correction += delay > self->shift_offset ? self->shift_offset : 1;
       self->shift_offset--;
       self->shift_offset = (self->shift_offset <= 1 ? 1 : self->shift_offset);
       if (delay_correction > available_read - self->mult - 1) {
         // There is not enough data in the buffer to perform this shift.  Hence,
         // we do not rely on the delay estimate and do nothing.
         delay_correction = 0;
       } else {
         self->previous_delay = last_delay;
         ++self->delay_correction_count;
       }
@@ skipping 795 matching lines @@
   TimeToFrequency(fft, xf, 0);
   WebRtc_WriteBuffer(aec->far_buf, &xf[0][0], 1);
 
   // Convert far-end partition to the frequency domain with windowing.
   memcpy(fft, farend, sizeof(float) * PART_LEN2);
   TimeToFrequency(fft, xf, 1);
   WebRtc_WriteBuffer(aec->far_buf_windowed, &xf[0][0], 1);
 }
 
 int WebRtcAec_MoveFarReadPtr(AecCore* aec, int elements) {
-  int elements_moved = WebRtc_MoveReadPtr(aec->far_buf_windowed, elements);
-  WebRtc_MoveReadPtr(aec->far_buf, elements);
-#ifdef WEBRTC_AEC_DEBUG_DUMP
-  WebRtc_MoveReadPtr(aec->far_time_buf, elements);
-#endif
+  int elements_moved = MoveFarReadPtrWithoutSystemDelayUpdate(aec, elements);
   aec->system_delay -= elements_moved * PART_LEN;
   return elements_moved;
 }
 
 void WebRtcAec_ProcessFrames(AecCore* aec,
                              const float* const* nearend,
                              int num_bands,
                              int num_samples,
                              int knownDelay,
                              float* const* out) {
@@ skipping 52 matching lines @@
       // 2 a) Compensate for a possible change in the system delay.
 
       // TODO(bjornv): Investigate how we should round the delay difference;
       // right now we know that incoming |knownDelay| is underestimated when
       // it's less than |aec->knownDelay|. We therefore, round (-32) in that
       // direction. In the other direction, we don't have this situation, but
       // might flush one partition too little. This can cause non-causality,
       // which should be investigated. Maybe, allow for a non-symmetric
       // rounding, like -16.
       int move_elements = (aec->knownDelay - knownDelay - 32) / PART_LEN;
-      int moved_elements = WebRtc_MoveReadPtr(aec->far_buf, move_elements);
-      WebRtc_MoveReadPtr(aec->far_buf_windowed, move_elements);
+      int moved_elements =
+          MoveFarReadPtrWithoutSystemDelayUpdate(aec, move_elements);
       aec->knownDelay -= moved_elements * PART_LEN;
-  #ifdef WEBRTC_AEC_DEBUG_DUMP
-      WebRtc_MoveReadPtr(aec->far_time_buf, move_elements);
-  #endif
     } else {
       // 2 b) Apply signal based delay correction.
       int move_elements = SignalBasedDelayCorrection(aec);
-      int moved_elements = WebRtc_MoveReadPtr(aec->far_buf, move_elements);
-      WebRtc_MoveReadPtr(aec->far_buf_windowed, move_elements);
-  #ifdef WEBRTC_AEC_DEBUG_DUMP
-      WebRtc_MoveReadPtr(aec->far_time_buf, move_elements);
-  #endif
+      int moved_elements =
+          MoveFarReadPtrWithoutSystemDelayUpdate(aec, move_elements);
+      int far_near_buffer_diff = WebRtc_available_read(aec->far_buf) -
+          WebRtc_available_read(aec->nearFrBuf) / PART_LEN;
       WebRtc_SoftResetDelayEstimator(aec->delay_estimator, moved_elements);
       WebRtc_SoftResetDelayEstimatorFarend(aec->delay_estimator_farend,
                                            moved_elements);
       aec->signal_delay_correction += moved_elements;
-      // TODO(bjornv): Investigate if this is reasonable.  I had to add this
-      // guard when the signal based delay correction replaces the system based
-      // one. Otherwise there was a buffer underrun in the "qa-new/01/"
-      // recording when adding 44 ms extra delay.  This was not seen if we kept
-      // both delay correction algorithms running in parallel.
-      // A first investigation showed that we have a drift in this case that
-      // causes the buffer underrun.  Compared to when delay correction was
-      // turned off, we get buffer underrun as well which was triggered in 1)
-      // above.  In addition there was a shift in |knownDelay| later increasing
-      // the buffer.  When running in parallel, this if statement was not
-      // triggered.  This suggests two alternatives; (a) use both algorithms, or
-      // (b) allow for smaller delay corrections when we operate close to the
-      // buffer limit.  At the time of testing we required a change of 6 blocks,
-      // but could change it to, e.g., 2 blocks. It requires some testing
-      // though.
-      if ((int)WebRtc_available_read(aec->far_buf) < (aec->mult + 1)) {
-        // We don't have enough data so we stuff the far-end buffers.
-        WebRtcAec_MoveFarReadPtr(aec, -(aec->mult + 1));
+      // If we rely on reported system delay values only a buffer underrun here
+      // can never occur, but since we apply signal based delay correction we
+      // can since the delay estimation can be wrong.  We therefore stuff the

[hlundin-webrtc, 2015/06/16 20:14:38]

Missing word? "... we can [what?]"

[bjornv1, 2015/06/16 20:30:05]

Done.

+      // buffer with enough elements if needed.
+      if (far_near_buffer_diff < 0) {
+        WebRtcAec_MoveFarReadPtr(aec, far_near_buffer_diff);
       }
     }
 
     // 4) Process as many blocks as possible.
     while (WebRtc_available_read(aec->nearFrBuf) >= PART_LEN) {
       ProcessBlock(aec);
     }
 
     // 5) Update system delay with respect to the entire frame.
     aec->system_delay -= FRAME_LEN;
@@ skipping 93 matching lines @@
 int WebRtcAec_extended_filter_enabled(AecCore* self) {
   return self->extended_filter_enabled;
 }
 
 int WebRtcAec_system_delay(AecCore* self) { return self->system_delay; }
 
 void WebRtcAec_SetSystemDelay(AecCore* self, int delay) {
   assert(delay >= 0);
   self->system_delay = delay;
 }