Ducking fix #1:Initial refactoring preparing for further AEC work (changes are bitexact).

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 157 matching lines @@
                         aec->wfBuf[0][pos + j],
                         aec->wfBuf[1][pos + j]);
       yf[1][j] += MulIm(aec->xfBuf[0][xPos + j],
                         aec->xfBuf[1][xPos + j],
                         aec->wfBuf[0][pos + j],
                         aec->wfBuf[1][pos + j]);
     }
   }
 }
 
-static void ScaleErrorSignal(AecCore* aec, float ef[2][PART_LEN1]) {
-  const float mu = aec->extended_filter_enabled ? kExtendedMu : aec->normal_mu;
-  const float error_threshold = aec->extended_filter_enabled
+static void ScaleErrorSignal(int extended_filter_enabled,
+                             float normal_mu,
+                             float normal_error_threshold,
+                             float *xPow,

[hlundin-webrtc, 2015/11/20 10:55:40]

Rename xPow -> x_pow while you are at it.

[ivoc, 2015/11/20 10:58:13]

I know the style guide doesn't cover C-code, but does it makes sense to use
const-ness to indicate input parameters when possible? This one could be float
const * const, I think. Also applies to a lot of other input arguments
elsewhere.

[peah-webrtc, 2015/11/23 21:39:07]

Done.

[peah-webrtc, 2015/11/23 21:39:07]

I got the advice from kwiberg (I think) not to add const specifiers to arguments
that are passed by value as those clutter the function interface for the user of
the function, without adding any value to the user of the function.

For pointers it is different and I totally agree that it would make sense to add
const specifiers to this one, but in this case I prefer to leave the variable as
it is since it is used by optimized code for NEON/MIPS/SSE2 and I don't know how
it would affect that one.
But it could be that I'm too cautious with that.

What do you think, does this make sense?

What do you think, us 

+                             float ef[2][PART_LEN1]) {
+  const float mu = extended_filter_enabled ? kExtendedMu : normal_mu;
+  const float error_threshold = extended_filter_enabled
                                     ? kExtendedErrorThreshold
-                                    : aec->normal_error_threshold;
+                                    : normal_error_threshold;
   int i;
   float abs_ef;
   for (i = 0; i < (PART_LEN1); i++) {
-    ef[0][i] /= (aec->xPow[i] + 1e-10f);
-    ef[1][i] /= (aec->xPow[i] + 1e-10f);
+    ef[0][i] /= (xPow[i] + 1e-10f);
+    ef[1][i] /= (xPow[i] + 1e-10f);
     abs_ef = sqrtf(ef[0][i] * ef[0][i] + ef[1][i] * ef[1][i]);
 
     if (abs_ef > error_threshold) {
       abs_ef = error_threshold / (abs_ef + 1e-10f);
       ef[0][i] *= abs_ef;
       ef[1][i] *= abs_ef;
     }
 
     // Stepsize factor
     ef[0][i] *= mu;
@@ skipping 632 matching lines @@
         self->num_delay_values;
   }
 
   // Reset histogram.
   memset(self->delay_histogram, 0, sizeof(self->delay_histogram));
   self->num_delay_values = 0;
 
   return;
 }
 
+static void FrequencyToTime(float freq_data[2][PART_LEN1],
+                            float time_data[PART_LEN2]) {
+  time_data[0] = freq_data[0][0];
+  time_data[1] = freq_data[0][PART_LEN];

[ivoc, 2015/11/20 10:58:13]

Is it me, or are freq_data[1][0] and freq_data[1][PART_LEN] not used at all? Or
is that because the time-domain signal is real-valued?

[peah-webrtc, 2015/11/23 21:39:07]

You are correct, and I think that that format is used in the aec to simplify the
code. It seems like the aec_rdft_inverse function uses the optimized storage
variant for real-valued fft data.
I think the wasted two floats in the array are acceptable if the code gets
easier to read, but long-term I think it could make sense to store the fft data
as an interlieved array so that we don't have to bother with multi-dimensional
arrays. 

+  for (int i = 1; i < PART_LEN; i++) {
+    time_data[2 * i] = freq_data[0][i];
+    time_data[2 * i + 1] = freq_data[1][i];
+  }
+  aec_rdft_inverse_128(time_data);
+}
+
+
 static void TimeToFrequency(float time_data[PART_LEN2],
                             float freq_data[2][PART_LEN1],
                             int window) {
   int i = 0;
 
   // TODO(bjornv): Should we have a different function/wrapper for windowed FFT?
   if (window) {
     for (i = 0; i < PART_LEN; i++) {
       time_data[i] *= WebRtcAec_sqrtHanning[i];
       time_data[PART_LEN + i] *= WebRtcAec_sqrtHanning[PART_LEN - i];
@@ skipping 85 matching lines @@
     float delay_quality = WebRtc_last_delay_quality(self->delay_estimator);
     delay_quality = (delay_quality > kDelayQualityThresholdMax ?
         kDelayQualityThresholdMax : delay_quality);
     self->delay_quality_threshold =
         (delay_quality > self->delay_quality_threshold ? delay_quality :
             self->delay_quality_threshold);
   }
   return delay_correction;
 }
 
-static void NonLinearProcessing(AecCore* aec,
-                                float* output,
-                                float* const* outputH) {
+static void EchoSubtraction(AecCore* aec,
+                            float* nearend_ptr) {
+  float yf[2][PART_LEN1];
+  float fft[PART_LEN2];
+  float y[PART_LEN];
+  float e[PART_LEN];
+  float ef[2][PART_LEN1];
+  float scale;
+  int i;
+  memset(yf, 0, sizeof(yf));
+
+  // Produce echo estimate.

[hlundin-webrtc, 2015/11/20 10:55:40]

... in frequency domain.

[peah-webrtc, 2015/11/23 21:39:06]

Done.

+  WebRtcAec_FilterFar(aec, yf);
+
+  // Inverse fft to obtain echo estimate and error.
+  FrequencyToTime(yf, fft);
+
+  // Extract the output signal and compute the time-domain error

[hlundin-webrtc, 2015/11/20 10:55:40]

.

[peah-webrtc, 2015/11/23 21:39:07]

Done.

+  scale = 2.0f / PART_LEN2;
+  for (i = 0; i < PART_LEN; i++) {
+    y[i] = fft[PART_LEN + i] * scale;  // fft scaling

[ivoc, 2015/11/20 10:58:13]

Would it make sense to integrate this into the FrequencyToTime function (with
another parameter for the scaling factor)?

[peah-webrtc, 2015/11/23 21:39:07]

I don't think we should, as that loses the symmetry with the TimeToFrequency
function. Furthermore, in one of the upcoming CLs I have renamed these functions
to Ifft and Fft which is really what they are (or at least wrappers of
real-valued such). It could make sense to include the 1/PART_LEN2 term but I
think that would add complexity as it is not necessary to use in all places.

In my mind, this scaling is more related to the block scheme, which requires
this particular scaling a factor 2/PART_LEN2.

Does that make sense?

+  }
+
+  for (i = 0; i < PART_LEN; i++) {
+    e[i] = nearend_ptr[i] - y[i];

[ivoc, 2015/11/20 10:58:13]

Could be integrated with the previous loop, for efficiency.

[peah-webrtc, 2015/11/23 21:39:07]

Agree, will do.

Done.

+  }
+
+

[ivoc, 2015/11/20 10:58:14]

2 empty lines is a bit much here (in my opinion).

[peah-webrtc, 2015/11/23 21:39:07]

Done.

+  // Error fft
+  memcpy(aec->eBuf + PART_LEN, e, sizeof(float) * PART_LEN);
+  memset(fft, 0, sizeof(float) * PART_LEN);
+  memcpy(fft + PART_LEN, e, sizeof(float) * PART_LEN);
+  TimeToFrequency(fft, ef, 0);
+
+  RTC_AEC_DEBUG_RAW_WRITE(aec->e_fft_file,
+                          &ef[0][0],
+                          sizeof(ef[0][0]) * PART_LEN1 * 2);
+
+  if (aec->metricsMode == 1) {
+    // Note that the first PART_LEN samples in fft (before transformation) are
+    // zero. Hence, the scaling by two in UpdateLevel() should not be
+    // performed. That scaling is taken care of in UpdateMetrics() instead.
+    UpdateLevel(&aec->linoutlevel, ef);
+  }
+
+  // Scale error signal inversely with far power.
+  WebRtcAec_ScaleErrorSignal(aec->extended_filter_enabled,
+                             aec->normal_mu,
+                             aec->normal_error_threshold,
+                             aec->xPow,
+                             ef);
+  WebRtcAec_FilterAdaptation(aec, fft, ef);
+
+

[ivoc, 2015/11/20 10:58:13]

2 empty lines is a bit much here (in my opinion).

[peah-webrtc, 2015/11/23 21:39:07]

Done.

+  RTC_AEC_DEBUG_WAV_WRITE(aec->outLinearFile, e, PART_LEN);
+}
+
+
+static void EchoSuppression(AecCore* aec,
+                            float* output,
+                            float* const* outputH) {
   float efw[2][PART_LEN1], xfw[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];
@@ skipping 212 matching lines @@
     memcpy(aec->dBufH[j], aec->dBufH[j] + PART_LEN, sizeof(float) * PART_LEN);
   }
 
   memmove(aec->xfwBuf + PART_LEN1,
           aec->xfwBuf,
           sizeof(aec->xfwBuf) - sizeof(complex_t) * PART_LEN1);
 }
 
 static void ProcessBlock(AecCore* aec) {
   size_t i;
-  float y[PART_LEN], e[PART_LEN];
-  float scale;
 
   float fft[PART_LEN2];
-  float xf[2][PART_LEN1], yf[2][PART_LEN1], ef[2][PART_LEN1];
+  float xf[2][PART_LEN1];
   float df[2][PART_LEN1];
   float far_spectrum = 0.0f;
   float near_spectrum = 0.0f;
   float abs_far_spectrum[PART_LEN1];
   float abs_near_spectrum[PART_LEN1];
 
   const float gPow[2] = {0.9f, 0.1f};
 
   // Noise estimate constants.
   const int noiseInitBlocks = 500 * aec->mult;
   const float step = 0.1f;
   const float ramp = 1.0002f;
   const float gInitNoise[2] = {0.999f, 0.001f};
 
   float nearend[PART_LEN];
   float* nearend_ptr = NULL;
   float output[PART_LEN];
   float outputH[NUM_HIGH_BANDS_MAX][PART_LEN];
   float* outputH_ptr[NUM_HIGH_BANDS_MAX];
+  float* xf_ptr = NULL;
+
   for (i = 0; i < NUM_HIGH_BANDS_MAX; ++i) {
     outputH_ptr[i] = outputH[i];
   }
 
-  float* xf_ptr = NULL;
-
   // Concatenate old and new nearend blocks.
   for (i = 0; i < aec->num_bands - 1; ++i) {
     WebRtc_ReadBuffer(aec->nearFrBufH[i],
                       (void**)&nearend_ptr,
                       nearend,
                       PART_LEN);
     memcpy(aec->dBufH[i] + PART_LEN, nearend_ptr, sizeof(nearend));
   }
   WebRtc_ReadBuffer(aec->nearFrBuf, (void**)&nearend_ptr, nearend, PART_LEN);
   memcpy(aec->dBuf + PART_LEN, nearend_ptr, sizeof(nearend));
@@ skipping 87 matching lines @@
   }
 
   // Buffer xf
   memcpy(aec->xfBuf[0] + aec->xfBufBlockPos * PART_LEN1,
          xf_ptr,
          sizeof(float) * PART_LEN1);
   memcpy(aec->xfBuf[1] + aec->xfBufBlockPos * PART_LEN1,
          &xf_ptr[PART_LEN1],
          sizeof(float) * PART_LEN1);
 
-  memset(yf, 0, sizeof(yf));
+  // Perform echo subtraction.
+  EchoSubtraction(aec, nearend_ptr);
 
-  // Filter far
-  WebRtcAec_FilterFar(aec, yf);
-
-  // Inverse fft to obtain echo estimate and error.
-  fft[0] = yf[0][0];
-  fft[1] = yf[0][PART_LEN];
-  for (i = 1; i < PART_LEN; i++) {
-    fft[2 * i] = yf[0][i];
-    fft[2 * i + 1] = yf[1][i];
-  }
-  aec_rdft_inverse_128(fft);
-
-  scale = 2.0f / PART_LEN2;
-  for (i = 0; i < PART_LEN; i++) {
-    y[i] = fft[PART_LEN + i] * scale;  // fft scaling
-  }
-
-  for (i = 0; i < PART_LEN; i++) {
-    e[i] = nearend_ptr[i] - y[i];
-  }
-
-  // Error fft
-  memcpy(aec->eBuf + PART_LEN, e, sizeof(float) * PART_LEN);
-  memset(fft, 0, sizeof(float) * PART_LEN);
-  memcpy(fft + PART_LEN, e, sizeof(float) * PART_LEN);
-  // TODO(bjornv): Change to use TimeToFrequency().
-  aec_rdft_forward_128(fft);
-
-  ef[1][0] = 0;
-  ef[1][PART_LEN] = 0;
-  ef[0][0] = fft[0];
-  ef[0][PART_LEN] = fft[1];
-  for (i = 1; i < PART_LEN; i++) {
-    ef[0][i] = fft[2 * i];
-    ef[1][i] = fft[2 * i + 1];
-  }
-
-  RTC_AEC_DEBUG_RAW_WRITE(aec->e_fft_file,
-                          &ef[0][0],
-                          sizeof(ef[0][0]) * PART_LEN1 * 2);
-
-  if (aec->metricsMode == 1) {
-    // Note that the first PART_LEN samples in fft (before transformation) are
-    // zero. Hence, the scaling by two in UpdateLevel() should not be
-    // performed. That scaling is taken care of in UpdateMetrics() instead.
-    UpdateLevel(&aec->linoutlevel, ef);
-  }
-
-  // Scale error signal inversely with far power.
-  WebRtcAec_ScaleErrorSignal(aec, ef);
-  WebRtcAec_FilterAdaptation(aec, fft, ef);
-  NonLinearProcessing(aec, output, outputH_ptr);
+  // Perform echo suppression.
+  EchoSuppression(aec, 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);
   // For high bands
   for (i = 0; i < aec->num_bands - 1; ++i) {
     WebRtc_WriteBuffer(aec->outFrBufH[i], outputH[i], PART_LEN);
   }
 
-  RTC_AEC_DEBUG_WAV_WRITE(aec->outLinearFile, e, PART_LEN);
   RTC_AEC_DEBUG_WAV_WRITE(aec->outFile, output, PART_LEN);
 }
 
 AecCore* WebRtcAec_CreateAec() {
   int i;
   AecCore* aec = malloc(sizeof(AecCore));
   if (!aec) {
     return NULL;
   }
 
@@ skipping 523 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;
 }