From 03fbace409566bcbf3646b7c9c36a503da1449f5 Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Mon, 21 Oct 2019 10:09:25 +0200 Subject: [PATCH] Remove apm_helpers, consolidate audio config in WebRtcVoiceEngine Refactorings to the audio processing module has, piece by piece, decreased the workload of the apm_helpers helpers. It has come to a point where it seems more reliable to consolidate what little is left into the WebRtcVoiceEngine itself. Bug: webrtc:9878 Change-Id: I6d983ace8e7ccb1b99d95178cf72608a657c7506 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157443 Reviewed-by: Niels Moller Reviewed-by: Alessio Bazzica Commit-Queue: Sam Zackrisson Cr-Commit-Position: refs/heads/master@{#29553} --- media/BUILD.gn | 3 - media/engine/apm_helpers.cc | 51 -------------- media/engine/apm_helpers.h | 35 ---------- media/engine/apm_helpers_unittest.cc | 73 -------------------- media/engine/webrtc_voice_engine.cc | 28 +++++--- media/engine/webrtc_voice_engine.h | 1 - media/engine/webrtc_voice_engine_unittest.cc | 46 +++++++----- 7 files changed, 47 insertions(+), 190 deletions(-) delete mode 100644 media/engine/apm_helpers.cc delete mode 100644 media/engine/apm_helpers.h delete mode 100644 media/engine/apm_helpers_unittest.cc diff --git a/media/BUILD.gn b/media/BUILD.gn index 371f6cd772..45353344d0 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -319,8 +319,6 @@ rtc_library("rtc_audio_video") { sources = [ "engine/adm_helpers.cc", "engine/adm_helpers.h", - "engine/apm_helpers.cc", - "engine/apm_helpers.h", "engine/null_webrtc_video_engine.h", "engine/payload_type_mapper.cc", "engine/payload_type_mapper.h", @@ -590,7 +588,6 @@ if (rtc_include_tests) { "base/video_adapter_unittest.cc", "base/video_broadcaster_unittest.cc", "base/video_common_unittest.cc", - "engine/apm_helpers_unittest.cc", "engine/encoder_simulcast_proxy_unittest.cc", "engine/internal_decoder_factory_unittest.cc", "engine/multiplex_codec_factory_unittest.cc", diff --git a/media/engine/apm_helpers.cc b/media/engine/apm_helpers.cc deleted file mode 100644 index 31bdd4bcdc..0000000000 --- a/media/engine/apm_helpers.cc +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "media/engine/apm_helpers.h" - -#include "modules/audio_processing/include/audio_processing.h" -#include "rtc_base/checks.h" -#include "rtc_base/logging.h" - -namespace webrtc { -namespace apm_helpers { - -void Init(AudioProcessing* apm) { - RTC_DCHECK(apm); - - constexpr int kMinVolumeLevel = 0; - constexpr int kMaxVolumeLevel = 255; - - AudioProcessing::Config config = apm->GetConfig(); -#if defined(WEBRTC_IOS) || defined(WEBRTC_ANDROID) - config.gain_controller1.mode = config.gain_controller1.kFixedDigital; -#else - config.gain_controller1.mode = config.gain_controller1.kAdaptiveAnalog; -#endif - RTC_LOG(LS_INFO) << "Setting AGC mode to " << config.gain_controller1.mode; - // This is the initialization which used to happen in VoEBase::Init(), but - // which is not covered by the WVoE::ApplyOptions(). - config.gain_controller1.analog_level_minimum = kMinVolumeLevel; - config.gain_controller1.analog_level_maximum = kMaxVolumeLevel; - apm->ApplyConfig(config); -} - -void SetEcStatus(AudioProcessing* apm, bool enable, EcModes mode) { - RTC_DCHECK(apm); - RTC_DCHECK(mode == kEcConference || mode == kEcAecm) << "mode: " << mode; - AudioProcessing::Config apm_config = apm->GetConfig(); - apm_config.echo_canceller.enabled = enable; - apm_config.echo_canceller.mobile_mode = (mode == kEcAecm); - apm_config.echo_canceller.legacy_moderate_suppression_level = false; - apm->ApplyConfig(apm_config); - RTC_LOG(LS_INFO) << "Echo control set to " << enable << " with mode " << mode; -} -} // namespace apm_helpers -} // namespace webrtc diff --git a/media/engine/apm_helpers.h b/media/engine/apm_helpers.h deleted file mode 100644 index 7bedda71fc..0000000000 --- a/media/engine/apm_helpers.h +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MEDIA_ENGINE_APM_HELPERS_H_ -#define MEDIA_ENGINE_APM_HELPERS_H_ - -#include - -namespace webrtc { - -class AudioProcessing; - -enum EcModes { - kEcConference, // Conferencing default (aggressive AEC). - kEcAecm, // AEC mobile. -}; - -namespace apm_helpers { - -void Init(AudioProcessing* apm); -void SetEcStatus(AudioProcessing* apm, bool enable, EcModes mode); -void SetEcMetricsStatus(AudioProcessing* apm, bool enable); -void SetAecmMode(AudioProcessing* apm, bool enable_cng); - -} // namespace apm_helpers -} // namespace webrtc - -#endif // MEDIA_ENGINE_APM_HELPERS_H_ diff --git a/media/engine/apm_helpers_unittest.cc b/media/engine/apm_helpers_unittest.cc deleted file mode 100644 index dac24b3a25..0000000000 --- a/media/engine/apm_helpers_unittest.cc +++ /dev/null @@ -1,73 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "media/engine/apm_helpers.h" - -#include "api/scoped_refptr.h" -#include "modules/audio_processing/include/audio_processing.h" -#include "test/gmock.h" -#include "test/gtest.h" - -namespace webrtc { -namespace { - -struct TestHelper { - TestHelper() { - // This replicates the conditions from voe_auto_test. - Config config; - config.Set(new ExperimentalAgc(false)); - apm_ = rtc::scoped_refptr( - AudioProcessingBuilder().Create(config)); - apm_helpers::Init(apm()); - } - - AudioProcessing* apm() { return apm_.get(); } - - const AudioProcessing* apm() const { return apm_.get(); } - - private: - rtc::scoped_refptr apm_; -}; -} // namespace - -TEST(ApmHelpersTest, EcStatus_DefaultMode) { - TestHelper helper; - webrtc::AudioProcessing::Config config = helper.apm()->GetConfig(); - EXPECT_FALSE(config.echo_canceller.enabled); -} - -TEST(ApmHelpersTest, EcStatus_EnableDisable) { - TestHelper helper; - webrtc::AudioProcessing::Config config; - - apm_helpers::SetEcStatus(helper.apm(), true, kEcAecm); - config = helper.apm()->GetConfig(); - EXPECT_TRUE(config.echo_canceller.enabled); - EXPECT_TRUE(config.echo_canceller.mobile_mode); - - apm_helpers::SetEcStatus(helper.apm(), false, kEcAecm); - config = helper.apm()->GetConfig(); - EXPECT_FALSE(config.echo_canceller.enabled); - - apm_helpers::SetEcStatus(helper.apm(), true, kEcConference); - config = helper.apm()->GetConfig(); - EXPECT_TRUE(config.echo_canceller.enabled); - EXPECT_FALSE(config.echo_canceller.mobile_mode); - - apm_helpers::SetEcStatus(helper.apm(), false, kEcConference); - config = helper.apm()->GetConfig(); - EXPECT_FALSE(config.echo_canceller.enabled); - - apm_helpers::SetEcStatus(helper.apm(), true, kEcAecm); - config = helper.apm()->GetConfig(); - EXPECT_TRUE(config.echo_canceller.enabled); - EXPECT_TRUE(config.echo_canceller.mobile_mode); -} -} // namespace webrtc diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 08fe73fb37..ee8e5f0bc3 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -26,7 +26,6 @@ #include "media/base/media_constants.h" #include "media/base/stream_params.h" #include "media/engine/adm_helpers.h" -#include "media/engine/apm_helpers.h" #include "media/engine/payload_type_mapper.h" #include "media/engine/webrtc_media_engine.h" #include "modules/audio_device/audio_device_impl.h" @@ -256,7 +255,6 @@ void WebRtcVoiceEngine::Init() { #endif // WEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE RTC_CHECK(adm()); webrtc::adm_helpers::Init(adm()); - webrtc::apm_helpers::Init(apm()); // Set up AudioState. { @@ -320,8 +318,8 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) { AudioOptions options = options_in; // The options are modified below. // Set and adjust echo canceller options. - // kEcConference is AEC with high suppression. - webrtc::EcModes ec_mode = webrtc::kEcConference; + // Use desktop AEC by default, when not using hardware AEC. + bool use_mobile_software_aec = false; #if defined(WEBRTC_IOS) if (options.ios_force_software_aec_HACK && @@ -337,7 +335,7 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) { RTC_LOG(LS_INFO) << "Always disable AEC on iOS. Use built-in instead."; } #elif defined(WEBRTC_ANDROID) - ec_mode = webrtc::kEcAecm; + use_mobile_software_aec = true; #endif // Set and adjust noise suppressor options. @@ -401,8 +399,6 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) { << "Disabling EC since built-in EC will be used instead"; } } - webrtc::apm_helpers::SetEcStatus(apm(), *options.echo_cancellation, - ec_mode); } if (options.auto_gain_control) { @@ -475,10 +471,26 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) { webrtc::AudioProcessing::Config apm_config = apm()->GetConfig(); + if (options.echo_cancellation) { + apm_config.echo_canceller.enabled = *options.echo_cancellation; + apm_config.echo_canceller.mobile_mode = use_mobile_software_aec; + apm_config.echo_canceller.legacy_moderate_suppression_level = false; + } + if (options.auto_gain_control) { const bool enabled = *options.auto_gain_control; apm_config.gain_controller1.enabled = enabled; - RTC_LOG(LS_INFO) << "Setting AGC to " << enabled; +#if defined(WEBRTC_IOS) || defined(WEBRTC_ANDROID) + apm_config.gain_controller1.mode = + apm_config.gain_controller1.kFixedDigital; +#else + apm_config.gain_controller1.mode = + apm_config.gain_controller1.kAdaptiveAnalog; +#endif + constexpr int kMinVolumeLevel = 0; + constexpr int kMaxVolumeLevel = 255; + apm_config.gain_controller1.analog_level_minimum = kMinVolumeLevel; + apm_config.gain_controller1.analog_level_maximum = kMaxVolumeLevel; } if (options.tx_agc_target_dbov) { apm_config.gain_controller1.target_level_dbfs = *options.tx_agc_target_dbov; diff --git a/media/engine/webrtc_voice_engine.h b/media/engine/webrtc_voice_engine.h index 8067ef064e..add587fddb 100644 --- a/media/engine/webrtc_voice_engine.h +++ b/media/engine/webrtc_voice_engine.h @@ -24,7 +24,6 @@ #include "call/call.h" #include "media/base/media_engine.h" #include "media/base/rtp_utils.h" -#include "media/engine/apm_helpers.h" #include "rtc_base/buffer.h" #include "rtc_base/constructor_magic.h" #include "rtc_base/network_route.h" diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index bcffa403d6..fd054df153 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -204,7 +204,7 @@ class WebRtcVoiceEngineTestFake : public ::testing::Test { recv_parameters_.codecs.push_back(kPcmuCodec); // Default Options. - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_TRUE(IsHighPassFilterEnabled()); EXPECT_TRUE(IsTypingDetectionEnabled()); EXPECT_TRUE(apm_config_.noise_suppression.enabled); @@ -755,8 +755,16 @@ class WebRtcVoiceEngineTestFake : public ::testing::Test { EXPECT_TRUE(apm_config_.gain_controller1.enable_limiter); } - bool IsEchoCancellationEnabled() { - return apm_config_.echo_canceller.enabled; + void VerifyEchoCancellationSettings(bool enabled) { + constexpr bool kDefaultUseAecm = +#if defined(WEBRTC_ANDROID) + true; +#else + false; +#endif + EXPECT_EQ(apm_config_.echo_canceller.enabled, enabled); + EXPECT_EQ(apm_config_.echo_canceller.mobile_mode, kDefaultUseAecm); + EXPECT_FALSE(apm_config_.echo_canceller.use_legacy_aec); } bool IsHighPassFilterEnabled() { @@ -2832,7 +2840,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAudioOptions) { // Nothing set in AudioOptions, so everything should be as default. send_parameters_.options = cricket::AudioOptions(); SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_TRUE(IsHighPassFilterEnabled()); EXPECT_TRUE(IsTypingDetectionEnabled()); EXPECT_EQ(200u, GetRecvStreamConfig(kSsrcY).jitter_buffer_max_packets); @@ -2856,18 +2864,18 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAudioOptions) { // Turn echo cancellation off send_parameters_.options.echo_cancellation = false; SetSendParameters(send_parameters_); - EXPECT_FALSE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/false); // Turn echo cancellation back on, with settings, and make sure // nothing else changed. send_parameters_.options.echo_cancellation = true; SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); // Turn off echo cancellation and delay agnostic aec. send_parameters_.options.echo_cancellation = false; SetSendParameters(send_parameters_); - EXPECT_FALSE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/false); // Restore AEC to be on to work with the following tests. send_parameters_.options.echo_cancellation = true; @@ -2876,13 +2884,13 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAudioOptions) { // Turn off AGC send_parameters_.options.auto_gain_control = false; SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(apm_config_.gain_controller1.enabled); // Turn AGC back on send_parameters_.options.auto_gain_control = true; SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_TRUE(apm_config_.gain_controller1.enabled); // Turn off other options. @@ -2890,7 +2898,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAudioOptions) { send_parameters_.options.highpass_filter = false; send_parameters_.options.stereo_swapping = true; SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(IsHighPassFilterEnabled()); EXPECT_TRUE(apm_config_.gain_controller1.enabled); EXPECT_FALSE(apm_config_.noise_suppression.enabled); @@ -2898,7 +2906,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAudioOptions) { // Set options again to ensure it has no impact. SetSendParameters(send_parameters_); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_TRUE(apm_config_.gain_controller1.enabled); EXPECT_FALSE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); @@ -2947,13 +2955,13 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOptionOverridesViaChannels) { parameters_options_all.options.auto_gain_control = true; parameters_options_all.options.noise_suppression = true; EXPECT_TRUE(channel1->SetSendParameters(parameters_options_all)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); VerifyGainControlEnabledCorrectly(); EXPECT_TRUE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); EXPECT_EQ(parameters_options_all.options, channel1->options()); EXPECT_TRUE(channel2->SetSendParameters(parameters_options_all)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); VerifyGainControlEnabledCorrectly(); EXPECT_EQ(parameters_options_all.options, channel2->options()); @@ -2961,7 +2969,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOptionOverridesViaChannels) { cricket::AudioSendParameters parameters_options_no_ns = send_parameters_; parameters_options_no_ns.options.noise_suppression = false; EXPECT_TRUE(channel1->SetSendParameters(parameters_options_no_ns)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); VerifyGainControlEnabledCorrectly(); @@ -2975,7 +2983,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOptionOverridesViaChannels) { cricket::AudioSendParameters parameters_options_no_agc = send_parameters_; parameters_options_no_agc.options.auto_gain_control = false; EXPECT_TRUE(channel2->SetSendParameters(parameters_options_no_agc)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(apm_config_.gain_controller1.enabled); EXPECT_TRUE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); @@ -2985,19 +2993,19 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOptionOverridesViaChannels) { EXPECT_EQ(expected_options, channel2->options()); EXPECT_TRUE(channel_->SetSendParameters(parameters_options_all)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); VerifyGainControlEnabledCorrectly(); EXPECT_TRUE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); channel1->SetSend(true); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); VerifyGainControlEnabledCorrectly(); EXPECT_FALSE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); channel2->SetSend(true); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(apm_config_.gain_controller1.enabled); EXPECT_TRUE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel); @@ -3008,7 +3016,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOptionOverridesViaChannels) { parameters_options_no_agc_nor_ns.options.auto_gain_control = false; parameters_options_no_agc_nor_ns.options.noise_suppression = false; EXPECT_TRUE(channel2->SetSendParameters(parameters_options_no_agc_nor_ns)); - EXPECT_TRUE(IsEchoCancellationEnabled()); + VerifyEchoCancellationSettings(/*enabled=*/true); EXPECT_FALSE(apm_config_.gain_controller1.enabled); EXPECT_FALSE(apm_config_.noise_suppression.enabled); EXPECT_EQ(apm_config_.noise_suppression.level, kDefaultNsLevel);