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 4b5abedd5aa7694172fe773438fcc0822dc08ee9..aaca520683cee64dfc9a086db06e0b2ea338d61e 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.cc
+++ b/webrtc/modules/audio_processing/aec/aec_core.cc
@@ -28,6 +28,7 @@
 extern "C" {
 #include "webrtc/common_audio/ring_buffer.h"
 }
+#include "webrtc/base/checks.h"
 #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"
@@ -365,6 +366,42 @@ static int PartitionDelay(const AecCore* aec) {
   return delay;
 }
 
+// Update metric with 10 * log10(numerator / denominator).
+static void UpdateLogRatioMetric(Stats* metric, float numerator,
+                                 float denominator) {
+  RTC_DCHECK(metric);
+  RTC_CHECK(numerator >= 0);

[tlegrand-webrtc, 2016/04/25 13:09:56]

Why isn't numerator =  0 allowed?

[kwiberg-webrtc, 2016/04/25 13:23:58]

It is. But what I'm wondering is why we allow denominator == 0...

(Also, for the future, please use RTC_DCHECK_GE, _GT, etc.---they print better
error messages.)

[minyue-webrtc, 2016/04/25 13:26:56]

It is allowed, isn't it?

[tlegrand-webrtc, 2016/04/28 08:17:30]

Right, my mistake. I was thinking 0 wouldn't be allowed for the denominator and
confused myself. But I realize now that 0 should be (and is) allowed for both.

+  RTC_CHECK(denominator >= 0);
+
+  const float log_numerator = log10(numerator + 1e-10f);
+  const float log_denominator = log10(denominator + 1e-10f);

[tlegrand-webrtc, 2016/04/25 13:09:56]

Which is more computational complex, log10 or division? I'm thinking log10 is
more expensive, but I'm not sure. If it is, we are wasting cycles here, just to
get aroung the division with 0, right?

[kwiberg-webrtc, 2016/04/25 13:23:58]

Yes, I'd be surprised if division wasn't significantly faster than log10. See
e.g.
http://stackoverflow.com/questions/2858483/how-can-i-compare-the-performance-...

[minyue-webrtc, 2016/04/25 13:26:56]

Per also asked about this. My reasoning was that it is not only to avoid
dividing by zero, but also to avoid overflow. Say if a/b results in infinity (b
does not need to be zero), it does not mean that log(a/b) is infinity. So log a
- log b has a better representability.

[tlegrand-webrtc, 2016/04/28 08:17:30]

Acknowledged.

+  metric->instant = 10.0f * (log_numerator - log_denominator);
+
+  // Max.

[tlegrand-webrtc, 2016/04/25 13:09:56]

This comment doesn't add any value. Same with "Min." below.

[minyue-webrtc, 2016/04/25 13:26:56]

I agree. I'd like to make a separate CL, which may refactor hicounter etc. as
well.

[tlegrand-webrtc, 2016/04/28 08:17:30]

Acknowledged.

+  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.

[tlegrand-webrtc, 2016/04/25 13:09:56]

hicounter, hisum and himean doesn't follow the naming convention. Should be on
the format hi_counter, but I'd like to see something more descriptive than "hi".

[minyue-webrtc, 2016/04/25 13:26:56]

True, this will need to refactor other part of AEC. We may better do it in a
separate CL.

[tlegrand-webrtc, 2016/04/28 08:17:30]

Acknowledged.

+  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;
 
@@ -638,16 +675,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++;
@@ -674,95 +707,22 @@ static void UpdateMetrics(AecCore* aec) {
         (aec->farlevel.framelevel.EndOfBlock()) &&
         (far_average_level > (actThreshold * aec->farlevel.minlevel))) {
 
+      // ERL: error return loss.
       const float near_average_level =
           aec->nearlevel.averagelevel.GetLatestMean();
+      UpdateLogRatioMetric(&aec->erl, far_average_level, near_average_level);
 
-      // 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;
-      }
-
-      // A_NLP
+      // A_NLP: error return loss enhanced before the nonlinear suppression.
       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;
+      UpdateLogRatioMetric(&aec->aNlp, near_average_level,
+                           linout_average_level);
 
-      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;
-      }
-
-      // ERLE
+      // ERLE: error return loss enhanced.
       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;
-      }
+      UpdateLogRatioMetric(&aec->erle, near_average_level,
+                           nlpout_average_level);
     }
 
     aec->stateCounter = 0;