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 <alessiob@webrtc.org>
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26637}diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc
index 5f0d676..3400abf 100644
--- a/modules/audio_processing/audio_processing_impl.cc
+++ b/modules/audio_processing/audio_processing_impl.cc
@@ -387,48 +387,42 @@
capture_(config.Get<ExperimentalNs>().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<bool>(echo_control_factory_);
- // Mark Echo Controller enabled if a factory is injected.
- capture_nonlocked_.echo_controller_enabled =
- static_cast<bool>(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<ResidualEchoDetector>();
- }
-
- 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<ResidualEchoDetector>();
}
+ 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 26ab1b4..cd21e4c 100644
--- a/modules/audio_processing/gain_control_impl.cc
+++ b/modules/audio_processing/gain_control_impl.cc
@@ -89,10 +89,8 @@
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 @@
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::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 @@
}
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 @@
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 @@
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 @@
}
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 @@
}
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 fc781d4..9412e88 100644
--- a/modules/audio_processing/gain_control_impl.h
+++ b/modules/audio_processing/gain_control_impl.h
@@ -30,8 +30,7 @@
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<const int16_t> packed_render_audio);
@@ -67,9 +66,8 @@
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<ApmDataDumper> data_dumper_;
diff --git a/modules/audio_processing/gain_control_unittest.cc b/modules/audio_processing/gain_control_unittest.cc
index 62908c7..891e78a 100644
--- a/modules/audio_processing/gain_control_unittest.cc
+++ b/modules/audio_processing/gain_control_unittest.cc
@@ -72,9 +72,8 @@
int analog_level_max,
int achieved_stream_analog_level_reference,
rtc::ArrayView<const float> 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 f2c9048..9854a78 100644
--- a/test/fuzzers/agc_fuzzer.cc
+++ b/test/fuzzers/agc_fuzzer.cc
@@ -114,8 +114,7 @@
}
test::FuzzDataHelper fuzz_data(rtc::ArrayView<const uint8_t>(data, size));
rtc::CriticalSection crit_capture;
- rtc::CriticalSection crit_render;
- auto gci = absl::make_unique<GainControlImpl>(&crit_render, &crit_capture);
+ auto gci = absl::make_unique<GainControlImpl>(&crit_capture);
FuzzGainController(&fuzz_data, gci.get());
}
} // namespace webrtc
