AEC3: clarify render delay controller metrics

This CL:
- makes it easier to understand the (nontrivial) metric interpretation
- corrects the computation of BufferDelay to use 0 for absent delay
- deletes metric MaxSkewShiftCount, unused since https://webrtc-review.googlesource.com/c/src/+/119701
- updates the unit test to directly test metric reporting

Corresponding update to histograms.xml:
https://crrev.com/c/3944909

Bug: webrtc:8671, chromium:1349051
Change-Id: If73b6fca4de7343bff2c53f72cedda458d36c599
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278782
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38362}

modules/audio_processing/aec3/render_delay_controller.cc

@@ -155,9 +155,11 @@ absl::optional<DelayEstimate> RenderDelayControllerImpl::GetDelay(
     last_delay_estimate_quality_ = delay_samples_->quality;
   }
 
-  metrics_.Update(delay_samples_ ? absl::optional<size_t>(delay_samples_->delay)
+  metrics_.Update(
-                                 : absl::nullopt,
+      delay_samples_ ? absl::optional<size_t>(delay_samples_->delay)
-                  delay_ ? delay_->delay : 0, 0, delay_estimator_.Clockdrift());
+                     : absl::nullopt,
+      delay_ ? absl::optional<size_t>(delay_->delay) : absl::nullopt,
+      delay_estimator_.Clockdrift());
 
   data_dumper_->DumpRaw("aec3_render_delay_controller_delay",
                         delay_samples ? delay_samples->delay : 0);

modules/audio_processing/aec3/render_delay_controller_metrics.cc

@@ -37,16 +37,13 @@ enum class DelayChangesCategory {
   kNumCategories
 };
 
-constexpr int kMaxSkewShiftCount = 20;
-
 }  // namespace
 
 RenderDelayControllerMetrics::RenderDelayControllerMetrics() = default;
 
 void RenderDelayControllerMetrics::Update(
     absl::optional<size_t> delay_samples,
-    size_t buffer_delay_blocks,
+    absl::optional<size_t> buffer_delay_blocks,
-    absl::optional<int> skew_shift_blocks,
     ClockdriftDetector::Level clockdrift) {
   ++call_counter_;
 
@@ -54,6 +51,8 @@ void RenderDelayControllerMetrics::Update(
     size_t delay_blocks;
     if (delay_samples) {
       ++reliable_delay_estimate_counter_;
+      // Add an offset by 1 (metric is halved before reporting) to reserve 0 for
+      // absent delay.
       delay_blocks = (*delay_samples) / kBlockSize + 2;
     } else {
       delay_blocks = 0;
@@ -64,21 +63,21 @@ void RenderDelayControllerMetrics::Update(
       delay_blocks_ = delay_blocks;
     }
 
-    if (skew_shift_blocks) {
-      skew_shift_count_ = std::min(kMaxSkewShiftCount, skew_shift_count_);
-    }
   } else if (++initial_call_counter_ == 5 * kNumBlocksPerSecond) {
     initial_update = false;
   }
 
   if (call_counter_ == kMetricsReportingIntervalBlocks) {
     int value_to_report = static_cast<int>(delay_blocks_);
+    // Divide by 2 to compress metric range.
     value_to_report = std::min(124, value_to_report >> 1);
     RTC_HISTOGRAM_COUNTS_LINEAR("WebRTC.Audio.EchoCanceller.EchoPathDelay",
                                 value_to_report, 0, 124, 125);
 
-    value_to_report = static_cast<int>(buffer_delay_blocks + 2);
+    // Divide by 2 to compress metric range.
-    value_to_report = std::min(124, value_to_report >> 1);
+    // Offset by 1 to reserve 0 for absent delay.
+    value_to_report = buffer_delay_blocks ? (*buffer_delay_blocks + 2) >> 1 : 0;
+    value_to_report = std::min(124, value_to_report);
     RTC_HISTOGRAM_COUNTS_LINEAR("WebRTC.Audio.EchoCanceller.BufferDelay",
                                 value_to_report, 0, 124, 125);
 
@@ -120,20 +119,8 @@ void RenderDelayControllerMetrics::Update(
         "WebRTC.Audio.EchoCanceller.Clockdrift", static_cast<int>(clockdrift),
         static_cast<int>(ClockdriftDetector::Level::kNumCategories));
 
-    metrics_reported_ = true;
     call_counter_ = 0;
     ResetMetrics();
-  } else {
-    metrics_reported_ = false;
-  }
-
-  if (!initial_update && ++skew_report_timer_ == 60 * kNumBlocksPerSecond) {
-    RTC_HISTOGRAM_COUNTS_LINEAR("WebRTC.Audio.EchoCanceller.MaxSkewShiftCount",
-                                skew_shift_count_, 0, kMaxSkewShiftCount,
-                                kMaxSkewShiftCount + 1);
-
-    skew_shift_count_ = 0;
-    skew_report_timer_ = 0;
   }
 }
 

modules/audio_processing/aec3/render_delay_controller_metrics.h

@@ -29,13 +29,9 @@ class RenderDelayControllerMetrics {
 
   // Updates the metric with new data.
   void Update(absl::optional<size_t> delay_samples,
-              size_t buffer_delay_blocks,
+              absl::optional<size_t> buffer_delay_blocks,
-              absl::optional<int> skew_shift_blocks,
               ClockdriftDetector::Level clockdrift);
 
-  // Returns true if the metrics have just been reported, otherwise false.
-  bool MetricsReported() { return metrics_reported_; }
-
  private:
   // Resets the metrics.
   void ResetMetrics();
@@ -44,11 +40,8 @@ class RenderDelayControllerMetrics {
   int reliable_delay_estimate_counter_ = 0;
   int delay_change_counter_ = 0;
   int call_counter_ = 0;
-  int skew_report_timer_ = 0;
   int initial_call_counter_ = 0;
-  bool metrics_reported_ = false;
   bool initial_update = true;
-  int skew_shift_count_ = 0;
 };
 
 }  // namespace webrtc

modules/audio_processing/aec3/render_delay_controller_metrics_unittest.cc

@@ -12,6 +12,7 @@
 
 #include "absl/types/optional.h"
 #include "modules/audio_processing/aec3/aec3_common.h"
+#include "system_wrappers/include/metrics.h"
 #include "test/gtest.h"
 
 namespace webrtc {
@@ -20,15 +21,49 @@ namespace webrtc {
 TEST(RenderDelayControllerMetrics, NormalUsage) {
   RenderDelayControllerMetrics metrics;
 
+  int expected_num_metric_reports = 0;
+
   for (int j = 0; j < 3; ++j) {
     for (int k = 0; k < kMetricsReportingIntervalBlocks - 1; ++k) {
-      metrics.Update(absl::nullopt, 0, absl::nullopt,
+      metrics.Update(absl::nullopt, absl::nullopt,
                      ClockdriftDetector::Level::kNone);
-      EXPECT_FALSE(metrics.MetricsReported());
+      EXPECT_METRIC_EQ(
+          metrics::NumSamples("WebRTC.Audio.EchoCanceller.EchoPathDelay"),
+          expected_num_metric_reports);
+      EXPECT_METRIC_EQ(
+          metrics::NumSamples("WebRTC.Audio.EchoCanceller.BufferDelay"),
+          expected_num_metric_reports);
+      EXPECT_METRIC_EQ(metrics::NumSamples(
+                           "WebRTC.Audio.EchoCanceller.ReliableDelayEstimates"),
+                       expected_num_metric_reports);
+      EXPECT_METRIC_EQ(
+          metrics::NumSamples("WebRTC.Audio.EchoCanceller.DelayChanges"),
+          expected_num_metric_reports);
+      EXPECT_METRIC_EQ(
+          metrics::NumSamples("WebRTC.Audio.EchoCanceller.Clockdrift"),
+          expected_num_metric_reports);
     }
-    metrics.Update(absl::nullopt, 0, absl::nullopt,
+
+    // We expect metric reports every kMetricsReportingIntervalBlocks blocks.
+    ++expected_num_metric_reports;
+
+    metrics.Update(absl::nullopt, absl::nullopt,
                    ClockdriftDetector::Level::kNone);
-    EXPECT_TRUE(metrics.MetricsReported());
+    EXPECT_METRIC_EQ(
+        metrics::NumSamples("WebRTC.Audio.EchoCanceller.EchoPathDelay"),
+        expected_num_metric_reports);
+    EXPECT_METRIC_EQ(
+        metrics::NumSamples("WebRTC.Audio.EchoCanceller.BufferDelay"),
+        expected_num_metric_reports);
+    EXPECT_METRIC_EQ(metrics::NumSamples(
+                         "WebRTC.Audio.EchoCanceller.ReliableDelayEstimates"),
+                     expected_num_metric_reports);
+    EXPECT_METRIC_EQ(
+        metrics::NumSamples("WebRTC.Audio.EchoCanceller.DelayChanges"),
+        expected_num_metric_reports);
+    EXPECT_METRIC_EQ(
+        metrics::NumSamples("WebRTC.Audio.EchoCanceller.Clockdrift"),
+        expected_num_metric_reports);
   }
 }