Cleaning up AEC metrics.

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 901e0fde0b8b5300c4b657b59bfdf61c7b8c116d..d7902488cb2e0be427413be99c5ca85bcd2ac105 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.c
+++ b/webrtc/modules/audio_processing/aec/aec_core.c
@@ -316,6 +316,31 @@ static int PartitionDelay(const AecCore* aec) {
   return delay;
 }
 
+static void UpdateMetric(Stats* metric, float value) {
+  // Instant.
+  metric->instant = value;
+
+  // Max.
+  if (metric->instant > metric->max)
+    metric->max = metric->instant;
+
+  // Min.
+  if (metric->instant < metric->min)
+    metric->min = metric->instant;
+
+  // Average.
+  metric->counter++;

[peah-webrtc, 2016/01/15 06:52:20]

This increase should handle wraparounds. I know this is very unlikely to happen,
but regardless of that I think we should still have it.
(for 32 bit ints, this will wraparound after 248 days, but for 16 bit ints it
will wraparound after 5 hours).

[tlegrand-webrtc, 2016/04/13 14:33:31]

To Per: Are there any platforms today where int is only 16-bits?

[peah-webrtc, 2016/04/14 13:46:10]

I would guess there are are no important such platforms. As far as I know, int
is in practice only 16 bit on 16 bit platforms. 

[kwiberg-webrtc, 2016/04/14 14:01:47]

"You should assume that an int is at least 32 bits, but don't assume that it has
more than 32 bits."
https://google.github.io/styleguide/cppguide.html#Integer_Types

+  metric->sum += metric->instant;
+  metric->average = metric->sum / metric->counter;
+
+  // Upper mean.
+  if (metric->instant > metric->average) {
+    metric->hicounter++;

[peah-webrtc, 2016/01/15 06:52:20]

As above, please add handling of wraparound.

+    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;
 
@@ -625,15 +650,11 @@ static void UpdateLevel(PowerLevel* level, float in[2][PART_LEN1]) {
 }
 
 static void UpdateMetrics(AecCore* aec) {
-  float dtmp, dtmp2;
-
   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++;
@@ -655,97 +676,17 @@ static void UpdateMetrics(AecCore* aec) {
         (aec->farlevel.averagelevel >
          (actThreshold * aec->farlevel.minlevel))) {
 
-      // Subtract noise power
-      echo = aec->nearlevel.averagelevel - safety * aec->nearlevel.minlevel;
-
       // ERL
-      dtmp = 10 * (float)log10(aec->farlevel.averagelevel /
-                                   aec->nearlevel.averagelevel +
-                               1e-10f);
-      dtmp2 = 10 * (float)log10(aec->farlevel.averagelevel / echo + 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, 10 * (float)log10(1e-10f +

[peah-webrtc, 2016/01/15 06:52:20]

While we are anyway changing the metrics: is there any reason for logging the
metric in dB? It is anyway stored as a float, and the log function may be costly
on some platforms. 

Storing the metric as the actual ratio would
1) Remove 3 log10 operations from the code.
2) Allow us to remove the 1e-10f constant which bias the values for levels close
to zero.

If the people monitoring these metrics know how to interpret these dB values, we
probably should keep them in dB (provided that there is no way to do that
conversion in the stats monitoring system). 
Otherwise I think we should instead store the actual ratio.

[minyue-webrtc, 2016/01/15 10:24:49]

Good suggestion, but be minded that to an upper level, we need to report integer
(unless we want to change that also). Then I am not sure if dynamic range of
integer is bit enough.

+          aec->farlevel.averagelevel / aec->nearlevel.averagelevel));

[peah-webrtc, 2016/01/15 06:52:20]

Is there any guarantee that averagelevel cannot be 0?

 
       // A_NLP
-      dtmp = 10 * (float)log10(aec->nearlevel.averagelevel /
-                                   (2 * aec->linoutlevel.averagelevel) +
-                               1e-10f);
-
-      // subtract noise power
-      suppressedEcho = 2 * (aec->linoutlevel.averagelevel -
-                            safety * aec->linoutlevel.minlevel);
-
-      dtmp2 = 10 * (float)log10(echo / suppressedEcho + 1e-10f);
-
-      aec->aNlp.instant = dtmp2;
-      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, 10 * (float)log10(1e-10f +
+          aec->nearlevel.averagelevel / (2 * aec->linoutlevel.averagelevel)));

[peah-webrtc, 2016/01/15 06:52:20]

Is there any guarantee that averagelevel cannot be 0?

[minyue-webrtc, 2016/01/15 10:24:49]

I don't see, and I am not sure how it did not run into any problem before. Good
to add epsilon to the denominator any way, IMO.

 
       // ERLE
-
-      // subtract noise power
-      suppressedEcho = 2 * (aec->nlpoutlevel.averagelevel -
-                            safety * aec->nlpoutlevel.minlevel);
-
-      dtmp = 10 * (float)log10(aec->nearlevel.averagelevel /
-                                   (2 * aec->nlpoutlevel.averagelevel) +
-                               1e-10f);
-      dtmp2 = 10 * (float)log10(echo / suppressedEcho + 1e-10f);
-
-      dtmp = dtmp2;
-      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, 10 * (float)log10(1e-10f +
+          aec->nearlevel.averagelevel / (2 * aec->nlpoutlevel.averagelevel)));

[peah-webrtc, 2016/01/15 06:52:20]

Is there any guarantee that averagelevel cannot be 0?

     }
 
     aec->stateCounter = 0;