From f3c86154d4201d165592a54282d3c16851e63717 Mon Sep 17 00:00:00 2001 From: Alessio Bazzica Date: Thu, 14 Jul 2022 11:54:28 +0200 Subject: [PATCH] Revert "Min mic analog level: override minimum and behavior on Mac" This reverts commit c9cad23274a837b135db98c6ce96f85bbbc81604. Reason for revert: add back field trial Original change's description: > Min mic analog level: override minimum and behavior on Mac > > This CL removes the `WebRTC-Audio-AgcMinMicLevelExperiment` field trial > and always enables the code path behind that flag on Mac. In summary, > the analog AGC behaves as follows on Mac: > 1. the minimum level is overridden to 20 > 2. the minimum is applied even when clipping is detected > 3. when the level is manually adjusted to 0, the minimum level is > enforced - i.e., 20 > > Note that the 3rd property had been unintentionally added when the > changes were added behind the aforementioned field trial. This will > be fixed in a follow-up CL. > > Bug: chromium:1275566 > Change-Id: If184c4455a0780fcd94f55141af34460c152e3c3 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266488 > Commit-Queue: Alessio Bazzica > Reviewed-by: Hanna Silen > Cr-Commit-Position: refs/heads/main@{#37459} Bug: chromium:1275566 Change-Id: I00a37ad9e16efc49f721558d25af16efd5f3db8c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/268540 Commit-Queue: Alessio Bazzica Reviewed-by: Hanna Silen Cr-Commit-Position: refs/heads/main@{#37521} --- .../agc/agc_manager_direct.cc | 32 ++- .../agc/agc_manager_direct_unittest.cc | 233 ++++++++++++++---- 2 files changed, 215 insertions(+), 50 deletions(-) diff --git a/modules/audio_processing/agc/agc_manager_direct.cc b/modules/audio_processing/agc/agc_manager_direct.cc index 73bf183a2b..8bc6de979f 100644 --- a/modules/audio_processing/agc/agc_manager_direct.cc +++ b/modules/audio_processing/agc/agc_manager_direct.cc @@ -62,13 +62,29 @@ bool UseMaxAnalogChannelLevel() { return field_trial::IsEnabled("WebRTC-UseMaxAnalogAgcChannelLevel"); } -// 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; -#else -constexpr absl::optional kMinMicLevelOverride = absl::nullopt; -#endif +// If the "WebRTC-Audio-AgcMinMicLevelExperiment" field trial is specified, +// parses it and returns a value between 0 and 255 depending on the field-trial +// string. Returns an unspecified value if the field trial is not specified, if +// disabled or if it cannot be parsed. Example: +// 'WebRTC-Audio-AgcMinMicLevelExperiment/Enabled-80' => returns 80. +absl::optional GetMinMicLevelOverride() { + constexpr char kMinMicLevelFieldTrial[] = + "WebRTC-Audio-AgcMinMicLevelExperiment"; + if (!webrtc::field_trial::IsEnabled(kMinMicLevelFieldTrial)) { + return absl::nullopt; + } + const auto field_trial_string = + webrtc::field_trial::FindFullName(kMinMicLevelFieldTrial); + int min_mic_level = -1; + sscanf(field_trial_string.c_str(), "Enabled-%d", &min_mic_level); + if (min_mic_level >= 0 && min_mic_level <= 255) { + return min_mic_level; + } else { + RTC_LOG(LS_WARNING) << "[agc] Invalid parameter for " + << kMinMicLevelFieldTrial << ", ignored."; + return absl::nullopt; + } +} int ClampLevel(int mic_level, int min_mic_level) { return rtc::SafeClamp(mic_level, min_mic_level, kMaxMicLevel); @@ -454,7 +470,7 @@ AgcManagerDirect::AgcManagerDirect( float clipped_ratio_threshold, int clipped_wait_frames, const ClippingPredictorConfig& clipping_config) - : min_mic_level_override_(kMinMicLevelOverride), + : min_mic_level_override_(GetMinMicLevelOverride()), data_dumper_(new ApmDataDumper(instance_counter_.fetch_add(1) + 1)), use_min_channel_level_(!UseMaxAnalogChannelLevel()), num_capture_channels_(num_capture_channels), diff --git a/modules/audio_processing/agc/agc_manager_direct_unittest.cc b/modules/audio_processing/agc/agc_manager_direct_unittest.cc index dce6aaa675..5c9b383f78 100644 --- a/modules/audio_processing/agc/agc_manager_direct_unittest.cc +++ b/modules/audio_processing/agc/agc_manager_direct_unittest.cc @@ -35,6 +35,7 @@ constexpr int kSamplesPerChannel = kSampleRateHz / 100; constexpr int kInitialVolume = 128; constexpr int kClippedMin = 165; // Arbitrary, but different from the default. constexpr float kAboveClippedThreshold = 0.2f; +constexpr int kMinMicLevel = 12; constexpr int kClippedLevelStep = 15; constexpr float kClippedRatioThreshold = 0.1f; constexpr int kClippedWaitFrames = 300; @@ -127,6 +128,18 @@ void CallPreProcessAudioBuffer(int num_calls, } } +std::string GetAgcMinMicLevelExperimentFieldTrial( + int enabled_value, + const std::string& suffix = "") { + RTC_DCHECK_GE(enabled_value, 0); + RTC_DCHECK_LE(enabled_value, 255); + char field_trial_buffer[64]; + rtc::SimpleStringBuilder builder(field_trial_buffer); + builder << "WebRTC-Audio-AgcMinMicLevelExperiment/Enabled-" << enabled_value + << suffix << "/"; + return builder.str(); +} + // (Over)writes `samples_value` for the samples in `audio_buffer`. // When `clipped_ratio`, a value in [0, 1], is greater than 0, the corresponding // fraction of the frame is set to a full scale value to simulate clipping. @@ -151,6 +164,15 @@ void WriteAudioBufferSamples(float samples_value, } } +void CallPreProcessAndProcess(int num_calls, + const AudioBuffer& audio_buffer, + AgcManagerDirect& manager) { + for (int n = 0; n < num_calls; ++n) { + manager.AnalyzePreProcess(&audio_buffer); + manager.Process(&audio_buffer); + } +} + } // namespace class AgcManagerDirectTest : public ::testing::Test { @@ -369,21 +391,13 @@ TEST_F(AgcManagerDirectTest, MicVolumeIsLimited) { EXPECT_CALL(*agc_, GetRmsErrorDb(_)) .WillOnce(DoAll(SetArgPointee<0>(-40), Return(true))); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(18, manager_.stream_analog_level()); -#endif // Won't go lower than the minimum. EXPECT_CALL(*agc_, GetRmsErrorDb(_)) .WillOnce(DoAll(SetArgPointee<0>(-40), Return(true))); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(12, manager_.stream_analog_level()); -#endif } TEST_F(AgcManagerDirectTest, CompressorStepsTowardsTarget) { @@ -535,11 +549,7 @@ TEST_F(AgcManagerDirectTest, UnmutingRaisesTooLowVolume) { ExpectCheckVolumeAndReset(11); EXPECT_CALL(*agc_, GetRmsErrorDb(_)).WillOnce(Return(false)); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(12, manager_.stream_analog_level()); -#endif } TEST_F(AgcManagerDirectTest, ManualLevelChangeResultsInNoSetMicCall) { @@ -614,39 +624,23 @@ TEST_F(AgcManagerDirectTest, RecoveryAfterManualLevelChangeBelowMin) { manager_.set_stream_analog_level(1); EXPECT_CALL(*agc_, Reset()).Times(AtLeast(1)); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(1, manager_.stream_analog_level()); -#endif // Continues working as usual afterwards. EXPECT_CALL(*agc_, GetRmsErrorDb(_)) .WillOnce(DoAll(SetArgPointee<0>(11), Return(true))); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(2, manager_.stream_analog_level()); -#endif EXPECT_CALL(*agc_, GetRmsErrorDb(_)) .WillOnce(DoAll(SetArgPointee<0>(30), Return(true))); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(11, manager_.stream_analog_level()); -#endif EXPECT_CALL(*agc_, GetRmsErrorDb(_)) .WillOnce(DoAll(SetArgPointee<0>(20), Return(true))); CallProcess(1); -#if defined(WEBRTC_MAC) - EXPECT_EQ(20, manager_.stream_analog_level()); -#else EXPECT_EQ(18, manager_.stream_analog_level()); -#endif } TEST_F(AgcManagerDirectTest, NoClippingHasNoImpact) { @@ -838,19 +832,6 @@ 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(); @@ -860,7 +841,6 @@ TEST_F(AgcManagerDirectTest, TakesNoActionOnZeroMicVolume) { CallProcess(10); EXPECT_EQ(0, manager_.stream_analog_level()); } -#endif TEST_F(AgcManagerDirectTest, ClippingDetectionLowersVolume) { SetVolumeAndProcess(255); @@ -896,6 +876,175 @@ TEST(AgcManagerDirectStandaloneTest, DisableDigitalDisablesDigital) { manager->SetupDigitalGainControl(&gctrl); } +TEST(AgcManagerDirectStandaloneTest, AgcMinMicLevelExperiment) { + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + EXPECT_EQ(manager->channel_agcs_[0]->min_mic_level(), kMinMicLevel); + EXPECT_EQ(manager->channel_agcs_[0]->startup_min_level(), kInitialVolume); +} + +TEST(AgcManagerDirectStandaloneTest, AgcMinMicLevelExperimentDisabled) { + for (const std::string& field_trial_suffix : {"", "_20220210"}) { + test::ScopedFieldTrials field_trial( + "WebRTC-Audio-AgcMinMicLevelExperiment/Disabled" + field_trial_suffix + + "/"); + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + EXPECT_EQ(manager->channel_agcs_[0]->min_mic_level(), kMinMicLevel); + EXPECT_EQ(manager->channel_agcs_[0]->startup_min_level(), kInitialVolume); + } +} + +// Checks that a field-trial parameter outside of the valid range [0,255] is +// ignored. +TEST(AgcManagerDirectStandaloneTest, AgcMinMicLevelExperimentOutOfRangeAbove) { + test::ScopedFieldTrials field_trial( + "WebRTC-Audio-AgcMinMicLevelExperiment/Enabled-256/"); + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + EXPECT_EQ(manager->channel_agcs_[0]->min_mic_level(), kMinMicLevel); + EXPECT_EQ(manager->channel_agcs_[0]->startup_min_level(), kInitialVolume); +} + +// Checks that a field-trial parameter outside of the valid range [0,255] is +// ignored. +TEST(AgcManagerDirectStandaloneTest, AgcMinMicLevelExperimentOutOfRangeBelow) { + test::ScopedFieldTrials field_trial( + "WebRTC-Audio-AgcMinMicLevelExperiment/Enabled--1/"); + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + EXPECT_EQ(manager->channel_agcs_[0]->min_mic_level(), kMinMicLevel); + EXPECT_EQ(manager->channel_agcs_[0]->startup_min_level(), kInitialVolume); +} + +// Verifies that a valid experiment changes the minimum microphone level. The +// start volume is larger than the min level and should therefore not be +// changed. +TEST(AgcManagerDirectStandaloneTest, AgcMinMicLevelExperimentEnabled50) { + constexpr int kMinMicLevelOverride = 50; + for (const std::string& field_trial_suffix : {"", "_20220210"}) { + SCOPED_TRACE(field_trial_suffix); + test::ScopedFieldTrials field_trial(GetAgcMinMicLevelExperimentFieldTrial( + kMinMicLevelOverride, field_trial_suffix)); + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + EXPECT_EQ(manager->channel_agcs_[0]->min_mic_level(), kMinMicLevelOverride); + EXPECT_EQ(manager->channel_agcs_[0]->startup_min_level(), kInitialVolume); + } +} + +// Checks that, when the "WebRTC-Audio-AgcMinMicLevelExperiment" field trial is +// specified with a valid value, the mic level never gets lowered beyond the +// override value in the presence of clipping. +TEST(AgcManagerDirectStandaloneTest, + AgcMinMicLevelExperimentCheckMinLevelWithClipping) { + constexpr int kMinMicLevelOverride = 250; + + // Create and initialize two AGCs by specifying and leaving unspecified the + // relevant field trial. + const auto factory = []() { + std::unique_ptr manager = + CreateAgcManagerDirect(kInitialVolume, kClippedLevelStep, + kClippedRatioThreshold, kClippedWaitFrames); + manager->Initialize(); + manager->set_stream_analog_level(kInitialVolume); + return manager; + }; + std::unique_ptr manager = factory(); + std::unique_ptr manager_with_override; + { + test::ScopedFieldTrials field_trial( + GetAgcMinMicLevelExperimentFieldTrial(kMinMicLevelOverride)); + manager_with_override = factory(); + } + + // Create a test input signal which containts 80% of clipped samples. + AudioBuffer audio_buffer(kSampleRateHz, 1, kSampleRateHz, 1, kSampleRateHz, + 1); + WriteAudioBufferSamples(/*samples_value=*/4000.0f, /*clipped_ratio=*/0.8f, + audio_buffer); + + // Simulate 4 seconds of clipping; it is expected to trigger a downward + // adjustment of the analog gain. + CallPreProcessAndProcess(/*num_calls=*/400, audio_buffer, *manager); + CallPreProcessAndProcess(/*num_calls=*/400, audio_buffer, + *manager_with_override); + + // Make sure that an adaptation occurred. + ASSERT_GT(manager->stream_analog_level(), 0); + + // Check that the test signal triggers a larger downward adaptation for + // `manager`, which is allowed to reach a lower gain. + EXPECT_GT(manager_with_override->stream_analog_level(), + manager->stream_analog_level()); + // Check that the gain selected by `manager_with_override` equals the minimum + // value overridden via field trial. + EXPECT_EQ(manager_with_override->stream_analog_level(), kMinMicLevelOverride); +} + +// Checks that, when the "WebRTC-Audio-AgcMinMicLevelExperiment" field trial is +// specified with a value lower than the `clipped_level_min`, the behavior of +// the analog gain controller is the same as that obtained when the field trial +// is not specified. +TEST(AgcManagerDirectStandaloneTest, + AgcMinMicLevelExperimentCompareMicLevelWithClipping) { + // Create and initialize two AGCs by specifying and leaving unspecified the + // relevant field trial. + const auto factory = []() { + // Use a large clipped level step to more quickly decrease the analog gain + // with clipping. + auto controller = std::make_unique( + /*num_capture_channels=*/1, kInitialVolume, + kDefaultAnalogConfig.clipped_level_min, + /*disable_digital_adaptive=*/true, /*clipped_level_step=*/64, + kClippedRatioThreshold, kClippedWaitFrames, + kDefaultAnalogConfig.clipping_predictor); + controller->Initialize(); + controller->set_stream_analog_level(kInitialVolume); + return controller; + }; + std::unique_ptr manager = factory(); + std::unique_ptr manager_with_override; + { + constexpr int kMinMicLevelOverride = 20; + static_assert( + kDefaultAnalogConfig.clipped_level_min >= kMinMicLevelOverride, + "Use a lower override value."); + test::ScopedFieldTrials field_trial( + GetAgcMinMicLevelExperimentFieldTrial(kMinMicLevelOverride)); + manager_with_override = factory(); + } + + // Create a test input signal which containts 80% of clipped samples. + AudioBuffer audio_buffer(kSampleRateHz, 1, kSampleRateHz, 1, kSampleRateHz, + 1); + WriteAudioBufferSamples(/*samples_value=*/4000.0f, /*clipped_ratio=*/0.8f, + audio_buffer); + + // Simulate 4 seconds of clipping; it is expected to trigger a downward + // adjustment of the analog gain. + CallPreProcessAndProcess(/*num_calls=*/400, audio_buffer, *manager); + CallPreProcessAndProcess(/*num_calls=*/400, audio_buffer, + *manager_with_override); + + // Make sure that an adaptation occurred. + ASSERT_GT(manager->stream_analog_level(), 0); + + // Check that the selected analog gain is the same for both controllers and + // that it equals the minimum level reached when clipping is handled. That is + // expected because the minimum microphone level override is less than the + // minimum level used when clipping is detected. + EXPECT_EQ(manager->stream_analog_level(), + manager_with_override->stream_analog_level()); + EXPECT_EQ(manager_with_override->stream_analog_level(), + kDefaultAnalogConfig.clipped_level_min); +} + // TODO(bugs.webrtc.org/12774): Test the bahavior of `clipped_level_step`. // TODO(bugs.webrtc.org/12774): Test the bahavior of `clipped_ratio_threshold`. // TODO(bugs.webrtc.org/12774): Test the bahavior of `clipped_wait_frames`.