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

webrtc/modules/audio_processing/aec/aec_core.c

Index: webrtc/modules/audio_processing/aec/aec_core.c
diff --git a/webrtc/modules/audio_processing/aec/aec_core.c b/webrtc/modules/audio_processing/aec/aec_core.c
index da5c26b68395eab344f7f5de650e3a394d478347..e1edbc6428178521d3861f533eda80e11dd19dae 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.c
+++ b/webrtc/modules/audio_processing/aec/aec_core.c
@@ -135,6 +135,9 @@ 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;
@@ -407,31 +410,13 @@ __inline static void StoreAsComplex(const float* data,
 
 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
@@ -1011,9 +996,12 @@ static void EchoSubtraction(
 
 
 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;
@@ -1040,6 +1028,22 @@ static void EchoSuppression(AecCore* aec,
 
   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;
@@ -1060,7 +1064,15 @@ static void EchoSuppression(AecCore* aec,
   // 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++) {
@@ -1399,10 +1411,7 @@ static void ProcessBlock(AecCore* aec) {
   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
@@ -1511,6 +1520,10 @@ AecCore* WebRtcAec_CreateAec() {
   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)) {