New statistics interface for APM

webrtc/modules/audio_processing/include/audio_processing.h

Index: webrtc/modules/audio_processing/include/audio_processing.h
diff --git a/webrtc/modules/audio_processing/include/audio_processing.h b/webrtc/modules/audio_processing/include/audio_processing.h
index 0fdbfd2d75cf48caa50bb8b2f8a86f5e156ee353..07fd1df24112ab946eea175346b023c3123cacf0 100644
--- a/webrtc/modules/audio_processing/include/audio_processing.h
+++ b/webrtc/modules/audio_processing/include/audio_processing.h
@@ -469,6 +469,46 @@ class AudioProcessing {
   // specific member variables are reset.
   virtual void UpdateHistogramsOnCallEnd() = 0;
 
+  struct Statistic {
+    int instant;  // Instantaneous value.

[the sun, 2016/10/24 08:26:58]

Since you're changing the API, I think it'd be better if these were floats.
Also, I think you should make them values private and have a setter, so that we
can enforce the invariant (instant, avg should lie within min/max).

Please also default init to 0.0f.

[ivoc, 2016/10/24 14:15:49]

I'm actually trying not to change the old statistics API, since it's used in a
lot of places. So instead of changing this struct I will add a new Statistic
struct to AudioProcessingStatics that behaves like you suggest (I named it
ValueStatistic, not sure if that makes sense), and leave this one in place for
the old API for now. Once the code that uses the statistics is updated we can
get rid of the old API and this struct as well.

I made one setter for all 4 values, otherwise the order of calling the setters
would determine if the invariants hold, which is not nice.

+    int average;  // Long-term average.
+    int maximum;  // Long-term maximum.
+    int minimum;  // Long-term minimum.
+  };
+  struct AudioProcessingStatistics {
+    // AEC Statistics.
+    // RERL = ERL + ERLE
+    Statistic residual_echo_return_loss = {0, 0, 0, 0};

[the sun, 2016/10/24 08:26:58]

With default init, no need for this (error-prone) manual initing.

[ivoc, 2016/10/24 14:15:49]

Good idea.

+    // ERL = 10log_10(P_far / P_echo)
+    Statistic echo_return_loss = {0, 0, 0, 0};
+    // ERLE = 10log_10(P_echo / P_out)
+    Statistic echo_return_loss_enhancement = {0, 0, 0, 0};
+    // (Pre non-linear processing suppression) A_NLP = 10log_10(P_echo / P_a)
+    Statistic a_nlp = {0, 0, 0, 0};
+    // Fraction of time that the AEC linear filter is divergent, in a 1-second
+    // non-overlapped aggregation window.
+    float divergent_filter_fraction = 0.0f;
+
+    // The delay metrics consists of the delay median and standard deviation. It
+    // also consists of the fraction of delay estimates that can make the echo
+    // cancellation perform poorly. The values are aggregated until the first
+    // call to |GetStatistics()| and afterwards aggregated and updated every
+    // second. Note that if there are several clients pulling metrics from
+    // |GetStatistics()| during a session the first call from any of them will
+    // change to one second aggregation window for all.
+    int delay_median = 0;
+    int delay_standard_deviation = 0;
+    float fraction_poor_delays = 0.0f;
+
+    // Residual echo detector likelihood. This value is not yet calculated and
+    // is currently always set to zero.
+    // TODO(ivoc): Implement this stat.
+    float residual_echo_likelihood = 0.0f;
+  };
+
+  virtual void SetStatisticsEnabled(bool enabled) = 0;

[peah-webrtc, 2016/10/21 07:02:45]

Do we need to have an enable statistics method? Can't we just always compute
statistics? They should anyway not be anything computationally complex.

[the sun, 2016/10/24 08:26:58]

Agree, but are you sure they're not expensive?

[peah-webrtc, 2016/10/24 09:44:38]

Almost, yes, in particular since 
-The AEC metrics are not run on mobile platforms and their complexity should be
negligible to the complexity of the AEC.
-The residual echo detector metrics are only run when the residual echo detector
is on, in which case they are obtained for free.

The question is, however a bit bigger than that I think since it also applies on
future metrics, will they be too complex? For a metric, I think we should be
able to rely on it being extremely lightweight.

Does that make sense? 

[ivoc, 2016/10/24 14:15:49]

Ok, I removed this function.

+  virtual void GetStatistics(AudioProcessingStatistics* stats) const = 0;
+
   // These provide access to the component interfaces and should never return
   // NULL. The pointers will be valid for the lifetime of the APM instance.
   // The memory for these objects is entirely managed internally.
@@ -480,13 +520,6 @@ class AudioProcessing {
   virtual NoiseSuppression* noise_suppression() const = 0;
   virtual VoiceDetection* voice_detection() const = 0;
 
-  struct Statistic {
-    int instant;  // Instantaneous value.
-    int average;  // Long-term average.
-    int maximum;  // Long-term maximum.
-    int minimum;  // Long-term minimum.
-  };
-
   enum Error {
     // Fatal errors.
     kNoError = 0,
@@ -708,6 +741,7 @@ class EchoCancellation {
     float divergent_filter_fraction;
   };
 
+  // Deprecated. Use GetStatistics on the AudioProcessing interface instead.
   // TODO(ajm): discuss the metrics update period.
   virtual int GetMetrics(Metrics* metrics) = 0;
 
@@ -724,8 +758,9 @@ class EchoCancellation {
   // Note that if there are several clients pulling metrics from
   // |GetDelayMetrics()| during a session the first call from any of them will
   // change to one second aggregation window for all.
-  // TODO(bjornv): Deprecated, remove.
+  // Deprecated. Use GetStatistics on the AudioProcessing interface instead.
   virtual int GetDelayMetrics(int* median, int* std) = 0;
+  // Deprecated. Use GetStatistics on the AudioProcessing interface instead.
   virtual int GetDelayMetrics(int* median, int* std,
                               float* fraction_poor_delays) = 0;