Cleaning up AEC metrics.

webrtc/modules/audio_processing/aec/aec_core.cc

 /*
  *  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 20 matching lines @@
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
 #include "webrtc/modules/audio_processing/aec/aec_common.h"
 #include "webrtc/modules/audio_processing/aec/aec_core_internal.h"
 extern "C" {
 #include "webrtc/modules/audio_processing/aec/aec_rdft.h"
 }
 #include "webrtc/modules/audio_processing/logging/aec_logging.h"
 extern "C" {
 #include "webrtc/modules/audio_processing/utility/delay_estimator_wrapper.h"
 }
+#include "webrtc/base/checks.h"
 #include "webrtc/system_wrappers/include/cpu_features_wrapper.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 
 // Buffer size (samples)
 static const size_t kBufSizePartitions = 250;  // 1 second of audio in 16 kHz.
 
 // Metrics
 static const size_t kSubCountLen = 4;
@@ skipping 311 matching lines @@
     }
 
     if (wfEn > wfEnMax) {
       wfEnMax = wfEn;
       delay = i;
     }
   }
   return delay;
 }
 
+// Update metric with 10 * log10(numerator / denominator),
+static void UpdateMetric(Stats* metric, float numerator, float denominator) {

[peah-webrtc, 2016/04/19 07:03:37]

Did you consider changing the name of the method to reflect that it only relates
to updating stats metric?

What about using the name
UpdateStatsMetric?
Or maybe better, UpdateRatioBasedMetric, as that would partly explain the
meanings of numerator/demoninator.

[peah-webrtc, 2016/04/19 07:03:37]

The roles of the numerator/denominator input variables are very unclear and the
names are quite anonymous. If you change the name of the function to
UpdateRatioBasedMetric their meaning will be more clear, but if you could come
up with some other way to more properly name the variables (I could not) it
would be even better.

+  RTC_DCHECK(metric);
+  RTC_CHECK(numerator >= 0);
+  RTC_CHECK(denominator >= 0);
+
+  const float log_numerator = log10(numerator + 1e-10);
+  const float log_denominator = log10(denominator + 1e-10);
+  metric->instant = 10.0 * (log_numerator - log_denominator);

[peah-webrtc, 2016/04/19 07:03:37]

I don't see why we cannot treat the case of the denominator == 0 as a special
case. It is cheaper, the calculation is more correct and it is explicitly
stating that the case of denominator == 0 is a special one that should be
treated separately.

Adding the epsilon term is fine mathematically, but apart from fixing the
numerical issue of division by zero I think it hides away information that could
otherwise be passed to the metric and adds an unnecessary bias.

[minyue-webrtc, 2016/04/19 07:21:06]

log(a) - log(b) does not only solve denominator being zero. Another problem with
a/b is that if a/b goes out of range, it will become inf, even though log can
still represent it.

[peah-webrtc, 2016/04/19 10:48:34]

That is definitely true. Let's keep it like this. But please look at th e other
comments as well.

 

+
+  // Max.
+  if (metric->instant > metric->max)
+    metric->max = metric->instant;
+
+  // Min.
+  if (metric->instant < metric->min)
+    metric->min = metric->instant;
+
+  // Average.
+  metric->counter++;
+  // This is to protect overflow, which should almost never happen.
+  RTC_CHECK_NE(0u, metric->counter);
+  metric->sum += metric->instant;
+  metric->average = metric->sum / metric->counter;
+
+  // Upper mean.
+  if (metric->instant > metric->average) {
+    metric->hicounter++;
+    // This is to protect overflow, which should almost never happen.
+    RTC_CHECK_NE(0u, metric->hicounter);
+    metric->hisum += metric->instant;
+    metric->himean = metric->hisum / metric->hicounter;
+  }
+}
+
 // Threshold to protect against the ill-effects of a zero far-end.
 const float WebRtcAec_kMinFarendPSD = 15;
 
 // Updates the following smoothed  Power Spectral Densities (PSD):
 //  - sd  : near-end
 //  - se  : residual echo
 //  - sx  : far-end
 //  - sde : cross-PSD of near-end and residual echo
 //  - sxd : cross-PSD of near-end and far-end
 //
@@ skipping 253 matching lines @@
         level->minlevel = new_frame_level;  // New minimum.
       } else {
         level->minlevel *= (1 + 0.001f);  // Small increase.
       }
     }
     level->averagelevel.AddValue(new_frame_level);
   }
 }
 
 static void UpdateMetrics(AecCore* aec) {
-  float dtmp;
-
   const float actThresholdNoisy = 8.0f;
   const float actThresholdClean = 40.0f;
-  const float safety = 0.99995f;
 
   const float noisyPower = 300000.0f;
 
   float actThreshold;
-  float echo, suppressedEcho;
 
   if (aec->echoState) {  // Check if echo is likely present
     aec->stateCounter++;
   }
 
   if (aec->linoutlevel.framelevel.EndOfBlock()) {
     aec->divergent_filter_fraction.AddObservation(aec->nearlevel,
                                                   aec->linoutlevel,
                                                   aec->nlpoutlevel);
   }
 
   if (aec->farlevel.averagelevel.EndOfBlock()) {
     if (aec->farlevel.minlevel < noisyPower) {
       actThreshold = actThresholdClean;
     } else {
       actThreshold = actThresholdNoisy;
     }
 
     const float far_average_level = aec->farlevel.averagelevel.GetLatestMean();
 
     // The last condition is to let estimation be made in active far-end
     // segments only.
     if ((aec->stateCounter > (0.5f * kCountLen * kSubCountLen)) &&
         (aec->farlevel.framelevel.EndOfBlock()) &&
         (far_average_level > (actThreshold * aec->farlevel.minlevel))) {
 
+      // ERL

[peah-webrtc, 2016/04/19 07:03:37]

The terminating period in the comment is still missing. Also, I still think it
would be better if you instead of the abbreviations wrote the description of the
abbreviation (e.g., instead of ERL wrote echo return loss).

       const float near_average_level =
           aec->nearlevel.averagelevel.GetLatestMean();
 
-      // Subtract noise power
-      echo = near_average_level - safety * aec->nearlevel.minlevel;
-
-      // ERL
-      dtmp = 10 * static_cast<float>(log10(far_average_level /
-                                           near_average_level + 1e-10f));
-
-      aec->erl.instant = dtmp;
-      if (dtmp > aec->erl.max) {
-        aec->erl.max = dtmp;
-      }
-
-      if (dtmp < aec->erl.min) {
-        aec->erl.min = dtmp;
-      }
-
-      aec->erl.counter++;
-      aec->erl.sum += dtmp;
-      aec->erl.average = aec->erl.sum / aec->erl.counter;
-
-      // Upper mean
-      if (dtmp > aec->erl.average) {
-        aec->erl.hicounter++;
-        aec->erl.hisum += dtmp;
-        aec->erl.himean = aec->erl.hisum / aec->erl.hicounter;
-      }
+      UpdateMetric(&aec->erl, far_average_level, near_average_level);
 
       // A_NLP

[peah-webrtc, 2016/04/19 07:03:37]

The terminating period in the comment is still missing. Also, I still think it
would be better if you instead of the abbreviations wrote the description of the
abbreviation (e.g., instead of ERL wrote echo return loss).

       const float linout_average_level =
           aec->linoutlevel.averagelevel.GetLatestMean();
-      dtmp = 10 * static_cast<float>(log10(near_average_level /
-                                           linout_average_level + 1e-10f));
-
-      // subtract noise power
-      suppressedEcho =
-          linout_average_level - safety * aec->linoutlevel.minlevel;
-
-      aec->aNlp.instant =
-          10 * static_cast<float>(log10(echo / suppressedEcho + 1e-10f));
-
-      if (dtmp > aec->aNlp.max) {
-        aec->aNlp.max = dtmp;
-      }
-
-      if (dtmp < aec->aNlp.min) {
-        aec->aNlp.min = dtmp;
-      }
-
-      aec->aNlp.counter++;
-      aec->aNlp.sum += dtmp;
-      aec->aNlp.average = aec->aNlp.sum / aec->aNlp.counter;
-
-      // Upper mean
-      if (dtmp > aec->aNlp.average) {
-        aec->aNlp.hicounter++;
-        aec->aNlp.hisum += dtmp;
-        aec->aNlp.himean = aec->aNlp.hisum / aec->aNlp.hicounter;
-      }
+      UpdateMetric(&aec->aNlp, near_average_level, linout_average_level);
 
       // ERLE

[peah-webrtc, 2016/04/19 07:03:37]

The terminating period in the comment is still missing. Also, I still think it
would be better if you instead of the abbreviations wrote the description of the
abbreviation (e.g., instead of ERL wrote echo return loss).

       const float nlpout_average_level =
           aec->nlpoutlevel.averagelevel.GetLatestMean();
-      // subtract noise power
-      suppressedEcho =
-          nlpout_average_level - safety * aec->nlpoutlevel.minlevel;
-      dtmp = 10 * static_cast<float>(log10(echo / suppressedEcho + 1e-10f));
-
-      aec->erle.instant = dtmp;
-      if (dtmp > aec->erle.max) {
-        aec->erle.max = dtmp;
-      }
-
-      if (dtmp < aec->erle.min) {
-        aec->erle.min = dtmp;
-      }
-
-      aec->erle.counter++;
-      aec->erle.sum += dtmp;
-      aec->erle.average = aec->erle.sum / aec->erle.counter;
-
-      // Upper mean
-      if (dtmp > aec->erle.average) {
-        aec->erle.hicounter++;
-        aec->erle.hisum += dtmp;
-        aec->erle.himean = aec->erle.hisum / aec->erle.hicounter;
-      }
+      UpdateMetric(&aec->erle, near_average_level, nlpout_average_level);
     }
 
     aec->stateCounter = 0;
   }
 }
 
 static void UpdateDelayMetrics(AecCore* self) {
   int i = 0;
   int delay_values = 0;
   int median = 0;
@@ skipping 1168 matching lines @@
 
 int WebRtcAec_system_delay(AecCore* self) {
   return self->system_delay;
 }
 
 void WebRtcAec_SetSystemDelay(AecCore* self, int delay) {
   assert(delay >= 0);
   self->system_delay = delay;
 }
 }  // namespace webrtc