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..b51101fd3b2e7e84c3b56629ee456c55ab5e615f 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.cc
+++ b/webrtc/modules/audio_processing/aec/aec_core.cc
@@ -369,6 +369,37 @@ static int PartitionDelay(const AecCore* aec) {
   return delay;
 }
 
+// Update metric with 10 * log10(nomin / denom),
+static void UpdateMetric(Stats* metric, float nomin, float denom) {

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

How do we test this function, to make sure it handles different input in  the
right way, like metric == NULL and denom = 0?

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

Since it is local within aec_core.cc, I think a DCHECK on metric not being null
should be sufficient for that. But the check for demom==0 must be added.

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

Breaking out the computation of the metric in a separate function is great as it
removes a lot of duplicated code.

I think, however, that the function name UpdateMetric is misleading, as it is
only a certain type of metric that is updated by this.

[minyue-webrtc, 2016/04/14 14:11:12]

Yes, it is good to sanity check on |metric|, added a DCHECK.

For denom = 0, it will be caught by std::isfinite(temp), which will catch other
error, like ((a very large number) / (a very small number))

[peah-webrtc, 2016/04/15 10:46:37]

Would it be possible to change the names of nomin and denom to something more
meaningful that indicates what kind of values that are passed to the function?

[peah-webrtc, 2016/04/15 10:46:37]

Maybe it is ok and a good solution, but to me it feels weird to do the division
by zero.

However, what I don't think is correct in that case is to just keep the last
computed value for the metric. If the metric is infinite, that has a meaning in
itself and should not be hidden by not updating the metric.
I'd rather have a maximum value set for the metric if it is detected to be
infinite.

[minyue-webrtc, 2016/04/18 10:56:25]

I think one reason for inf is that we calculate division then log, and the
division is easy to overflow. How about use (log a - log b) instead of log(a/b),
see new patch.

[minyue-webrtc, 2016/04/18 10:56:25]

Done.

+  const float temp = nomin / denom;

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

Is division with 0 allowed for float?

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

Please rename this variable to what is is, as it is only being used for this
purpose. For instance, the name quotient would be more indicative to what it
stores.

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

No, we should never do that. (If happening on the tests, that will also throw an
exception that the trybots won't accept.)

[minyue-webrtc, 2016/04/14 14:11:12]

For float, denom = 0 will not crash.

[peah-webrtc, 2016/04/15 10:46:37]

Ah, you are right, I did not know that.

But I still don't see the advantage of first doing the division and then test
for whether it becomes infinite instead of first checking if it will become
infinite and only do the division if it will not become infinite.

[minyue-webrtc, 2016/04/18 10:56:25]

Done.

+  if (!std::isfinite(temp))

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

I think it is better to check for denom being zero, and then return. That is
more explicit, avoids the division by zero, and has a lower complexity.

Furthermore, I think a special care should be taken to what happens with the
metric when denom is zero. For instance, an SNR should probably be set high if
the denom is zero, rather than just keep the previous SNR.

+    return;
+
+  // Instant.
+  // Adding 1e-10 to set the lower bound to -100.
+  metric->instant = 10.0 * log10(temp + 1e-10);
+
+  // Max.
+  if (metric->instant > metric->max)
+    metric->max = metric->instant;
+
+  // Min.
+  if (metric->instant < metric->min)
+    metric->min = metric->instant;
+
+  // Average.
+  metric->counter++;

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

Do we ever reset these counters? I realize you didn't change this functionality,
but might be worth checking. What happens if you keep a call running over night?

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

I think it is nice to have a controlled wraparound, even though they will take
very long to wraparound on 32 bit systems. For 32 bit integers, if they are
updated 250 times a second (currently they are updated less frequenctly)
wraparound will happen after 99 days, but since that wraparound will eventually
cause a division by zero I think it is better to handle that case explicitly in
a controlled manner.

[minyue-webrtc, 2016/04/14 14:11:12]

Very good point. I am not fully sure what we would like the average value to
represent. The average of a full call? If so, we only need to try to avoid
overflow or improve the calculation.

I did two things: 1) change the counter to type to size_t, which this, overflow
should in principle never happen. 2) add a CHECK to protect overflow.

[minyue-webrtc, 2016/04/14 14:11:12]

After 391, I added a RTC_CHECK_NE(0u, metric->counter). It is gone (weird). Do
you think it is good? or is it better to add something like

if (metric->counter == 0) {
  metric->sum = 0;
} else {
  metric->sum += metric->instant;
  metric->average = metric->sum / metric->counter;
}

To me, |metric->counter == 0| is more or less a design error, for e.g., too
frequent update, or wrong type on counter. A check would be good. But I agree it
is safer to go for the second solution.

[peah-webrtc, 2016/04/15 10:46:37]

Acknowledged.

+  metric->sum += metric->instant;
+  metric->average = metric->sum / metric->counter;
+
+  // Upper mean.
+  if (metric->instant > metric->average) {
+    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 +673,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 +705,21 @@ static void UpdateMetrics(AecCore* aec) {
         (aec->farlevel.framelevel.EndOfBlock()) &&
         (far_average_level > (actThreshold * aec->farlevel.minlevel))) {
 
+      // ERL

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

Comment should be terminated with a period. Furthermore, I think it would be
good with a more verbose comment, since the ERL abbreviation does not add
anything more than what can be read from the UpdateMetric method call. For
instance, a description describing in words what the metric measures.

[minyue-webrtc, 2016/04/18 10:56:25]

Done.

       const float near_average_level =
           aec->nearlevel.averagelevel.GetLatestMean();
 

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

To be consistent with how it is done below, I think the empty line here should
be removed.

[minyue-webrtc, 2016/04/18 10:56:25]

Done.

-      // 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/14 13:46:10]

See comment above.

[minyue-webrtc, 2016/04/18 10:56:25]

done, although I am not sure if I name it properly. I do not know what A_NLP
stands for exactly, so I tried to described it from its calculation.

       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/14 13:46:10]

See comment above.

[minyue-webrtc, 2016/04/18 10:56:25]

Done.

       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;