Adding fraction of filter divergence in 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 e23a79312b05beb9c7904c103c605a5d454b3e69..097aae7dc9369e679d8eb60692c6a3e91adf27ed 100644
--- a/webrtc/modules/audio_processing/aec/aec_core.cc
+++ b/webrtc/modules/audio_processing/aec/aec_core.cc
@@ -50,6 +50,12 @@ static const size_t kSubCountLen = 4;
 static const size_t kCountLen = 50;
 static const int kDelayMetricsAggregationWindow = 1250;  // 5 seconds at 16 kHz.
 
+// Divergence metric is based on audio level, which gets updated every
+// |kCountLen + 1| * 10 milliseconds. Divergence metric takes the statistics of
+// |kDivergeMetricAggregationWindow| samples. Current value corresponds to 0.5
+// seconds at 16 kHz.
+static const int kDivergeMetricAggregationWindow = 25;
+
 // Quantities to control H band scaling for SWB input
 static const float cnScaleHband = 0.4f;  // scale for comfort noise in H band.
 // Initial bin for averaging nlp gain in low band
@@ -151,15 +157,17 @@ __inline static float MulIm(float aRe, float aIm, float bRe, float bIm) {
 }
 
 PowerLevel::PowerLevel()
-// TODO(minyue): Due to a legacy bug, |framelevel| and |averagelevel| use a
-// window, of which the length is 1 unit longer than indicated. Remove "+1"
-// when the code is refactored.
-: framelevel(kSubCountLen + 1),
-  averagelevel(kCountLen + 1) {
+    // TODO(minyue): Due to a legacy bug, |framelevel| and |averagelevel| use a

[peah-webrtc, 2016/04/05 09:15:52]

I'd prefer to have the comments outside of the constructor definition.

[minyue-webrtc, 2016/04/05 09:50:31]

Done.

+    // window, of which the length is 1 unit longer than indicated. Remove "+1"
+    // when the code is refactored.
+    : framelevel(kSubCountLen + 1),
+      averagelevel(kCountLen + 1) {
 }
 
 // TODO(minyue): Moving some initialization from WebRtcAec_CreateAec() to ctor.
-AecCore::AecCore() = default;
+AecCore::AecCore()
+    : fraction_filter_divergent(kDivergeMetricAggregationWindow) {
+}
 
 static int CmpFloat(const void* a, const void* b) {
   const float* da = (const float*)a;
@@ -562,6 +570,8 @@ static void InitMetrics(AecCore* self) {
   InitStats(&self->erle);
   InitStats(&self->aNlp);
   InitStats(&self->rerl);
+
+  self->fraction_filter_divergent.Reset();
 }
 
 static float CalculatePower(const float* in, size_t num_samples) {
@@ -605,6 +615,19 @@ static void UpdateMetrics(AecCore* aec) {
     aec->stateCounter++;
   }
 
+  if (aec->linoutlevel.framelevel.EndOfBlock()) {
+    const float near_level = aec->nearlevel.framelevel.GetLatestMean();
+    const float level_increase =
+        aec->linoutlevel.framelevel.GetLatestMean() - near_level;
+    const bool audible = aec->nlpoutlevel.framelevel.GetLatestMean() >

[peah-webrtc, 2016/04/05 09:15:52]

The audible flag basically checks wether the NLP mean is more than 100 times (20
dB) larger than the NLP min, right? To me the name does not really make sense as
I cannot make the connection to what it is that would be audible; the echo or
the nearend?
Please either clarify this in a comment, or rename the variable.

[minyue-webrtc, 2016/04/05 09:50:31]

how about nlpout_active?

[peah-webrtc, 2016/04/05 10:42:03]

That is also vague to me. Do you mean that the NLP is transparent?

audible will be high if 
-the nlp gain varies between low and high, 
but low it either
-the nlp gain is constantly high
-the nlp gain is constantly low.

Therefore I think audible is neither a measure of nlp transparency, nlp
activeness or whether something is audible in the output.

[minyue-webrtc, 2016/04/05 12:29:12]

I used nlpout_active to tell if the final output signal is active or silent. It
is similar to line 644 that measures whether far-end signal is active.

But maybe nlpout_active as a name has too much emphasis on nlp, how about
out_signal_active?

+        100.0 * aec->nlpoutlevel.minlevel;

[minyue-webrtc, 2016/04/04 15:07:44]

I found that 100.0 might be a bit large. I prefer using 40.0, which is
consistent with actThresholdClean (line 606). Experiments showed a bit better
result.

[peah-webrtc, 2016/04/05 09:15:52]

What do you mean here? Do you want to keep it 100 or do you want to change it to
40?

[minyue-webrtc, 2016/04/05 09:50:31]

I want to change to 40, which gave a bit better result in my offline tests. I
did not add a patch set just for this, therefore, I put a comment instead.

[peah-webrtc, 2016/04/05 10:42:03]

Acknowledged.

+    // Level increase should be, in principle, negative, when the filter
+    // does not diverge. Here we allow some margin (0.01 * near end level) and
+    // numerical error (1.0).
+    aec->fraction_filter_divergent.AddValue(audible &&

[peah-webrtc, 2016/04/05 09:15:52]

What about renaming fraction_filter_divergent to filter_divergence?

[peah-webrtc, 2016/04/05 09:15:52]

Here the fraction_filter_divergent object is only used as an integer counter. I
don't think the MeanCalculator should be used for this as that is a class for
computing the blocked mean of floating point variables.

In this case, the all the variables are integers and as the block length is
known and hardcoded the mean does not really need to be computed. 

[minyue-webrtc, 2016/04/05 09:50:31]

I agree that BlockMeanCalculator is for floating point, while fraction is to
count the occurrences. If we want to calculate fraction, we basically need to
introduce three variables:

size_t count_  // counts total samples
size_t occurrences_  // counts occurrences
float latest_fraction_

and do the following

if (event occurred) {
  occurrences_++;
}
count_++
if (count_ == block_length) {
  count_ = 0;
  latest_fraction_ = (float)occurrences_ / block_length;
}

This is essentially what BlockMeanCalculator does. I think using
BlockMeanCalculator eliminates much code duplication.

My suggestion is adding a comment on why we can use BlockMeanCalculator to
calculate fraction. WDYT?

[minyue-webrtc, 2016/04/05 09:50:31]

Tina also wanted to drop "fraction". My idea is that the value is between 0 and
1, and "fraction" directly tells what it means.

Regarding "divergent" or "divergence", I have no preference. "divergent" was
suggested by Karl.

How about "filter_divergence_fraction"?

[peah-webrtc, 2016/04/05 10:42:03]

I think it would be better to use a proper counter or that you create a separate
class for that. The current usage of the AddValue method involves an implicit
cast from bool to int and relies on this to be a summable integer which is added
to the mean.

I think that even though it is fully possible to reuse the BlockMeanCalculator
this complicates the code. (And even though the code is shorter, it is slightly
more complex as the cast is needed and as the AddValue calls is needed even when
the filter is not diverging).

[peah-webrtc, 2016/04/05 10:42:03]

Lets go with that, or with divergent_filter_fraction. It is nice to have a unit
attached to the variable; I think there should be a better unit for that but I
cannot come up with a better suggestion so lets go for that.

[minyue-webrtc, 2016/04/05 12:29:12]

Then I prefer to add a new class: BlockFractionCalculator. It can also be used
for fraction_poor_delays in the future.

+        level_increase > std::max(0.01 * near_level, 1.0) ? 1.0 : 0.0);
+  }
+
   if (aec->farlevel.averagelevel.EndOfBlock()) {
     if (aec->farlevel.minlevel < noisyPower) {
       actThreshold = actThresholdClean;