From 2a959d96c9046c8dc49225c9ddd2aa58e7468acf Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Mon, 23 Jul 2018 14:48:07 +0000 Subject: [PATCH] Revert "Add one-stop-shop for built-in AEC toggling in APM" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 771b50ca0b92477ce8aabba025e959790dd1a949. Reason for revert: Introduces error-prone config. Original change's description: > Add one-stop-shop for built-in AEC toggling in APM > > This does not change what AEC functionality is available. > However, a client that only uses this interface - and not the submodule > pointer accessors - gets simpler code, and is guaranteed not to run any > two AECs in tandem. > > The submodule interface EchoControlMobile is being deprecated in > https://webrtc-review.googlesource.com/c/src/+/89392 > > Bug: webrtc:9535 > Change-Id: Id9326074e566be6d8768010fc421c457beff402c > Reviewed-on: https://webrtc-review.googlesource.com/89386 > Commit-Queue: Sam Zackrisson > Reviewed-by: Per Åhgren > Cr-Commit-Position: refs/heads/master@{#24066} TBR=saza@webrtc.org,peah@webrtc.org Change-Id: I43283a1b22538a4caa77313499989146b2ce67f1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9535 Reviewed-on: https://webrtc-review.googlesource.com/90060 Reviewed-by: Sam Zackrisson Commit-Queue: Sam Zackrisson Cr-Commit-Position: refs/heads/master@{#24067} --- .../audio_processing/audio_processing_impl.cc | 49 ++++++------------- .../audio_processing/audio_processing_impl.h | 2 - .../audio_processing_unittest.cc | 9 ++-- .../include/audio_processing.h | 11 +---- .../test/debug_dump_replayer.cc | 12 ++--- .../audio_processing/test/debug_dump_test.cc | 9 ++-- .../audio_processing_configs_fuzzer.cc | 2 +- 7 files changed, 30 insertions(+), 64 deletions(-) diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index e229b455ac..7e5955a98f 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -385,10 +385,8 @@ AudioProcessingImpl::AudioProcessingImpl( rtc::CritScope cs_capture(&crit_capture_); // Mark Echo Controller enabled if a factory is injected. - if (echo_control_factory_) { - config_.echo_cancellation.enabled = true; - capture_nonlocked_.echo_controller_enabled = true; - } + capture_nonlocked_.echo_controller_enabled = + static_cast(echo_control_factory_); public_submodules_->echo_cancellation.reset( new EchoCancellationImpl(&crit_render_, &crit_capture_)); @@ -493,15 +491,6 @@ int AudioProcessingImpl::MaybeInitialize( } int AudioProcessingImpl::InitializeLocked() { - // Ensure at most one built-in AEC is active, by arbitrarily preferring AEC2. - if (public_submodules_->echo_cancellation->is_enabled()) { - echo_control_mobile()->Enable(false); - config_.echo_cancellation.enabled = true; - config_.echo_cancellation.mobile_mode = false; - } else if (public_submodules_->echo_control_mobile->is_enabled()) { - config_.echo_cancellation.enabled = true; - config_.echo_cancellation.mobile_mode = true; - } UpdateActiveSubmoduleStates(); const int render_audiobuffer_num_output_frames = @@ -677,13 +666,6 @@ void AudioProcessingImpl::ApplyConfig(const AudioProcessing::Config& config) { rtc::CritScope cs_render(&crit_render_); rtc::CritScope cs_capture(&crit_capture_); - capture_nonlocked_.echo_controller_enabled = - config_.echo_cancellation.enabled && private_submodules_->echo_controller; - echo_cancellation()->Enable(config_.echo_cancellation.enabled && - !config_.echo_cancellation.mobile_mode); - echo_control_mobile()->Enable(config_.echo_cancellation.enabled && - config_.echo_cancellation.mobile_mode); - InitializeLowCutFilter(); RTC_LOG(LS_INFO) << "Highpass filter activated: " @@ -1187,8 +1169,8 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { HandleCaptureRuntimeSettings(); // Ensure that not both the AEC and AECM are active at the same time. - // TODO(bugs.webrtc.org/9535): Simplify once the public API Enable functions - // for these are moved to APM. + // TODO(peah): Simplify once the public API Enable functions for these + // are moved to APM. RTC_DCHECK(!(public_submodules_->echo_cancellation->is_enabled() && public_submodules_->echo_control_mobile->is_enabled())); @@ -1215,7 +1197,7 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { levels.peak, 1, RmsLevel::kMinLevelDb, 64); } - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { // Detect and flag any change in the analog gain. int analog_mic_level = gain_control()->stream_analog_level(); capture_.echo_path_gain_change = @@ -1239,7 +1221,7 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { capture_buffer->SplitIntoFrequencyBands(); } - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { // Force down-mixing of the number of channels after the detection of // capture signal saturation. // TODO(peah): Look into ensuring that this kind of tampering with the @@ -1249,7 +1231,7 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { // TODO(peah): Move the AEC3 low-cut filter to this place. if (private_submodules_->low_cut_filter && - !capture_nonlocked_.echo_controller_enabled) { + !private_submodules_->echo_controller) { private_submodules_->low_cut_filter->Process(capture_buffer); } RETURN_ON_ERR( @@ -1259,11 +1241,11 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { // Ensure that the stream delay was set before the call to the // AEC ProcessCaptureAudio function. if (public_submodules_->echo_cancellation->is_enabled() && - !was_stream_delay_set()) { + !private_submodules_->echo_controller && !was_stream_delay_set()) { return AudioProcessing::kStreamParameterNotSetError; } - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { data_dumper_->DumpRaw("stream_delay", stream_delay_ms()); if (was_stream_delay_set()) { @@ -1303,7 +1285,8 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { return AudioProcessing::kStreamParameterNotSetError; } - if (public_submodules_->echo_control_mobile->is_enabled()) { + if (!(private_submodules_->echo_controller || + public_submodules_->echo_cancellation->is_enabled())) { RETURN_ON_ERR(public_submodules_->echo_control_mobile->ProcessCaptureAudio( capture_buffer, stream_delay_ms())); } @@ -1521,7 +1504,7 @@ int AudioProcessingImpl::ProcessRenderStreamLocked() { } // TODO(peah): Perform the queueing ínside QueueRenderAudiuo(). - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { private_submodules_->echo_controller->AnalyzeRender(render_buffer); } @@ -1644,7 +1627,7 @@ AudioProcessing::AudioProcessingStatistics AudioProcessingImpl::GetStatistics() const { AudioProcessingStatistics stats; EchoCancellation::Metrics metrics; - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { rtc::CritScope cs_capture(&crit_capture_); auto ec_metrics = private_submodules_->echo_controller->GetMetrics(); float erl = static_cast(ec_metrics.echo_return_loss); @@ -1680,7 +1663,7 @@ AudioProcessingStats AudioProcessingImpl::GetStatistics( AudioProcessingStats stats; if (has_remote_tracks) { EchoCancellation::Metrics metrics; - if (capture_nonlocked_.echo_controller_enabled) { + if (private_submodules_->echo_controller) { rtc::CritScope cs_capture(&crit_capture_); auto ec_metrics = private_submodules_->echo_controller->GetMetrics(); stats.echo_return_loss = ec_metrics.echo_return_loss; @@ -1870,8 +1853,8 @@ void AudioProcessingImpl::MaybeUpdateHistograms() { static const int kMinDiffDelayMs = 60; if (echo_cancellation()->is_enabled()) { - // Activate delay_jumps_ counters if we know AEC2 is running. - // If a stream has echo we know that echo cancellation is in process. + // Activate delay_jumps_ counters if we know echo_cancellation is running. + // If a stream has echo we know that the echo_cancellation is in process. if (capture_.stream_delay_jumps == -1 && echo_cancellation()->stream_has_echo()) { capture_.stream_delay_jumps = 0; diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 8d0c4ce77b..44d0d087a5 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -393,8 +393,6 @@ class AudioProcessingImpl : public AudioProcessing { int prev_analog_mic_level; } capture_ RTC_GUARDED_BY(crit_capture_); - // This struct requires EITHER crit_capture_ OR crit_render_ to read, and - // requires both locks to write. struct ApmCaptureNonLockedState { ApmCaptureNonLockedState(bool intelligibility_enabled) : capture_processing_format(kSampleRate16kHz), diff --git a/modules/audio_processing/audio_processing_unittest.cc b/modules/audio_processing/audio_processing_unittest.cc index 3b7c930ecf..4b244fc8ab 100644 --- a/modules/audio_processing/audio_processing_unittest.cc +++ b/modules/audio_processing/audio_processing_unittest.cc @@ -176,24 +176,23 @@ bool FrameDataAreEqual(const AudioFrame& frame1, const AudioFrame& frame2) { } void EnableAllAPComponents(AudioProcessing* ap) { - AudioProcessing::Config apm_config; #if defined(WEBRTC_AUDIOPROC_FIXED_PROFILE) - apm_config.echo_cancellation.enabled = true; - apm_config.echo_cancellation.mobile_mode = true; + EXPECT_NOERR(ap->echo_control_mobile()->Enable(true)); EXPECT_NOERR(ap->gain_control()->set_mode(GainControl::kAdaptiveDigital)); EXPECT_NOERR(ap->gain_control()->Enable(true)); #elif defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE) - apm_config.echo_cancellation.enabled = true; - apm_config.echo_cancellation.mobile_mode = false; EXPECT_NOERR(ap->echo_cancellation()->enable_drift_compensation(true)); EXPECT_NOERR(ap->echo_cancellation()->enable_metrics(true)); EXPECT_NOERR(ap->echo_cancellation()->enable_delay_logging(true)); + EXPECT_NOERR(ap->echo_cancellation()->Enable(true)); EXPECT_NOERR(ap->gain_control()->set_mode(GainControl::kAdaptiveAnalog)); EXPECT_NOERR(ap->gain_control()->set_analog_level_limits(0, 255)); EXPECT_NOERR(ap->gain_control()->Enable(true)); #endif + + AudioProcessing::Config apm_config; apm_config.high_pass_filter.enabled = true; ap->ApplyConfig(apm_config); diff --git a/modules/audio_processing/include/audio_processing.h b/modules/audio_processing/include/audio_processing.h index b543b6cf71..9d27f1627d 100644 --- a/modules/audio_processing/include/audio_processing.h +++ b/modules/audio_processing/include/audio_processing.h @@ -253,13 +253,6 @@ class AudioProcessing : public rtc::RefCountInterface { // by changing the default values in the AudioProcessing::Config struct. // The config is applied by passing the struct to the ApplyConfig method. struct Config { - // Configures whether acoustic echo cancellation is performed. - // Has a specific tuning for mobile devices. - struct EchoCancellation { - bool enabled = false; - bool mobile_mode = false; - } echo_cancellation; - struct ResidualEchoDetector { bool enabled = true; } residual_echo_detector; @@ -803,7 +796,7 @@ class ProcessingConfig { class EchoCancellation { public: // EchoCancellation and EchoControlMobile may not be enabled simultaneously. - // If both are enabled, one (unspecified) will automatically be disabled. + // Enabling one will disable the other. virtual int Enable(bool enable) = 0; virtual bool is_enabled() const = 0; @@ -907,7 +900,7 @@ class EchoCancellation { class EchoControlMobile { public: // EchoCancellation and EchoControlMobile may not be enabled simultaneously. - // If both are enabled, one (unspecified) will automatically be disabled. + // Enabling one will disable the other. virtual int Enable(bool enable) = 0; virtual bool is_enabled() const = 0; diff --git a/modules/audio_processing/test/debug_dump_replayer.cc b/modules/audio_processing/test/debug_dump_replayer.cc index 13dfabb235..a06c76e32a 100644 --- a/modules/audio_processing/test/debug_dump_replayer.cc +++ b/modules/audio_processing/test/debug_dump_replayer.cc @@ -202,10 +202,8 @@ void DebugDumpReplayer::ConfigureApm(const audioproc::Config& msg) { // AEC configs. RTC_CHECK(msg.has_aec_enabled()); - if (msg.aec_enabled()) { - apm_config.echo_cancellation.enabled = true; - apm_config.echo_cancellation.mobile_mode = false; - } + RTC_CHECK_EQ(AudioProcessing::kNoError, + apm_->echo_cancellation()->Enable(msg.aec_enabled())); RTC_CHECK(msg.has_aec_drift_compensation_enabled()); RTC_CHECK_EQ(AudioProcessing::kNoError, @@ -220,10 +218,8 @@ void DebugDumpReplayer::ConfigureApm(const audioproc::Config& msg) { // AECM configs. RTC_CHECK(msg.has_aecm_enabled()); - if (msg.aecm_enabled()) { - apm_config.echo_cancellation.enabled = true; - apm_config.echo_cancellation.mobile_mode = true; - } + RTC_CHECK_EQ(AudioProcessing::kNoError, + apm_->echo_control_mobile()->Enable(msg.aecm_enabled())); RTC_CHECK(msg.has_aecm_comfort_noise_enabled()); RTC_CHECK_EQ(AudioProcessing::kNoError, diff --git a/modules/audio_processing/test/debug_dump_test.cc b/modules/audio_processing/test/debug_dump_test.cc index f86ee6feac..4e2943395e 100644 --- a/modules/audio_processing/test/debug_dump_test.cc +++ b/modules/audio_processing/test/debug_dump_test.cc @@ -368,9 +368,8 @@ TEST_F(DebugDumpTest, ToggleDelayAgnosticAec) { generator.StartRecording(); generator.Process(100); - AudioProcessing::Config new_config; - new_config.echo_cancellation.enabled = true; - generator.apm()->ApplyConfig(new_config); + EchoCancellation* aec = generator.apm()->echo_cancellation(); + EXPECT_EQ(AudioProcessing::kNoError, aec->Enable(!aec->is_enabled())); generator.Process(100); generator.StopRecording(); @@ -408,7 +407,6 @@ TEST_F(DebugDumpTest, VerifyCombinedExperimentalStringInclusive) { // Arbitrarily set clipping gain to 17, which will never be the default. config.Set(new ExperimentalAgc(true, 0, 17)); bool enable_aec3 = true; - apm_config.echo_cancellation.enabled = true; DebugDumpGenerator generator(config, apm_config, enable_aec3); generator.StartRecording(); generator.Process(100); @@ -465,8 +463,7 @@ TEST_F(DebugDumpTest, VerifyCombinedExperimentalStringExclusive) { TEST_F(DebugDumpTest, VerifyAec3ExperimentalString) { Config config; AudioProcessing::Config apm_config; - apm_config.echo_cancellation.enabled = true; - DebugDumpGenerator generator(config, apm_config, true /* enable_aec3 */); + DebugDumpGenerator generator(config, apm_config, true); generator.StartRecording(); generator.Process(100); generator.StopRecording(); diff --git a/test/fuzzers/audio_processing_configs_fuzzer.cc b/test/fuzzers/audio_processing_configs_fuzzer.cc index 4da38a9f7a..545e455e7e 100644 --- a/test/fuzzers/audio_processing_configs_fuzzer.cc +++ b/test/fuzzers/audio_processing_configs_fuzzer.cc @@ -128,7 +128,7 @@ std::unique_ptr CreateApm(test::FuzzDataHelper* fuzz_data, apm->AttachAecDump( absl::make_unique>()); - webrtc::AudioProcessing::Config apm_config = apm->GetConfig(); + webrtc::AudioProcessing::Config apm_config; apm_config.residual_echo_detector.enabled = red; apm_config.high_pass_filter.enabled = hpf; apm_config.gain_controller2.enabled = use_agc2_limiter;