From d0a6fd239cef0d9fd5fdd5a41df389a696bff017 Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Wed, 13 Jul 2022 18:57:34 +0000 Subject: [PATCH] Revert "`AgcManagerDirect`: stop enforcing min mic level override with 0 level" This reverts commit e76daab8b36f8c2a16d59a116425a3a2f98022f6. Reason for revert: revert required to revert the parent CL Original change's description: > `AgcManagerDirect`: stop enforcing min mic level override with 0 level > > https://webrtc-review.googlesource.com/c/src/+/250141 introduced a bug > due to which the min mic level override is always enforced, if specified > even if the user manually adjusts the mic level to zero. > > This CL fixes that bug, the changes run behind a kill switch. > > TESTED=Test video call on Chromium on Mac; input volume not adjusted after zeroing it from the system preferences UI > > Bug: chromium:1275566 > Change-Id: I18ce2e5970d3002b301f51f84544583c64982d57 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267844 > Reviewed-by: Hanna Silen > Commit-Queue: Alessio Bazzica > Cr-Commit-Position: refs/heads/main@{#37460} Bug: chromium:1275566 Change-Id: I6d22d8f3fafdc7da3814827b9b69146a506595db Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/268468 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Alessio Bazzica Cr-Commit-Position: refs/heads/main@{#37515} --- .../agc/agc_manager_direct.cc | 24 ++++--------------- .../audio_processing/agc/agc_manager_direct.h | 1 - .../agc/agc_manager_direct_unittest.cc | 14 +++++++++++ 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/modules/audio_processing/agc/agc_manager_direct.cc b/modules/audio_processing/agc/agc_manager_direct.cc index f385438249..73bf183a2b 100644 --- a/modules/audio_processing/agc/agc_manager_direct.cc +++ b/modules/audio_processing/agc/agc_manager_direct.cc @@ -62,23 +62,12 @@ bool UseMaxAnalogChannelLevel() { return field_trial::IsEnabled("WebRTC-UseMaxAnalogAgcChannelLevel"); } -// Minimum mic level override. -// `kMinMicLevelOverride`: if specified, the override replaces `kMinMicLevel` -// and it is applied even when clipping is detected. -// `EnforceMinMicLevelOverrideOnZeroLevel()`: returns true if the analog -// controller must enforce the minimum mic level override even when the mic -// level has manually been set to zero. +// Minimum mic level override. If specified, the override replaces +// `kMinMicLevel` and it is applied even when clipping is detected. #if defined(WEBRTC_MAC) constexpr absl::optional kMinMicLevelOverride = 20; -bool EnforceMinMicLevelOverrideOnZeroLevel() { - return field_trial::IsEnabled( - "WebRTC-Audio-AgcAnalogFixZeroMicLevelBugKillSwitch"); -} #else constexpr absl::optional kMinMicLevelOverride = absl::nullopt; -constexpr bool EnforceMinMicLevelOverrideOnZeroLevel() { - return false; -} #endif int ClampLevel(int mic_level, int min_mic_level) { @@ -466,8 +455,6 @@ AgcManagerDirect::AgcManagerDirect( int clipped_wait_frames, const ClippingPredictorConfig& clipping_config) : min_mic_level_override_(kMinMicLevelOverride), - enforce_min_mic_level_override_on_zero_level_( - EnforceMinMicLevelOverrideOnZeroLevel()), data_dumper_(new ApmDataDumper(instance_counter_.fetch_add(1) + 1)), use_min_channel_level_(!UseMaxAnalogChannelLevel()), num_capture_channels_(num_capture_channels), @@ -719,10 +706,9 @@ void AgcManagerDirect::AggregateChannelLevels() { } } } - - if (min_mic_level_override_.has_value() && - (enforce_min_mic_level_override_on_zero_level_ || - stream_analog_level_ > 0)) { + // TODO(crbug.com/1275566): Do not enforce minimum if the user has manually + // set the mic level to zero. + if (min_mic_level_override_.has_value()) { stream_analog_level_ = std::max(stream_analog_level_, *min_mic_level_override_); } diff --git a/modules/audio_processing/agc/agc_manager_direct.h b/modules/audio_processing/agc/agc_manager_direct.h index a437206ceb..86abeecb91 100644 --- a/modules/audio_processing/agc/agc_manager_direct.h +++ b/modules/audio_processing/agc/agc_manager_direct.h @@ -128,7 +128,6 @@ class AgcManagerDirect final { void AggregateChannelLevels(); const absl::optional min_mic_level_override_; - const bool enforce_min_mic_level_override_on_zero_level_; std::unique_ptr data_dumper_; static std::atomic instance_counter_; const bool use_min_channel_level_; diff --git a/modules/audio_processing/agc/agc_manager_direct_unittest.cc b/modules/audio_processing/agc/agc_manager_direct_unittest.cc index adbb2f272c..dce6aaa675 100644 --- a/modules/audio_processing/agc/agc_manager_direct_unittest.cc +++ b/modules/audio_processing/agc/agc_manager_direct_unittest.cc @@ -838,6 +838,19 @@ TEST_F(AgcManagerDirectTest, ClippingDoesNotPullLowVolumeBackUp) { EXPECT_EQ(initial_volume, manager_.stream_analog_level()); } +#if defined(WEBRTC_MAC) +// TODO(crbug.com/1275566): Fix AGC, remove test below. +TEST_F(AgcManagerDirectTest, BumpsToMinLevelOnZeroMicVolume) { + FirstProcess(); + + EXPECT_CALL(*agc_, GetRmsErrorDb(_)) + .WillRepeatedly(DoAll(SetArgPointee<0>(30), Return(true))); + manager_.set_stream_analog_level(0); + CallProcess(10); + EXPECT_EQ(20, manager_.stream_analog_level()); +} +#else +// TODO(crbug.com/1275566): Fix AGC, reenable test below on Mac. TEST_F(AgcManagerDirectTest, TakesNoActionOnZeroMicVolume) { FirstProcess(); @@ -847,6 +860,7 @@ TEST_F(AgcManagerDirectTest, TakesNoActionOnZeroMicVolume) { CallProcess(10); EXPECT_EQ(0, manager_.stream_analog_level()); } +#endif TEST_F(AgcManagerDirectTest, ClippingDetectionLowersVolume) { SetVolumeAndProcess(255);