Cleaning up AEC metrics.

webrtc/modules/audio_processing/aec/aec_core.cc

Index: webrtc/modules/audio_processing/aec/aec_core.cc
diff --git a/webrtc/modules/audio_processing/aec/aec_core.cc b/webrtc/modules/audio_processing/aec/aec_core.cc
index c4379c617104cd12f622bc1631908b37620fd887..f9604ec5199d21587cf20d371389b9994607e3b8 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.cc
+++ b/webrtc/modules/audio_processing/aec/aec_core.cc
@@ -38,6 +38,7 @@ extern "C" {
 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"
 
@@ -369,6 +370,41 @@ static int PartitionDelay(const AecCore* aec) {
   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;
 
@@ -642,16 +678,12 @@ static void UpdateLevel(PowerLevel* level, float power) {
 }
 
 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++;
@@ -678,95 +710,21 @@ static void UpdateMetrics(AecCore* aec) {
         (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;