From 421c85935126c5c529149236fb5de4caaf5b925f Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Mon, 11 Feb 2019 13:39:46 +0100 Subject: [PATCH] Remove crit_render_ lock from webrtc::GainControlImpl The lock is unnecessary and potentially unsafe: 1) All gain_control accesses in AudioProcessingImpl happen - and are intended to happen - while holding the crit_capture_ lock, and all external API calls take the same lock once inside GainControlImpl. 2) If ProcessCaptureStreamLocked (locked by crit_capture) calls a gain_control function that takes crit_render, the mandated locking order (render before capture) is violated and we might get a deadlock with the render thread. Bug: b/123456404 Change-Id: Id7a888827e347e5e1d50e2f87d90e8b68f52b7b8 Reviewed-on: https://webrtc-review.googlesource.com/c/122087 Reviewed-by: Alessio Bazzica Commit-Queue: Sam Zackrisson Cr-Commit-Position: refs/heads/master@{#26637} --- .../audio_processing/audio_processing_impl.cc | 70 +++++++++---------- modules/audio_processing/gain_control_impl.cc | 30 +++----- modules/audio_processing/gain_control_impl.h | 6 +- .../audio_processing/gain_control_unittest.cc | 3 +- test/fuzzers/agc_fuzzer.cc | 3 +- 5 files changed, 44 insertions(+), 68 deletions(-) diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index 5f0d676511..3400abf6f2 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -387,48 +387,42 @@ AudioProcessingImpl::AudioProcessingImpl( capture_(config.Get().enabled), #endif capture_nonlocked_() { - { - rtc::CritScope cs_render(&crit_render_); - rtc::CritScope cs_capture(&crit_capture_); + // Mark Echo Controller enabled if a factory is injected. + capture_nonlocked_.echo_controller_enabled = + static_cast(echo_control_factory_); - // Mark Echo Controller enabled if a factory is injected. - capture_nonlocked_.echo_controller_enabled = - static_cast(echo_control_factory_); + public_submodules_->gain_control.reset(new GainControlImpl(&crit_capture_)); + public_submodules_->level_estimator.reset( + new LevelEstimatorImpl(&crit_capture_)); + public_submodules_->noise_suppression.reset( + new NoiseSuppressionImpl(&crit_capture_)); + public_submodules_->noise_suppression_proxy.reset(new NoiseSuppressionProxy( + this, public_submodules_->noise_suppression.get())); + public_submodules_->voice_detection.reset( + new VoiceDetectionImpl(&crit_capture_)); + public_submodules_->gain_control_for_experimental_agc.reset( + new GainControlForExperimentalAgc(public_submodules_->gain_control.get(), + &crit_capture_)); - public_submodules_->gain_control.reset( - new GainControlImpl(&crit_render_, &crit_capture_)); - public_submodules_->level_estimator.reset( - new LevelEstimatorImpl(&crit_capture_)); - public_submodules_->noise_suppression.reset( - new NoiseSuppressionImpl(&crit_capture_)); - public_submodules_->noise_suppression_proxy.reset(new NoiseSuppressionProxy( - this, public_submodules_->noise_suppression.get())); - public_submodules_->voice_detection.reset( - new VoiceDetectionImpl(&crit_capture_)); - public_submodules_->gain_control_for_experimental_agc.reset( - new GainControlForExperimentalAgc( - public_submodules_->gain_control.get(), &crit_capture_)); - - // If no echo detector is injected, use the ResidualEchoDetector. - if (!private_submodules_->echo_detector) { - private_submodules_->echo_detector = - new rtc::RefCountedObject(); - } - - private_submodules_->echo_cancellation.reset(new EchoCancellationImpl()); - private_submodules_->echo_control_mobile.reset(new EchoControlMobileImpl()); - // TODO(alessiob): Move the injected gain controller once injection is - // implemented. - private_submodules_->gain_controller2.reset(new GainController2()); - - RTC_LOG(LS_INFO) << "Capture analyzer activated: " - << !!private_submodules_->capture_analyzer - << "\nCapture post processor activated: " - << !!private_submodules_->capture_post_processor - << "\nRender pre processor activated: " - << !!private_submodules_->render_pre_processor; + // If no echo detector is injected, use the ResidualEchoDetector. + if (!private_submodules_->echo_detector) { + private_submodules_->echo_detector = + new rtc::RefCountedObject(); } + private_submodules_->echo_cancellation.reset(new EchoCancellationImpl()); + private_submodules_->echo_control_mobile.reset(new EchoControlMobileImpl()); + // TODO(alessiob): Move the injected gain controller once injection is + // implemented. + private_submodules_->gain_controller2.reset(new GainController2()); + + RTC_LOG(LS_INFO) << "Capture analyzer activated: " + << !!private_submodules_->capture_analyzer + << "\nCapture post processor activated: " + << !!private_submodules_->capture_post_processor + << "\nRender pre processor activated: " + << !!private_submodules_->render_pre_processor; + SetExtraOptions(config); } diff --git a/modules/audio_processing/gain_control_impl.cc b/modules/audio_processing/gain_control_impl.cc index 26ab1b457b..cd21e4c93e 100644 --- a/modules/audio_processing/gain_control_impl.cc +++ b/modules/audio_processing/gain_control_impl.cc @@ -89,10 +89,8 @@ class GainControlImpl::GainController { int GainControlImpl::instance_counter_ = 0; -GainControlImpl::GainControlImpl(rtc::CriticalSection* crit_render, - rtc::CriticalSection* crit_capture) - : crit_render_(crit_render), - crit_capture_(crit_capture), +GainControlImpl::GainControlImpl(rtc::CriticalSection* crit_capture) + : crit_capture_(crit_capture), data_dumper_(new ApmDataDumper(instance_counter_)), mode_(kAdaptiveAnalog), minimum_capture_level_(0), @@ -103,7 +101,6 @@ GainControlImpl::GainControlImpl(rtc::CriticalSection* crit_render, analog_capture_level_(0), was_analog_level_set_(false), stream_is_saturated_(false) { - RTC_DCHECK(crit_render); RTC_DCHECK(crit_capture); } @@ -267,7 +264,6 @@ int GainControlImpl::stream_analog_level() { } int GainControlImpl::Enable(bool enable) { - rtc::CritScope cs_render(crit_render_); rtc::CritScope cs_capture(crit_capture_); if (enable && !enabled_) { enabled_ = enable; // Must be set before Initialize() is called. @@ -287,7 +283,6 @@ bool GainControlImpl::is_enabled() const { } int GainControlImpl::set_mode(Mode mode) { - rtc::CritScope cs_render(crit_render_); rtc::CritScope cs_capture(crit_capture_); if (MapSetting(mode) == -1) { return AudioProcessing::kBadParameterError; @@ -354,10 +349,8 @@ int GainControlImpl::set_target_level_dbfs(int level) { if (level > 31 || level < 0) { return AudioProcessing::kBadParameterError; } - { - rtc::CritScope cs(crit_capture_); - target_level_dbfs_ = level; - } + rtc::CritScope cs(crit_capture_); + target_level_dbfs_ = level; return Configure(); } @@ -370,18 +363,14 @@ int GainControlImpl::set_compression_gain_db(int gain) { if (gain < 0 || gain > 90) { return AudioProcessing::kBadParameterError; } - { - rtc::CritScope cs(crit_capture_); - compression_gain_db_ = gain; - } + rtc::CritScope cs(crit_capture_); + compression_gain_db_ = gain; return Configure(); } int GainControlImpl::enable_limiter(bool enable) { - { - rtc::CritScope cs(crit_capture_); - limiter_enabled_ = enable; - } + rtc::CritScope cs(crit_capture_); + limiter_enabled_ = enable; return Configure(); } @@ -391,7 +380,6 @@ bool GainControlImpl::is_limiter_enabled() const { } void GainControlImpl::Initialize(size_t num_proc_channels, int sample_rate_hz) { - rtc::CritScope cs_render(crit_render_); rtc::CritScope cs_capture(crit_capture_); data_dumper_->InitiateNewSetOfRecordings(); @@ -415,8 +403,6 @@ void GainControlImpl::Initialize(size_t num_proc_channels, int sample_rate_hz) { } int GainControlImpl::Configure() { - rtc::CritScope cs_render(crit_render_); - rtc::CritScope cs_capture(crit_capture_); WebRtcAgcConfig config; // TODO(ajm): Flip the sign here (since AGC expects a positive value) if we // change the interface. diff --git a/modules/audio_processing/gain_control_impl.h b/modules/audio_processing/gain_control_impl.h index fc781d46ff..9412e88bc1 100644 --- a/modules/audio_processing/gain_control_impl.h +++ b/modules/audio_processing/gain_control_impl.h @@ -30,8 +30,7 @@ class AudioBuffer; class GainControlImpl : public GainControl { public: - GainControlImpl(rtc::CriticalSection* crit_render, - rtc::CriticalSection* crit_capture); + GainControlImpl(rtc::CriticalSection* crit_capture); ~GainControlImpl() override; void ProcessRenderAudio(rtc::ArrayView packed_render_audio); @@ -67,9 +66,8 @@ class GainControlImpl : public GainControl { int analog_level_maximum() const override; bool stream_is_saturated() const override; - int Configure(); + int Configure() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_capture_); - rtc::CriticalSection* const crit_render_ RTC_ACQUIRED_BEFORE(crit_capture_); rtc::CriticalSection* const crit_capture_; std::unique_ptr data_dumper_; diff --git a/modules/audio_processing/gain_control_unittest.cc b/modules/audio_processing/gain_control_unittest.cc index 62908c7aec..891e78a394 100644 --- a/modules/audio_processing/gain_control_unittest.cc +++ b/modules/audio_processing/gain_control_unittest.cc @@ -72,9 +72,8 @@ void RunBitExactnessTest(int sample_rate_hz, int analog_level_max, int achieved_stream_analog_level_reference, rtc::ArrayView output_reference) { - rtc::CriticalSection crit_render; rtc::CriticalSection crit_capture; - GainControlImpl gain_controller(&crit_render, &crit_capture); + GainControlImpl gain_controller(&crit_capture); SetupComponent(sample_rate_hz, mode, target_level_dbfs, stream_analog_level, compression_gain_db, enable_limiter, analog_level_min, analog_level_max, &gain_controller); diff --git a/test/fuzzers/agc_fuzzer.cc b/test/fuzzers/agc_fuzzer.cc index f2c90480c8..9854a78ddb 100644 --- a/test/fuzzers/agc_fuzzer.cc +++ b/test/fuzzers/agc_fuzzer.cc @@ -114,8 +114,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { } test::FuzzDataHelper fuzz_data(rtc::ArrayView(data, size)); rtc::CriticalSection crit_capture; - rtc::CriticalSection crit_render; - auto gci = absl::make_unique(&crit_render, &crit_capture); + auto gci = absl::make_unique(&crit_capture); FuzzGainController(&fuzz_data, gci.get()); } } // namespace webrtc