From 652ada5029b35834ea64b06892c34543da1936f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Per=20=C3=85hgren?= Date: Wed, 3 Mar 2021 10:52:44 +0000 Subject: [PATCH] Enabling a safe fall-back functionality for overruns in the runtime settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: b/177830919 Change-Id: I9369f6fc004ceb2b626d33b36262bc8aeabdb1a0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/206988 Commit-Queue: Per Åhgren Reviewed-by: Alessio Bazzica Cr-Commit-Position: refs/heads/master@{#33371} --- api/audio/echo_control.h | 7 ++ .../audio_processing/aec3/echo_canceller3.cc | 5 ++ .../audio_processing/aec3/echo_canceller3.h | 6 ++ .../audio_processing/audio_processing_impl.cc | 42 ++++++---- .../audio_processing/audio_processing_impl.h | 6 ++ .../audio_processing_impl_unittest.cc | 83 +++++++++++++++++++ modules/audio_processing/common.h | 4 + .../include/audio_processing.h | 5 +- .../audio_processing/test/echo_control_mock.h | 4 + 9 files changed, 146 insertions(+), 16 deletions(-) diff --git a/api/audio/echo_control.h b/api/audio/echo_control.h index 8d567bf2b8..74fbc27b12 100644 --- a/api/audio/echo_control.h +++ b/api/audio/echo_control.h @@ -48,6 +48,13 @@ class EchoControl { // Provides an optional external estimate of the audio buffer delay. virtual void SetAudioBufferDelay(int delay_ms) = 0; + // Specifies whether the capture output will be used. The purpose of this is + // to allow the echo controller to deactivate some of the processing when the + // resulting output is anyway not used, for instance when the endpoint is + // muted. + // TODO(b/177830919): Make pure virtual. + virtual void SetCaptureOutputUsage(bool capture_output_used) {} + // Returns wheter the signal is altered. virtual bool ActiveProcessing() const = 0; diff --git a/modules/audio_processing/aec3/echo_canceller3.cc b/modules/audio_processing/aec3/echo_canceller3.cc index 0f8e35d09e..698ede7543 100644 --- a/modules/audio_processing/aec3/echo_canceller3.cc +++ b/modules/audio_processing/aec3/echo_canceller3.cc @@ -833,6 +833,11 @@ void EchoCanceller3::SetAudioBufferDelay(int delay_ms) { block_processor_->SetAudioBufferDelay(delay_ms); } +void EchoCanceller3::SetCaptureOutputUsage(bool capture_output_used) { + // TODO(b/177830919): Add functionality for reducing the complexity when the + // echo canceller output is not used. +} + bool EchoCanceller3::ActiveProcessing() const { return true; } diff --git a/modules/audio_processing/aec3/echo_canceller3.h b/modules/audio_processing/aec3/echo_canceller3.h index bacd5dfc48..a4aab4987f 100644 --- a/modules/audio_processing/aec3/echo_canceller3.h +++ b/modules/audio_processing/aec3/echo_canceller3.h @@ -118,6 +118,12 @@ class EchoCanceller3 : public EchoControl { // Provides an optional external estimate of the audio buffer delay. void SetAudioBufferDelay(int delay_ms) override; + // Specifies whether the capture output will be used. The purpose of this is + // to allow the echo controller to deactivate some of the processing when the + // resulting output is anyway not used, for instance when the endpoint is + // muted. + void SetCaptureOutputUsage(bool capture_output_used) override; + bool ActiveProcessing() const override; // Signals whether an external detector has detected echo leakage from the diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index 1d32c851ab..79a315113a 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -49,8 +49,6 @@ namespace webrtc { -constexpr int kRuntimeSettingQueueSize = 100; - namespace { static bool LayoutHasKeyboard(AudioProcessing::ChannelLayout layout) { @@ -253,8 +251,8 @@ AudioProcessingImpl::AudioProcessingImpl( new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))), use_setup_specific_default_aec3_config_( UseSetupSpecificDefaultAec3Congfig()), - capture_runtime_settings_(kRuntimeSettingQueueSize), - render_runtime_settings_(kRuntimeSettingQueueSize), + capture_runtime_settings_(RuntimeSettingQueueSize()), + render_runtime_settings_(RuntimeSettingQueueSize()), capture_runtime_settings_enqueuer_(&capture_runtime_settings_), render_runtime_settings_enqueuer_(&render_runtime_settings_), echo_control_factory_(std::move(echo_control_factory)), @@ -674,6 +672,10 @@ void AudioProcessingImpl::HandleCaptureOutputUsedSetting( submodules_.agc_manager->HandleCaptureOutputUsedChange( capture_.capture_output_used); } + if (submodules_.echo_controller) { + submodules_.echo_controller->SetCaptureOutputUsage( + capture_.capture_output_used); + } } void AudioProcessingImpl::SetRuntimeSetting(RuntimeSetting setting) { @@ -720,19 +722,13 @@ AudioProcessingImpl::RuntimeSettingEnqueuer::~RuntimeSettingEnqueuer() = bool AudioProcessingImpl::RuntimeSettingEnqueuer::Enqueue( RuntimeSetting setting) { - int remaining_attempts = 10; - while (!runtime_settings_.Insert(&setting) && remaining_attempts-- > 0) { - RuntimeSetting setting_to_discard; - if (runtime_settings_.Remove(&setting_to_discard)) { - RTC_LOG(LS_ERROR) - << "The runtime settings queue is full. Oldest setting discarded."; - } - } - if (remaining_attempts == 0) { + const bool successful_insert = runtime_settings_.Insert(&setting); + + if (!successful_insert) { RTC_HISTOGRAM_BOOLEAN("WebRTC.Audio.ApmRuntimeSettingCannotEnqueue", 1); RTC_LOG(LS_ERROR) << "Cannot enqueue a new runtime setting."; } - return remaining_attempts == 0; + return successful_insert; } int AudioProcessingImpl::MaybeInitializeCapture( @@ -806,6 +802,7 @@ int AudioProcessingImpl::ProcessStream(const float* const* src, void AudioProcessingImpl::HandleCaptureRuntimeSettings() { RuntimeSetting setting; + int num_settings_processed = 0; while (capture_runtime_settings_.Remove(&setting)) { if (aec_dump_) { aec_dump_->WriteRuntimeSetting(setting); @@ -864,6 +861,23 @@ void AudioProcessingImpl::HandleCaptureRuntimeSettings() { HandleCaptureOutputUsedSetting(value); break; } + ++num_settings_processed; + } + + if (num_settings_processed >= RuntimeSettingQueueSize()) { + // Handle overrun of the runtime settings queue, which likely will has + // caused settings to be discarded. + HandleOverrunInCaptureRuntimeSettingsQueue(); + } +} + +void AudioProcessingImpl::HandleOverrunInCaptureRuntimeSettingsQueue() { + // Fall back to a safe state for the case when a setting for capture output + // usage setting has been missed. + capture_.capture_output_used = true; + if (submodules_.echo_controller) { + submodules_.echo_controller->SetCaptureOutputUsage( + capture_.capture_output_used); } } diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 23028ec788..8306ac7502 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -342,6 +342,12 @@ class AudioProcessingImpl : public AudioProcessing { void RecordAudioProcessingState() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_capture_); + // Ensures that overruns in the capture runtime settings queue is properly + // handled by the code, providing safe-fallbacks to mitigate the implications + // of any settings being missed. + void HandleOverrunInCaptureRuntimeSettingsQueue() + RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_capture_); + // AecDump instance used for optionally logging APM config, input // and output to file in the AEC-dump format defined in debug.proto. std::unique_ptr aec_dump_; diff --git a/modules/audio_processing/audio_processing_impl_unittest.cc b/modules/audio_processing/audio_processing_impl_unittest.cc index e289c316bc..65de4d6a05 100644 --- a/modules/audio_processing/audio_processing_impl_unittest.cc +++ b/modules/audio_processing/audio_processing_impl_unittest.cc @@ -14,6 +14,7 @@ #include #include "api/scoped_refptr.h" +#include "modules/audio_processing/common.h" #include "modules/audio_processing/include/audio_processing.h" #include "modules/audio_processing/optionally_built_submodule_creators.h" #include "modules/audio_processing/test/audio_processing_builder_for_testing.h" @@ -202,6 +203,88 @@ TEST(AudioProcessingImplTest, UpdateCapturePreGainRuntimeSetting) { << "Frame should be amplified."; } +TEST(AudioProcessingImplTest, EchoControllerObservesSetCaptureUsageChange) { + // Tests that the echo controller observes that the capture usage has been + // updated. + auto echo_control_factory = std::make_unique(); + const MockEchoControlFactory* echo_control_factory_ptr = + echo_control_factory.get(); + + std::unique_ptr apm( + AudioProcessingBuilderForTesting() + .SetEchoControlFactory(std::move(echo_control_factory)) + .Create()); + + constexpr int16_t kAudioLevel = 10000; + constexpr int kSampleRateHz = 48000; + constexpr int kNumChannels = 2; + std::array frame; + StreamConfig config(kSampleRateHz, kNumChannels, /*has_keyboard=*/false); + frame.fill(kAudioLevel); + + MockEchoControl* echo_control_mock = echo_control_factory_ptr->GetNext(); + + // Ensure that SetCaptureOutputUsage is not called when no runtime settings + // are passed. + EXPECT_CALL(*echo_control_mock, SetCaptureOutputUsage(testing::_)).Times(0); + apm->ProcessStream(frame.data(), config, config, frame.data()); + + // Ensure that SetCaptureOutputUsage is called with the right information when + // a runtime setting is passed. + EXPECT_CALL(*echo_control_mock, + SetCaptureOutputUsage(/*capture_output_used=*/false)) + .Times(1); + EXPECT_TRUE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/false))); + apm->ProcessStream(frame.data(), config, config, frame.data()); + + EXPECT_CALL(*echo_control_mock, + SetCaptureOutputUsage(/*capture_output_used=*/true)) + .Times(1); + EXPECT_TRUE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/true))); + apm->ProcessStream(frame.data(), config, config, frame.data()); + + // The number of positions to place items in the queue is equal to the queue + // size minus 1. + constexpr int kNumSlotsInQueue = RuntimeSettingQueueSize(); + + // Ensure that SetCaptureOutputUsage is called with the right information when + // many runtime settings are passed. + for (int k = 0; k < kNumSlotsInQueue - 1; ++k) { + EXPECT_TRUE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/false))); + } + EXPECT_CALL(*echo_control_mock, + SetCaptureOutputUsage(/*capture_output_used=*/false)) + .Times(kNumSlotsInQueue - 1); + apm->ProcessStream(frame.data(), config, config, frame.data()); + + // Ensure that SetCaptureOutputUsage is properly called with the fallback + // value when the runtime settings queue becomes full. + for (int k = 0; k < kNumSlotsInQueue; ++k) { + EXPECT_TRUE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/false))); + } + EXPECT_FALSE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/false))); + EXPECT_FALSE(apm->PostRuntimeSetting( + AudioProcessing::RuntimeSetting::CreateCaptureOutputUsedSetting( + /*capture_output_used=*/false))); + EXPECT_CALL(*echo_control_mock, + SetCaptureOutputUsage(/*capture_output_used=*/false)) + .Times(kNumSlotsInQueue); + EXPECT_CALL(*echo_control_mock, + SetCaptureOutputUsage(/*capture_output_used=*/true)) + .Times(1); + apm->ProcessStream(frame.data(), config, config, frame.data()); +} + TEST(AudioProcessingImplTest, EchoControllerObservesPreAmplifierEchoPathGainChange) { // Tests that the echo controller observes an echo path gain change when the diff --git a/modules/audio_processing/common.h b/modules/audio_processing/common.h index d8532c5749..2c88c4e46c 100644 --- a/modules/audio_processing/common.h +++ b/modules/audio_processing/common.h @@ -16,6 +16,10 @@ namespace webrtc { +constexpr int RuntimeSettingQueueSize() { + return 100; +} + static inline size_t ChannelsFromLayout(AudioProcessing::ChannelLayout layout) { switch (layout) { case AudioProcessing::kMono: diff --git a/modules/audio_processing/include/audio_processing.h b/modules/audio_processing/include/audio_processing.h index 2312ffd536..36d82565e0 100644 --- a/modules/audio_processing/include/audio_processing.h +++ b/modules/audio_processing/include/audio_processing.h @@ -431,8 +431,9 @@ class RTC_EXPORT AudioProcessing : public rtc::RefCountInterface { return {Type::kCustomRenderProcessingRuntimeSetting, payload}; } - static RuntimeSetting CreateCaptureOutputUsedSetting(bool payload) { - return {Type::kCaptureOutputUsed, payload}; + static RuntimeSetting CreateCaptureOutputUsedSetting( + bool capture_output_used) { + return {Type::kCaptureOutputUsed, capture_output_used}; } Type type() const { return type_; } diff --git a/modules/audio_processing/test/echo_control_mock.h b/modules/audio_processing/test/echo_control_mock.h index 927de43ae0..763d6e4f0b 100644 --- a/modules/audio_processing/test/echo_control_mock.h +++ b/modules/audio_processing/test/echo_control_mock.h @@ -34,6 +34,10 @@ class MockEchoControl : public EchoControl { (override)); MOCK_METHOD(EchoControl::Metrics, GetMetrics, (), (const, override)); MOCK_METHOD(void, SetAudioBufferDelay, (int delay_ms), (override)); + MOCK_METHOD(void, + SetCaptureOutputUsage, + (bool capture_output_used), + (override)); MOCK_METHOD(bool, ActiveProcessing, (), (const, override)); };