UMA histogram WebRTC.AecDelayBasedQuality updated

content/renderer/media/media_stream_audio_processor_options.cc

Index: content/renderer/media/media_stream_audio_processor_options.cc
diff --git a/content/renderer/media/media_stream_audio_processor_options.cc b/content/renderer/media/media_stream_audio_processor_options.cc
index b2465b3350286d05a8f543b8c11bf04455a0bca5..498374768d79e821ba9ee817043cfca80c4304f3 100644
--- a/content/renderer/media/media_stream_audio_processor_options.cc
+++ b/content/renderer/media/media_stream_audio_processor_options.cc
@@ -11,7 +11,6 @@
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/time/time.h"
 #include "content/common/media/media_stream_options.h"
 #include "content/renderer/media/media_stream_constraints_util.h"
 #include "content/renderer/media/media_stream_source.h"
@@ -82,6 +81,7 @@ enum DelayBasedEchoQuality {
   DELAY_BASED_ECHO_QUALITY_GOOD = 0,
   DELAY_BASED_ECHO_QUALITY_SPURIOUS,
   DELAY_BASED_ECHO_QUALITY_BAD,
+  DELAY_BASED_ECHO_QUALITY_INVALID,
   DELAY_BASED_ECHO_QUALITY_MAX
 };
 
@@ -94,7 +94,11 @@ DelayBasedEchoQuality EchoDelayFrequencyToQuality(float delay_frequency) {
   //   delay is out of bounds 10-80 % of the time.
   // DELAY_BASED_ECHO_QUALITY_BAD
   //   delay is mostly out of bounds >= 80 % of the time.
-  if (delay_frequency <= kEchoDelayFrequencyLowerLimit)
+  // DELAY_BASED_ECHO_QUALITY_INVALID
+  //   delay_frequency is negative which happens if we have insufficient data.
+  if (delay_frequency < 0)
+    return DELAY_BASED_ECHO_QUALITY_INVALID;
+  else if (delay_frequency <= kEchoDelayFrequencyLowerLimit)
     return DELAY_BASED_ECHO_QUALITY_GOOD;
   else if (delay_frequency < kEchoDelayFrequencyUpperLimit)
     return DELAY_BASED_ECHO_QUALITY_SPURIOUS;
@@ -214,52 +218,59 @@ bool MediaAudioConstraints::GetDefaultValueForConstraint(
 }
 
 EchoInformation::EchoInformation()
-    : echo_poor_delay_counts_(0),
-      echo_total_delay_counts_(0),
-      last_log_time_(base::TimeTicks::Now()) {}
+    : num_chunks_(0),
+      num_queries_(0),
+      echo_fraction_poor_delays_(0.0f) {}
 
 EchoInformation::~EchoInformation() {}
 
-void EchoInformation::UpdateAecDelayStats(int delay) {
-  // One way to get an indication of how well the echo cancellation performs is
-  // to compare the, by AEC, estimated delay with the AEC filter length.
-  // |kMaxAecFilterLengthMs| is the maximum delay we can allow before we
-  // consider the AEC to fail. This value should not be larger than the filter
-  // length used inside AEC. This is for now set to match the extended filter
-  // mode which is turned on for all platforms.
-  const int kMaxAecFilterLengthMs = 128;
-  if ((delay < -2) || (delay > kMaxAecFilterLengthMs)) {
-    // The |delay| is out of bounds which indicates that the echo cancellation
-    // filter can not handle the echo. Hence, we have a potential full echo
-    // case. |delay| values {-1, -2} are reserved for errors.
-    ++echo_poor_delay_counts_;
+void EchoInformation::UpdateAecDelayStats(
+    webrtc::EchoCancellation* echo_cancellation) {
+  // In WebRTC, three echo delay metrics are calculated and updated every
+  // second. We use one of them, |fraction_poor_delays|, but aggregate over
+  // five seconds to log in a UMA histogram to monitor Echo Cancellation
+  // quality. Since the stat in WebRTC has a fixed aggregation window of one
+  // second we query the stat every second and average over five such queries.
+  // WebRTC process audio in 10 ms chunks.
+  const int kNumChunksInOneSecond = 100;
+  if (!echo_cancellation->is_delay_logging_enabled() ||
+      !echo_cancellation->is_enabled()) {
+    return;
+  }
+
+  num_chunks_++;
+  if (num_chunks_ < kNumChunksInOneSecond) {
+    return;
+  }
+
+  int dummy_median = 0, dummy_std = 0;
+  float fraction_poor_delays = 0;
+  if (!echo_cancellation->GetDelayMetrics(&dummy_median, &dummy_std,

[perkj_chrome, 2015/02/09 11:48:30]

Not from this cl I see but it looks quite weird to have a return value that is
false and at the same time trust an out parameter to that method. 

[bjornv, 2015/02/09 14:39:11]

Agree. In fact Karl would have killed me for not changing to use == instead.

+                                          &fraction_poor_delays)) {
+    echo_fraction_poor_delays_ += fraction_poor_delays;
+    num_queries_++;
+    num_chunks_ = 0;
   }
-  ++echo_total_delay_counts_;
   LogAecDelayStats();
 }
 
 void EchoInformation::LogAecDelayStats() {
   // We update the UMA statistics every 5 seconds.
-  const int kTimeBetweenLogsInSeconds = 5;
-  const base::TimeDelta time_since_last_log =
-      base::TimeTicks::Now() - last_log_time_;
-  if (time_since_last_log.InSeconds() < kTimeBetweenLogsInSeconds)
+  const int kNumQueriesIn5Seconds = 5;
+  if (num_queries_ < kNumQueriesIn5Seconds) {
     return;
+  }
 
   // Calculate how frequent the AEC delay was out of bounds since last time we
-  // updated UMA histograms. Then store the result into one of three histogram
-  // buckets; see DelayBasedEchoQuality.
-  float poor_delay_frequency = 0.f;
-  if (echo_total_delay_counts_ > 0) {
-    poor_delay_frequency = static_cast<float>(echo_poor_delay_counts_) /
-        static_cast<float>(echo_total_delay_counts_);
-    UMA_HISTOGRAM_ENUMERATION("WebRTC.AecDelayBasedQuality",
-                              EchoDelayFrequencyToQuality(poor_delay_frequency),
-                              DELAY_BASED_ECHO_QUALITY_MAX);
-  }
-  echo_poor_delay_counts_ = 0;
-  echo_total_delay_counts_ = 0;
-  last_log_time_ = base::TimeTicks::Now();
+  // updated UMA histograms by averaging |echo_fraction_poor_delays_| over
+  // |num_queries_|. Then store the result into one of four histogram buckets;
+  // see DelayBasedEchoQuality.
+  float poor_delay_frequency = echo_fraction_poor_delays_ / num_queries_;
+  UMA_HISTOGRAM_ENUMERATION("WebRTC.AecDelayBasedQuality",
+                            EchoDelayFrequencyToQuality(poor_delay_frequency),
+                            DELAY_BASED_ECHO_QUALITY_MAX);
+  num_queries_ = 0;
+  echo_fraction_poor_delays_ = 0.0f;
 }
 
 void EnableEchoCancellation(AudioProcessing* audio_processing) {
@@ -345,7 +356,7 @@ void GetAecStats(AudioProcessing* audio_processing,
   stats->echo_return_loss = -100;
   stats->echo_return_loss_enhancement = -100;
 
-  // These values can also be negative, but in practice -1 is only used to
+  // The median value can also be negative, but in practice -1 is only used to
   // signal insufficient data, since the resolution is limited to multiples
   // of 4ms.
   stats->echo_delay_median_ms = -1;
@@ -371,7 +382,9 @@ void GetAecStats(AudioProcessing* audio_processing,
   }
 
   int median = 0, std = 0;
-  if (!audio_processing->echo_cancellation()->GetDelayMetrics(&median, &std)) {
+  float dummy = 0;
+  if (!audio_processing->echo_cancellation()->GetDelayMetrics(
+      &median, &std, &dummy)) {
     stats->echo_delay_median_ms = median;
     stats->echo_delay_std_ms = std;
   }