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}

modules/audio_processing/audio_processing_impl.cc

@@ -387,48 +387,42 @@ AudioProcessingImpl::AudioProcessingImpl(
       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);
 }
 

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.

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<const int16_t> 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<ApmDataDumper> data_dumper_;

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<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);

test/fuzzers/agc_fuzzer.cc

@@ -114,8 +114,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) {
   }
   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