Refactoring (bitexact) of the EchoSuppressor in WebRTC AEC (#1)

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 117 matching lines @@
 #ifdef WEBRTC_AEC_DEBUG_DUMP
 extern int webrtc_aec_instance_count;
 #endif
 
 WebRtcAecFilterFar WebRtcAec_FilterFar;
 WebRtcAecScaleErrorSignal WebRtcAec_ScaleErrorSignal;
 WebRtcAecFilterAdaptation WebRtcAec_FilterAdaptation;
 WebRtcAecOverdriveAndSuppress WebRtcAec_OverdriveAndSuppress;
 WebRtcAecComfortNoise WebRtcAec_ComfortNoise;
 WebRtcAecSubBandCoherence WebRtcAec_SubbandCoherence;
+WebRtcAecStoreAsComplex WebRtcAec_StoreAsComplex;
+WebRtcAecPartitionDelay WebRtcAec_PartitionDelay;
+WebRtcAecWindowData WebRtcAec_WindowData;
 
 __inline static float MulRe(float aRe, float aIm, float bRe, float bIm) {
   return aRe * bRe - aIm * bIm;
 }
 
 __inline static float MulIm(float aRe, float aIm, float bRe, float bIm) {
   return aRe * bIm + aIm * bRe;
 }
 
 static int CmpFloat(const void* a, const void* b) {
@@ skipping 252 matching lines @@
   for (i = 1; i < PART_LEN; i++) {
     data_complex[0][i] = data[2 * i];
     data_complex[1][i] = data[2 * i + 1];
   }
   data_complex[0][PART_LEN] = data[1];
   data_complex[1][PART_LEN] = 0;
 }
 
 static void SubbandCoherence(AecCore* aec,
                              float efw[2][PART_LEN1],
+                             float dfw[2][PART_LEN1],
                              float xfw[2][PART_LEN1],
                              float* fft,
                              float* cohde,
                              float* cohxd) {
-  float dfw[2][PART_LEN1];
   int i;
 
-  if (aec->delayEstCtr == 0)
-    aec->delayIdx = PartitionDelay(aec);
-
-  // Use delayed far.
-  memcpy(xfw,
-         aec->xfwBuf + aec->delayIdx * PART_LEN1,
-         sizeof(xfw[0][0]) * 2 * PART_LEN1);
-
-  // Windowed near fft
-  WindowData(fft, aec->dBuf);
-  aec_rdft_forward_128(fft);
-  StoreAsComplex(fft, dfw);
-
-  // Windowed error fft
-  WindowData(fft, aec->eBuf);
-  aec_rdft_forward_128(fft);
-  StoreAsComplex(fft, efw);
-
   SmoothedPSD(aec, efw, dfw, xfw);
 
   // Subband coherence
   for (i = 0; i < PART_LEN1; i++) {
     cohde[i] =
         (aec->sde[i][0] * aec->sde[i][0] + aec->sde[i][1] * aec->sde[i][1]) /
         (aec->sd[i] * aec->se[i] + 1e-10f);
     cohxd[i] =
         (aec->sxd[i][0] * aec->sxd[i][0] + aec->sxd[i][1] * aec->sxd[i][1]) /
         (aec->sx[i] * aec->sd[i] + 1e-10f);
@@ skipping 559 matching lines @@
   WebRtcAec_FilterAdaptation(num_partitions,
                              x_fft_buf_block_pos,
                              x_fft_buf,
                              e_fft,
                              h_fft_buf);
   memcpy(echo_subtractor_output, e, sizeof(float) * PART_LEN);
 }
 
 
 static void EchoSuppression(AecCore* aec,
+                            float* echo_subtractor_output,
                             float* output,
                             float* const* outputH) {
-  float efw[2][PART_LEN1], xfw[2][PART_LEN1];
+  float efw[2][PART_LEN1];
+  float xfw[2][PART_LEN1];
+  float dfw[2][PART_LEN1];
   complex_t comfortNoiseHband[PART_LEN1];
   float fft[PART_LEN2];
   float scale, dtmp;
   float nlpGainHband;
   int i;
   size_t j;
 
   // Coherence and non-linear filter
   float cohde[PART_LEN1], cohxd[PART_LEN1];
   float hNlDeAvg, hNlXdAvg;
   float hNl[PART_LEN1];
   float hNlPref[kPrefBandSize];
   float hNlFb = 0, hNlFbLow = 0;
   const float prefBandQuant = 0.75f, prefBandQuantLow = 0.5f;
   const int prefBandSize = kPrefBandSize / aec->mult;
   const int minPrefBand = 4 / aec->mult;
   // Power estimate smoothing coefficients.
   const float* min_overdrive = aec->extended_filter_enabled
                                    ? kExtendedMinOverDrive
                                    : kNormalMinOverDrive;
 
   // Filter energy
   const int delayEstInterval = 10 * aec->mult;
 
   float* xfw_ptr = NULL;
 
+  // Update eBuf with echo subtractor output.

[hlundin-webrtc, 2015/12/04 09:56:58]

Is there any reason you moved this block earlier than it was before? The natural
position seems to me to be just before calling WebRtcAec_SubbandCoherence below.

[peah-webrtc, 2015/12/04 14:14:49]

I'm a bit confused, afaics it has been moved later. It was before called before
EchoSuppression(), right? And since it is only used in echosuppression() I think
it should be updated there only.

[hlundin-webrtc, 2015/12/04 15:04:04]

What I meant was this: What used to be one block of code (lines 417-433 in
SubbandCoherence) is now split into two blocks; this block and the one at line
1067 below. The order of these two subblocks of code has changed compared with
the original.

[peah-webrtc, 2015/12/04 22:37:52]

Ah, then I see :-) Sorry.


Firstly, note that the initialization of the comfort noise will be moved in an
upcoming CL as it is placed far away from where it is actually used. When moving
that, what remains before the call to SubbandCoherence all are preparations of
the inputs to that function. Also, the lines 1047-1050 will be bundled with the
1067-1068. And the updates of the buffers eBuf and xfwBuf will be moved as well.

I like to think of the echo suppressor as something that operates fully in the
filterbank domain, and then it feels natural for me to as far as possible have
it as an analysis filterbank followed by processing and then a synthesis
filterbank. This is one of the main operations in the EchoSuppressor and since
it can be arbitrarily placed among the input preparations to the
SubbandCoherence function I think it makes sense to put it in the beginning.

Does it make sense?

[hlundin-webrtc, 2015/12/08 11:48:52]

That makes sense. Thanks!

+  memcpy(aec->eBuf + PART_LEN,
+         echo_subtractor_output,
+         sizeof(float) * PART_LEN);
+
+  // Analysis filter banks for the echo suppressor.
+  // Windowed near-end ffts.
+  WindowData(fft, aec->dBuf);
+  aec_rdft_forward_128(fft);
+  WebRtcAec_StoreAsComplex(fft, dfw);

[hlundin-webrtc, 2015/12/04 09:56:58]

Why did this function change to WebRtcAec_*, but not the one below?

[peah-webrtc, 2015/12/04 14:14:49]

Good find! Initially neither had the WebRtcAec_ prefix (which means it has an
optimized counterpart). I assume there was a reason for that (maybe the
optimized counterpart is broken) so I'll change both to use the local version
(as I cannot verify the correctness of the optimized code).

Does that sound ok?

[hlundin-webrtc, 2015/12/04 15:04:04]

Acknowledged.

+
+  // Windowed echo suppressor output ffts.
+  WindowData(fft, aec->eBuf);
+  aec_rdft_forward_128(fft);
+  StoreAsComplex(fft, efw);
+
   aec->delayEstCtr++;
   if (aec->delayEstCtr == delayEstInterval) {
     aec->delayEstCtr = 0;
   }
 
   // initialize comfort noise for H band
   memset(comfortNoiseHband, 0, sizeof(comfortNoiseHband));
   nlpGainHband = (float)0.0;
   dtmp = (float)0.0;
 
   // We should always have at least one element stored in |far_buf|.
   assert(WebRtc_available_read(aec->far_buf_windowed) > 0);
   // NLP
   WebRtc_ReadBuffer(aec->far_buf_windowed, (void**)&xfw_ptr, &xfw[0][0], 1);
 
   // TODO(bjornv): Investigate if we can reuse |far_buf_windowed| instead of
   // |xfwBuf|.
   // Buffer far.
   memcpy(aec->xfwBuf, xfw_ptr, sizeof(float) * 2 * PART_LEN1);
 
-  WebRtcAec_SubbandCoherence(aec, efw, xfw, fft, cohde, cohxd);
+  if (aec->delayEstCtr == 0)
+    aec->delayIdx = WebRtcAec_PartitionDelay(aec);
+
+  // Use delayed far.
+  memcpy(xfw,
+         aec->xfwBuf + aec->delayIdx * PART_LEN1,
+         sizeof(xfw[0][0]) * 2 * PART_LEN1);
+
+  WebRtcAec_SubbandCoherence(aec, efw, dfw, xfw, fft, cohde, cohxd);
 
   hNlXdAvg = 0;
   for (i = minPrefBand; i < prefBandSize + minPrefBand; i++) {
     hNlXdAvg += cohxd[i];
   }
   hNlXdAvg /= prefBandSize;
   hNlXdAvg = 1 - hNlXdAvg;
 
   hNlDeAvg = 0;
   for (i = minPrefBand; i < prefBandSize + minPrefBand; i++) {
@@ skipping 318 matching lines @@
                   aec->xfBuf,
                   nearend_ptr,
                   aec->xPow,
                   aec->wfBuf,
                   &aec->linoutlevel,
                   echo_subtractor_output);
 
   RTC_AEC_DEBUG_WAV_WRITE(aec->outLinearFile, echo_subtractor_output, PART_LEN);
 
   // Perform echo suppression.
-  memcpy(aec->eBuf + PART_LEN,

[minyue-webrtc, 2015/12/03 12:42:51]

This is ok, but it makes the treatment of eBuf different from dBuf (and other)

                Filling-in new data   |   Updating history
______________________________________________
eBuf    |       EchoSuppression       |  EchoSuppression
______________________________________________
dBuf    |       ProcessBlock          |  EchoSuppression

The left-down corner (ProcessBlock) is not movable. I personally think it would
make better sense to move all operations to EchoSuppression because
1. more consistent and clear,
2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
3. We may likely use eBuf/dBuf in ProcessBlock, at some point.

[peah-webrtc, 2015/12/03 15:00:36]

I guess that what you are saying is that all operations should move from
EchoSuppression to ProcessBlock, right (and not the other way around).

1) For eBuf I think it is more clear to do the actual buffering close to where
it is used. We anyway cannot do the filling in of new data in the same place as
where that happens for dBuf so I don't think we can achieve any consistency gain
by moving it to ProcessBlock. 

2) For dBuf I agree that the updating of the history should be moved to
ProcessBlock.
But for eBuf, it is currently not used by anything other than EchoSuppression
and therefore I think it should belong there. 

3) If we need it for some future use we could refactor that then, but currently
I don't see any need for it and if you don't see any immediate place where to
use it, I don't think we should plan for that.

-         echo_subtractor_output,
-         sizeof(float) * PART_LEN);
-  EchoSuppression(aec, output, outputH_ptr);
+  EchoSuppression(aec, echo_subtractor_output, output, outputH_ptr);
 
   if (aec->metricsMode == 1) {
     // Update power levels and echo metrics
     UpdateLevel(&aec->farlevel, (float(*)[PART_LEN1])xf_ptr);
     UpdateLevel(&aec->nearlevel, df);
     UpdateMetrics(aec);
   }
 
   // Store the output block.
   WebRtc_WriteBuffer(aec->outFrBuf, output, PART_LEN);
@@ skipping 88 matching lines @@
 #endif
   aec->extended_filter_enabled = 0;
 
   // Assembly optimization
   WebRtcAec_FilterFar = FilterFar;
   WebRtcAec_ScaleErrorSignal = ScaleErrorSignal;
   WebRtcAec_FilterAdaptation = FilterAdaptation;
   WebRtcAec_OverdriveAndSuppress = OverdriveAndSuppress;
   WebRtcAec_ComfortNoise = ComfortNoise;
   WebRtcAec_SubbandCoherence = SubbandCoherence;
+  WebRtcAec_StoreAsComplex = StoreAsComplex;
+  WebRtcAec_PartitionDelay = PartitionDelay;
+  WebRtcAec_WindowData = WindowData;
+
 
 #if defined(WEBRTC_ARCH_X86_FAMILY)
   if (WebRtc_GetCPUInfo(kSSE2)) {
     WebRtcAec_InitAec_SSE2();
   }
 #endif
 
 #if defined(MIPS_FPU_LE)
   WebRtcAec_InitAec_mips();
 #endif
@@ skipping 430 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;
 }