Revert "AEC3: clarify render delay controller metrics"

This reverts commit fd745d3e3c7083cfa52307b9e4fc908930ddf2d2.

Reason for revert: Breaks downstream projects.

Original change's description:
> 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}

Bug: webrtc:8671, chromium:1349051
Change-Id: I1e2bd0f91acb67532e21f5d2f8526a398711a413
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/279040
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38367}

4 files changed, 36 insertions, 53 deletions

diff --git a/modules/audio_processing/aec3/render_delay_controller.cc b/modules/audio_processing/aec3/render_delay_controller.cc
index 465e77fb7c..7fd2868c38 100644
--- a/modules/audio_processing/aec3/render_delay_controller.cc
+++ b/modules/audio_processing/aec3/render_delay_controller.cc
@@ -155,11 +155,9 @@ absl::optional<DelayEstimate> RenderDelayControllerImpl::GetDelay(
     last_delay_estimate_quality_ = delay_samples_->quality;
   }
 
-  metrics_.Update(
-      delay_samples_ ? absl::optional<size_t>(delay_samples_->delay)
-                     : absl::nullopt,
-      delay_ ? absl::optional<size_t>(delay_->delay) : absl::nullopt,
-      delay_estimator_.Clockdrift());
+  metrics_.Update(delay_samples_ ? absl::optional<size_t>(delay_samples_->delay)
+                                 : absl::nullopt,
+                  delay_ ? delay_->delay : 0, 0, delay_estimator_.Clockdrift());
 
   data_dumper_->DumpRaw("aec3_render_delay_controller_delay",
                         delay_samples ? delay_samples->delay : 0);
diff --git a/modules/audio_processing/aec3/render_delay_controller_metrics.cc b/modules/audio_processing/aec3/render_delay_controller_metrics.cc
index 1e0a0f443e..582e033482 100644
--- a/modules/audio_processing/aec3/render_delay_controller_metrics.cc
+++ b/modules/audio_processing/aec3/render_delay_controller_metrics.cc
@@ -37,13 +37,16 @@ enum class DelayChangesCategory {
   kNumCategories
 };
 
+constexpr int kMaxSkewShiftCount = 20;
+
 }  // namespace
 
 RenderDelayControllerMetrics::RenderDelayControllerMetrics() = default;
 
 void RenderDelayControllerMetrics::Update(
     absl::optional<size_t> delay_samples,
-    absl::optional<size_t> buffer_delay_blocks,
+    size_t buffer_delay_blocks,
+    absl::optional<int> skew_shift_blocks,
     ClockdriftDetector::Level clockdrift) {
   ++call_counter_;
 
@@ -51,8 +54,6 @@ 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;
@@ -63,21 +64,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);
 
-    // Divide by 2 to compress metric range.
-    // 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);
+    value_to_report = static_cast<int>(buffer_delay_blocks + 2);
+    value_to_report = std::min(124, value_to_report >> 1);
     RTC_HISTOGRAM_COUNTS_LINEAR("WebRTC.Audio.EchoCanceller.BufferDelay",
                                 value_to_report, 0, 124, 125);
 
@@ -119,8 +120,20 @@ 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;
   }
 }
 
diff --git a/modules/audio_processing/aec3/render_delay_controller_metrics.h b/modules/audio_processing/aec3/render_delay_controller_metrics.h
index b81833b43f..309122d80d 100644
--- a/modules/audio_processing/aec3/render_delay_controller_metrics.h
+++ b/modules/audio_processing/aec3/render_delay_controller_metrics.h
@@ -29,9 +29,13 @@ class RenderDelayControllerMetrics {
 
   // Updates the metric with new data.
   void Update(absl::optional<size_t> delay_samples,
-              absl::optional<size_t> buffer_delay_blocks,
+              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();
@@ -40,8 +44,11 @@ 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
diff --git a/modules/audio_processing/aec3/render_delay_controller_metrics_unittest.cc b/modules/audio_processing/aec3/render_delay_controller_metrics_unittest.cc
index 2d242336f2..e7d7703433 100644
--- a/modules/audio_processing/aec3/render_delay_controller_metrics_unittest.cc
+++ b/modules/audio_processing/aec3/render_delay_controller_metrics_unittest.cc
@@ -12,7 +12,6 @@
 
 #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 {
@@ -21,49 +20,15 @@ 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, absl::nullopt,
+      metrics.Update(absl::nullopt, 0, absl::nullopt,
                      ClockdriftDetector::Level::kNone);
-      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);
+      EXPECT_FALSE(metrics.MetricsReported());
     }
-
-    // We expect metric reports every kMetricsReportingIntervalBlocks blocks.
-    ++expected_num_metric_reports;
-
-    metrics.Update(absl::nullopt, absl::nullopt,
+    metrics.Update(absl::nullopt, 0, absl::nullopt,
                    ClockdriftDetector::Level::kNone);
-    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);
+    EXPECT_TRUE(metrics.MetricsReported());
   }
 }