Lock scheme #8: Introduced the new locking scheme

webrtc/modules/audio_processing/gain_control_impl.cc

Index: webrtc/modules/audio_processing/gain_control_impl.cc
diff --git a/webrtc/modules/audio_processing/gain_control_impl.cc b/webrtc/modules/audio_processing/gain_control_impl.cc
index 0f8dd88e532b982c841b3eb090ddbc88947eb10b..516c89a2f9c4b402afcb2b20170e1328381b6f09 100644
--- a/webrtc/modules/audio_processing/gain_control_impl.cc
+++ b/webrtc/modules/audio_processing/gain_control_impl.cc
@@ -14,7 +14,6 @@
 
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/agc/legacy/gain_control.h"
-#include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 
 namespace webrtc {
 
@@ -45,12 +44,14 @@ static const size_t kMaxNumFramesToBuffer = 100;
 
 GainControlImpl::GainControlImpl(
     const AudioProcessing* apm,
-    CriticalSectionWrapper* crit,
+    rtc::CriticalSection* crit_render,
+    rtc::CriticalSection* crit_capture,
     const rtc::ThreadChecker* render_thread_checker,
     const rtc::ThreadChecker* capture_thread_checker)
     : ProcessingComponent(),
       apm_(apm),
-      crit_(crit),
+      crit_render_(crit_render),
+      crit_capture_(crit_capture),
       render_thread_checker_(render_thread_checker),
       capture_thread_checker_(capture_thread_checker),
       mode_(kAdaptiveAnalog),
@@ -91,7 +92,11 @@ int GainControlImpl::ProcessRenderAudio(AudioBuffer* audio) {
 
   // Insert the samples into the queue.
   if (!render_signal_queue_->Insert(&render_queue_buffer_)) {
-    ReadQueuedRenderData();
+    // The data queue is full and needs to be emptied.
+    {
+      rtc::CritScope cs_capture(crit_capture_);
+      ReadQueuedRenderData();
+    }
 
     // Retry the insert (should always work).
     RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true);
@@ -227,7 +232,8 @@ int GainControlImpl::ProcessCaptureAudio(AudioBuffer* audio) {
 // TODO(ajm): ensure this is called under kAdaptiveAnalog.
 int GainControlImpl::set_stream_analog_level(int level) {
   RTC_DCHECK(capture_thread_checker_->CalledOnValidThread());
-  CriticalSectionScoped crit_scoped(crit_);
+
+  rtc::CritScope cs(crit_capture_);
   was_analog_level_set_ = true;
   if (level < minimum_capture_level_ || level > maximum_capture_level_) {
     return apm_->kBadParameterError;
@@ -238,6 +244,7 @@ int GainControlImpl::set_stream_analog_level(int level) {
 }
 
 int GainControlImpl::stream_analog_level() {
+  rtc::CritScope cs(crit_capture_);
   RTC_DCHECK(capture_thread_checker_->CalledOnValidThread());
   // TODO(ajm): enable this assertion?
   //assert(mode_ == kAdaptiveAnalog);
@@ -246,16 +253,19 @@ int GainControlImpl::stream_analog_level() {
 }
 
 int GainControlImpl::Enable(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs_render(crit_render_);
+  rtc::CritScope cs(crit_capture_);

[the sun, 2015/11/23 21:36:05]

Why do you sometimes use "cs_capture", and sometimes "cs"?

[peah-webrtc, 2015/11/24 21:42:24]

The scheme is to only use cs, when there is only one critical section active
(one lock acquired), and if two critical sections are active
cs_render/cs_capture is used to distinguish between them. But there clearly was
a miss here on that. Please let me know if you find any other place where that
happened!

Done.

   return EnableComponent(enable);
 }
 
 bool GainControlImpl::is_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return is_component_enabled();
 }
 
 int GainControlImpl::set_mode(Mode mode) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs_render(crit_render_);
+  rtc::CritScope cs_capture(crit_capture_);
   if (MapSetting(mode) == -1) {
     return apm_->kBadParameterError;
   }
@@ -265,12 +275,13 @@ int GainControlImpl::set_mode(Mode mode) {
 }
 
 GainControl::Mode GainControlImpl::mode() const {
+  rtc::CritScope cs(crit_capture_);
   return mode_;
 }
 
 int GainControlImpl::set_analog_level_limits(int minimum,
                                              int maximum) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (minimum < 0) {
     return apm_->kBadParameterError;
   }
@@ -290,19 +301,22 @@ int GainControlImpl::set_analog_level_limits(int minimum,
 }
 
 int GainControlImpl::analog_level_minimum() const {
+  rtc::CritScope cs(crit_capture_);
   return minimum_capture_level_;
 }
 
 int GainControlImpl::analog_level_maximum() const {
+  rtc::CritScope cs(crit_capture_);
   return maximum_capture_level_;
 }
 
 bool GainControlImpl::stream_is_saturated() const {
+  rtc::CritScope cs(crit_capture_);
   return stream_is_saturated_;
 }
 
 int GainControlImpl::set_target_level_dbfs(int level) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (level > 31 || level < 0) {
     return apm_->kBadParameterError;
   }
@@ -312,11 +326,12 @@ int GainControlImpl::set_target_level_dbfs(int level) {
 }
 
 int GainControlImpl::target_level_dbfs() const {
+  rtc::CritScope cs(crit_capture_);
   return target_level_dbfs_;
 }
 
 int GainControlImpl::set_compression_gain_db(int gain) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (gain < 0 || gain > 90) {
     return apm_->kBadParameterError;
   }
@@ -326,16 +341,18 @@ int GainControlImpl::set_compression_gain_db(int gain) {
 }
 
 int GainControlImpl::compression_gain_db() const {
+  rtc::CritScope cs(crit_capture_);
   return compression_gain_db_;
 }
 
 int GainControlImpl::enable_limiter(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   limiter_enabled_ = enable;
   return Configure();
 }
 
 bool GainControlImpl::is_limiter_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return limiter_enabled_;
 }
 
@@ -354,6 +371,7 @@ int GainControlImpl::Initialize() {
 }
 
 void GainControlImpl::AllocateRenderQueue() {
+  // Only called from within APM, hence no locking is needed.
   const size_t new_render_queue_element_max_size =
       std::max<size_t>(static_cast<size_t>(1),
                        kMaxAllowedValuesOfSamplesPerFrame * num_handles());
@@ -375,14 +393,17 @@ void GainControlImpl::AllocateRenderQueue() {
 }
 
 void* GainControlImpl::CreateHandle() const {
+  // Only called from within APM, hence no locking is needed.
   return WebRtcAgc_Create();
 }
 
 void GainControlImpl::DestroyHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   WebRtcAgc_Free(static_cast<Handle*>(handle));
 }
 
 int GainControlImpl::InitializeHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   return WebRtcAgc_Init(static_cast<Handle*>(handle),
                           minimum_capture_level_,
                           maximum_capture_level_,
@@ -391,6 +412,7 @@ int GainControlImpl::InitializeHandle(void* handle) const {
 }
 
 int GainControlImpl::ConfigureHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   WebRtcAgcConfig config;
   // TODO(ajm): Flip the sign here (since AGC expects a positive value) if we
   //            change the interface.
@@ -405,10 +427,12 @@ int GainControlImpl::ConfigureHandle(void* handle) const {
 }
 
 int GainControlImpl::num_handles_required() const {
+  // Only called from within APM, hence no locking is needed.
   return apm_->num_output_channels();
 }
 
 int GainControlImpl::GetHandleError(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   // The AGC has no get_error() function.
   // (Despite listing errors in its interface...)
   assert(handle != NULL);