From 35b3c63ba47cd56adb611a3584c057c3073d0249 Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Wed, 26 Oct 2022 13:25:44 +0000 Subject: [PATCH] Revert "APM: log both applied and recommended input volume stats" This reverts commit 8d7273357d92fab881561d886ce8dfe94e6e2238. Reason for revert: revert needed to land https://webrtc-review.googlesource.com/c/src/+/280600 Original change's description: > APM: log both applied and recommended input volume stats > > This CL replaces the existing `WebRTC.Audio.ApmAnalogGain.*` stats > with `WebRTC.Audio.Apm.AppliedInputVolume.*` and adds the > `WebRTC.Audio.Apm.RecommendedInputVolume.*` stats. > > Bug: webrtc:7494 > Change-Id: I70be710d20b1589fc814cbce3d3329ac1500686f > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280220 > Reviewed-by: Hanna Silen > Commit-Queue: Alessio Bazzica > Cr-Commit-Position: refs/heads/main@{#38468} Bug: webrtc:7494 Change-Id: I4a2acfd5a983d9397932b2879cfa057deaf0eb2b No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280581 Commit-Queue: Alessio Bazzica Auto-Submit: Alessio Bazzica Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#38476} --- modules/audio_processing/agc2/BUILD.gn | 2 - .../agc2/input_volume_stats_reporter.cc | 141 +++++------------- .../agc2/input_volume_stats_reporter.h | 16 +- .../input_volume_stats_reporter_unittest.cc | 119 +++++---------- .../audio_processing/audio_processing_impl.cc | 10 +- .../audio_processing/audio_processing_impl.h | 2 - 6 files changed, 74 insertions(+), 216 deletions(-) diff --git a/modules/audio_processing/agc2/BUILD.gn b/modules/audio_processing/agc2/BUILD.gn index 0e20ae6768..bc1507676d 100644 --- a/modules/audio_processing/agc2/BUILD.gn +++ b/modules/audio_processing/agc2/BUILD.gn @@ -436,9 +436,7 @@ rtc_library("input_volume_stats_reporter_unittests") { sources = [ "input_volume_stats_reporter_unittest.cc" ] deps = [ ":input_volume_stats_reporter", - "../../../rtc_base:stringutils", "../../../system_wrappers:metrics", "../../../test:test_support", ] - absl_deps = [ "//third_party/abseil-cpp/absl/strings" ] } diff --git a/modules/audio_processing/agc2/input_volume_stats_reporter.cc b/modules/audio_processing/agc2/input_volume_stats_reporter.cc index 6d70186697..4a8b016f45 100644 --- a/modules/audio_processing/agc2/input_volume_stats_reporter.cc +++ b/modules/audio_processing/agc2/input_volume_stats_reporter.cc @@ -19,8 +19,6 @@ namespace webrtc { namespace { -using InputVolumeType = InputVolumeStatsReporter::InputVolumeType; - constexpr int kFramesIn60Seconds = 6000; constexpr int kMinInputVolume = 0; constexpr int kMaxInputVolume = 255; @@ -37,102 +35,9 @@ float ComputeAverageUpdate(int sum_updates, int num_updates) { return std::round(static_cast(sum_updates) / static_cast(num_updates)); } - -metrics::Histogram* CreateRateHistogram(absl::string_view name) { - return metrics::HistogramFactoryGetCounts( - name, /*min=*/1, /*max=*/kFramesIn60Seconds, /*bucket_count=*/50); -} - -metrics::Histogram* CreateAverageHistogram(absl::string_view name) { - return metrics::HistogramFactoryGetCounts(name, /*min=*/1, /*max=*/kMaxUpdate, - /*bucket_count=*/50); -} - -metrics::Histogram* CreateDecreaseRateHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateRateHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.DecreaseRate"); - case InputVolumeType::kRecommended: - return CreateRateHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.DecreaseRate"); - } -} - -metrics::Histogram* CreateDecreaseAverageHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.DecreaseAverage"); - case InputVolumeType::kRecommended: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.DecreaseAverage"); - } -} - -metrics::Histogram* CreateIncreaseRateHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateRateHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.IncreaseRate"); - case InputVolumeType::kRecommended: - return CreateRateHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.IncreaseRate"); - } -} - -metrics::Histogram* CreateIncreaseAverageHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.IncreaseAverage"); - case InputVolumeType::kRecommended: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.IncreaseAverage"); - } -} - -metrics::Histogram* CreateUpdateRateHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateRateHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.UpdateRate"); - case InputVolumeType::kRecommended: - return CreateRateHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.UpdateRate"); - } -} - -metrics::Histogram* CreateUpdateAverageHistogram( - InputVolumeType input_volume_type) { - switch (input_volume_type) { - case InputVolumeType::kApplied: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.AppliedInputVolume.UpdateAverage"); - case InputVolumeType::kRecommended: - return CreateAverageHistogram( - "WebRTC.Audio.Apm.RecommendedInputVolume.UpdateAverage"); - } -} - } // namespace -InputVolumeStatsReporter::InputVolumeStatsReporter( - InputVolumeType input_volume_type) - : decrease_rate_histogram_(CreateDecreaseRateHistogram(input_volume_type)), - decrease_average_histogram_( - CreateDecreaseAverageHistogram(input_volume_type)), - increase_rate_histogram_(CreateIncreaseRateHistogram(input_volume_type)), - increase_average_histogram_( - CreateIncreaseAverageHistogram(input_volume_type)), - update_rate_histogram_(CreateUpdateRateHistogram(input_volume_type)), - update_average_histogram_( - CreateUpdateAverageHistogram(input_volume_type)) {} +InputVolumeStatsReporter::InputVolumeStatsReporter() = default; InputVolumeStatsReporter::~InputVolumeStatsReporter() = default; @@ -169,19 +74,47 @@ void InputVolumeStatsReporter::LogVolumeUpdateStats() const { const float average_update = ComputeAverageUpdate( volume_update_stats_.sum_decreases + volume_update_stats_.sum_increases, num_updates); - metrics::HistogramAdd(decrease_rate_histogram_, - volume_update_stats_.num_decreases); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainDecreaseRate", + /*sample=*/volume_update_stats_.num_decreases, + /*min=*/1, + /*max=*/kFramesIn60Seconds, + /*bucket_count=*/50); if (volume_update_stats_.num_decreases > 0) { - metrics::HistogramAdd(decrease_average_histogram_, average_decrease); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainDecreaseAverage", + /*sample=*/average_decrease, + /*min=*/1, + /*max=*/kMaxUpdate, + /*bucket_count=*/50); } - metrics::HistogramAdd(increase_rate_histogram_, - volume_update_stats_.num_increases); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainIncreaseRate", + /*sample=*/volume_update_stats_.num_increases, + /*min=*/1, + /*max=*/kFramesIn60Seconds, + /*bucket_count=*/50); if (volume_update_stats_.num_increases > 0) { - metrics::HistogramAdd(increase_average_histogram_, average_increase); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainIncreaseAverage", + /*sample=*/average_increase, + /*min=*/1, + /*max=*/kMaxUpdate, + /*bucket_count=*/50); } - metrics::HistogramAdd(update_rate_histogram_, num_updates); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainUpdateRate", + /*sample=*/num_updates, + /*min=*/1, + /*max=*/kFramesIn60Seconds, + /*bucket_count=*/50); if (num_updates > 0) { - metrics::HistogramAdd(update_average_histogram_, average_update); + RTC_HISTOGRAM_COUNTS_LINEAR( + /*name=*/"WebRTC.Audio.ApmAnalogGainUpdateAverage", + /*sample=*/average_update, + /*min=*/1, + /*max=*/kMaxUpdate, + /*bucket_count=*/50); } } diff --git a/modules/audio_processing/agc2/input_volume_stats_reporter.h b/modules/audio_processing/agc2/input_volume_stats_reporter.h index 757e6ff141..0040754209 100644 --- a/modules/audio_processing/agc2/input_volume_stats_reporter.h +++ b/modules/audio_processing/agc2/input_volume_stats_reporter.h @@ -13,7 +13,6 @@ #include "absl/types/optional.h" #include "rtc_base/gtest_prod_util.h" -#include "system_wrappers/include/metrics.h" namespace webrtc { @@ -22,12 +21,7 @@ namespace webrtc { // the statistics into a histogram. class InputVolumeStatsReporter { public: - enum class InputVolumeType { - kApplied = 0, - kRecommended = 1, - }; - - explicit InputVolumeStatsReporter(InputVolumeType input_volume_type); + InputVolumeStatsReporter(); InputVolumeStatsReporter(const InputVolumeStatsReporter&) = delete; InputVolumeStatsReporter operator=(const InputVolumeStatsReporter&) = delete; ~InputVolumeStatsReporter(); @@ -63,14 +57,6 @@ class InputVolumeStatsReporter { // Computes aggregate stat and logs them into a histogram. void LogVolumeUpdateStats() const; - // Histograms. - metrics::Histogram* decrease_rate_histogram_; - metrics::Histogram* decrease_average_histogram_; - metrics::Histogram* increase_rate_histogram_; - metrics::Histogram* increase_average_histogram_; - metrics::Histogram* update_rate_histogram_; - metrics::Histogram* update_average_histogram_; - int log_volume_update_stats_counter_ = 0; absl::optional previous_input_volume_ = absl::nullopt; }; diff --git a/modules/audio_processing/agc2/input_volume_stats_reporter_unittest.cc b/modules/audio_processing/agc2/input_volume_stats_reporter_unittest.cc index a3e2ccaf91..f8cd010784 100644 --- a/modules/audio_processing/agc2/input_volume_stats_reporter_unittest.cc +++ b/modules/audio_processing/agc2/input_volume_stats_reporter_unittest.cc @@ -10,71 +10,24 @@ #include "modules/audio_processing/agc2/input_volume_stats_reporter.h" -#include "absl/strings/string_view.h" -#include "rtc_base/strings/string_builder.h" #include "system_wrappers/include/metrics.h" #include "test/gmock.h" namespace webrtc { namespace { -using InputVolumeType = InputVolumeStatsReporter::InputVolumeType; - constexpr int kFramesIn60Seconds = 6000; -constexpr absl::string_view kLabelPrefix = "WebRTC.Audio.Apm."; - -class InputVolumeStatsReporterTest - : public ::testing::TestWithParam { +class InputVolumeStatsReporterTest : public ::testing::Test { public: - InputVolumeStatsReporterTest() { metrics::Reset(); } + InputVolumeStatsReporterTest() {} protected: - InputVolumeType InputVolumeType() const { return GetParam(); } - std::string DecreaseRateLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "DecreaseRate") - .str(); - } - std::string DecreaseAverageLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "DecreaseAverage") - .str(); - } - std::string IncreaseRateLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "IncreaseRate") - .str(); - } - std::string IncreaseAverageLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "IncreaseAverage") - .str(); - } - std::string UpdateRateLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "UpdateRate") - .str(); - } - std::string UpdateAverageLabel() const { - return (rtc::StringBuilder(kLabelPrefix) - << VolumeTypeLabel() << "UpdateAverage") - .str(); - } - - private: - absl::string_view VolumeTypeLabel() const { - switch (InputVolumeType()) { - case InputVolumeType::kApplied: - return "AppliedInputVolume."; - case InputVolumeType::kRecommended: - return "RecommendedInputVolume."; - } - } + void SetUp() override { metrics::Reset(); } }; -TEST_P(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsEmpty) { - InputVolumeStatsReporter stats_reporter(InputVolumeType()); +TEST_F(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsEmpty) { + InputVolumeStatsReporter stats_reporter; constexpr int kInputVolume = 10; stats_reporter.UpdateStatistics(kInputVolume); // Update almost until the periodic logging and reset. @@ -82,22 +35,25 @@ TEST_P(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsEmpty) { stats_reporter.UpdateStatistics(kInputVolume + 2); stats_reporter.UpdateStatistics(kInputVolume); } - EXPECT_METRIC_THAT(metrics::Samples(UpdateRateLabel()), + EXPECT_METRIC_THAT(metrics::Samples("WebRTC.Audio.ApmAnalogGainUpdateRate"), ::testing::ElementsAre()); - EXPECT_METRIC_THAT(metrics::Samples(DecreaseRateLabel()), + EXPECT_METRIC_THAT(metrics::Samples("WebRTC.Audio.ApmAnalogGainDecreaseRate"), ::testing::ElementsAre()); - EXPECT_METRIC_THAT(metrics::Samples(IncreaseRateLabel()), - ::testing::ElementsAre()); - EXPECT_METRIC_THAT(metrics::Samples(UpdateAverageLabel()), - ::testing::ElementsAre()); - EXPECT_METRIC_THAT(metrics::Samples(DecreaseAverageLabel()), - ::testing::ElementsAre()); - EXPECT_METRIC_THAT(metrics::Samples(IncreaseAverageLabel()), + EXPECT_METRIC_THAT(metrics::Samples("WebRTC.Audio.ApmAnalogGainIncreaseRate"), ::testing::ElementsAre()); + EXPECT_METRIC_THAT( + metrics::Samples("WebRTC.Audio.ApmAnalogGainUpdateAverage"), + ::testing::ElementsAre()); + EXPECT_METRIC_THAT( + metrics::Samples("WebRTC.Audio.ApmAnalogGainDecreaseAverage"), + ::testing::ElementsAre()); + EXPECT_METRIC_THAT( + metrics::Samples("WebRTC.Audio.ApmAnalogGainIncreaseAverage"), + ::testing::ElementsAre()); } -TEST_P(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsNotEmpty) { - InputVolumeStatsReporter stats_reporter(InputVolumeType()); +TEST_F(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsNotEmpty) { + InputVolumeStatsReporter stats_reporter; constexpr int kInputVolume = 10; stats_reporter.UpdateStatistics(kInputVolume); // Update until periodic logging. @@ -111,30 +67,30 @@ TEST_P(InputVolumeStatsReporterTest, CheckLogVolumeUpdateStatsNotEmpty) { stats_reporter.UpdateStatistics(kInputVolume); } EXPECT_METRIC_THAT( - metrics::Samples(UpdateRateLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainUpdateRate"), ::testing::ElementsAre(::testing::Pair(kFramesIn60Seconds - 1, 1), ::testing::Pair(kFramesIn60Seconds, 1))); EXPECT_METRIC_THAT( - metrics::Samples(DecreaseRateLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainDecreaseRate"), ::testing::ElementsAre(::testing::Pair(kFramesIn60Seconds / 2 - 1, 1), ::testing::Pair(kFramesIn60Seconds / 2, 1))); EXPECT_METRIC_THAT( - metrics::Samples(IncreaseRateLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainIncreaseRate"), ::testing::ElementsAre(::testing::Pair(kFramesIn60Seconds / 2, 2))); EXPECT_METRIC_THAT( - metrics::Samples(UpdateAverageLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainUpdateAverage"), ::testing::ElementsAre(::testing::Pair(2, 1), ::testing::Pair(3, 1))); EXPECT_METRIC_THAT( - metrics::Samples(DecreaseAverageLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainDecreaseAverage"), ::testing::ElementsAre(::testing::Pair(2, 1), ::testing::Pair(3, 1))); EXPECT_METRIC_THAT( - metrics::Samples(IncreaseAverageLabel()), + metrics::Samples("WebRTC.Audio.ApmAnalogGainIncreaseAverage"), ::testing::ElementsAre(::testing::Pair(2, 1), ::testing::Pair(3, 1))); } } // namespace -TEST_P(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsForEmptyStats) { - InputVolumeStatsReporter stats_reporter(InputVolumeType()); +TEST_F(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsForEmptyStats) { + InputVolumeStatsReporter stats_reporter; const auto& update_stats = stats_reporter.volume_update_stats(); EXPECT_EQ(update_stats.num_decreases, 0); EXPECT_EQ(update_stats.sum_decreases, 0); @@ -142,10 +98,10 @@ TEST_P(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsForEmptyStats) { EXPECT_EQ(update_stats.sum_increases, 0); } -TEST_P(InputVolumeStatsReporterTest, +TEST_F(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterNoVolumeChange) { constexpr int kInputVolume = 10; - InputVolumeStatsReporter stats_reporter(InputVolumeType()); + InputVolumeStatsReporter stats_reporter; stats_reporter.UpdateStatistics(kInputVolume); stats_reporter.UpdateStatistics(kInputVolume); stats_reporter.UpdateStatistics(kInputVolume); @@ -156,10 +112,10 @@ TEST_P(InputVolumeStatsReporterTest, EXPECT_EQ(update_stats.sum_increases, 0); } -TEST_P(InputVolumeStatsReporterTest, +TEST_F(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterVolumeIncrease) { constexpr int kInputVolume = 10; - InputVolumeStatsReporter stats_reporter(InputVolumeType()); + InputVolumeStatsReporter stats_reporter; stats_reporter.UpdateStatistics(kInputVolume); stats_reporter.UpdateStatistics(kInputVolume + 4); stats_reporter.UpdateStatistics(kInputVolume + 5); @@ -170,10 +126,10 @@ TEST_P(InputVolumeStatsReporterTest, EXPECT_EQ(update_stats.sum_increases, 5); } -TEST_P(InputVolumeStatsReporterTest, +TEST_F(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterVolumeDecrease) { constexpr int kInputVolume = 10; - InputVolumeStatsReporter stats_reporter(InputVolumeType()); + InputVolumeStatsReporter stats_reporter; stats_reporter.UpdateStatistics(kInputVolume); stats_reporter.UpdateStatistics(kInputVolume - 4); stats_reporter.UpdateStatistics(kInputVolume - 5); @@ -184,8 +140,8 @@ TEST_P(InputVolumeStatsReporterTest, EXPECT_EQ(stats_update.sum_increases, 0); } -TEST_P(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterReset) { - InputVolumeStatsReporter stats_reporter(InputVolumeType()); +TEST_F(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterReset) { + InputVolumeStatsReporter stats_reporter; constexpr int kInputVolume = 10; stats_reporter.UpdateStatistics(kInputVolume); // Update until the periodic reset. @@ -213,9 +169,4 @@ TEST_P(InputVolumeStatsReporterTest, CheckVolumeUpdateStatsAfterReset) { EXPECT_EQ(stats_after_reset.sum_increases, 3); } -INSTANTIATE_TEST_SUITE_P(, - InputVolumeStatsReporterTest, - ::testing::Values(InputVolumeType::kApplied, - InputVolumeType::kRecommended)); - } // namespace webrtc diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index a0415e2bc3..76e380426f 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -291,11 +291,7 @@ AudioProcessingImpl::AudioProcessingImpl( MinimizeProcessingForUnusedOutput(), field_trial::IsEnabled("WebRTC-TransientSuppressorForcedOff")), capture_(), - capture_nonlocked_(), - applied_input_volume_stats_reporter_( - InputVolumeStatsReporter::InputVolumeType::kApplied), - recommended_input_volume_stats_reporter_( - InputVolumeStatsReporter::InputVolumeType::kRecommended) { + capture_nonlocked_() { RTC_LOG(LS_INFO) << "Injected APM submodules:" "\nEcho control factory: " << !!echo_control_factory_ @@ -1365,10 +1361,6 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { stats_reporter_.UpdateStatistics(capture_.stats); UpdateRecommendedInputVolumeLocked(); - if (capture_.recommended_input_volume.has_value()) { - recommended_input_volume_stats_reporter_.UpdateStatistics( - *capture_.recommended_input_volume); - } if (submodules_.capture_levels_adjuster) { submodules_.capture_levels_adjuster->ApplyPostLevelAdjustment( diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 5daea9088a..bc582b542d 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -541,8 +541,6 @@ class AudioProcessingImpl : public AudioProcessing { InputVolumeStatsReporter applied_input_volume_stats_reporter_ RTC_GUARDED_BY(mutex_capture_); - InputVolumeStatsReporter recommended_input_volume_stats_reporter_ - RTC_GUARDED_BY(mutex_capture_); // Lock protection not needed. std::unique_ptr<