Lock scheme #8: Introduced the new locking scheme

webrtc/modules/audio_processing/echo_cancellation_impl.cc

Index: webrtc/modules/audio_processing/echo_cancellation_impl.cc
diff --git a/webrtc/modules/audio_processing/echo_cancellation_impl.cc b/webrtc/modules/audio_processing/echo_cancellation_impl.cc
index abca59f723ecd3835ad38191bdcaa2ab2351fbd9..e672573a67e65c8f9a600a92b8134e7c5cae2653 100644
--- a/webrtc/modules/audio_processing/echo_cancellation_impl.cc
+++ b/webrtc/modules/audio_processing/echo_cancellation_impl.cc
@@ -16,9 +16,9 @@
 extern "C" {
 #include "webrtc/modules/audio_processing/aec/aec_core.h"
 }
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/modules/audio_processing/aec/include/echo_cancellation.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
-#include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 
 namespace webrtc {
 
@@ -60,11 +60,13 @@ const size_t EchoCancellationImpl::kAllowedValuesOfSamplesPerFrame2;
 
 EchoCancellationImpl::EchoCancellationImpl(
     const AudioProcessing* apm,
-    CriticalSectionWrapper* crit,
+    rtc::CriticalSection* crit_render,
+    rtc::CriticalSection* crit_capture,
     rtc::ThreadChecker* render_thread_checker)
     : ProcessingComponent(),
       apm_(apm),
-      crit_(crit),
+      crit_render_(crit_render),
+      crit_capture_(crit_capture),
       render_thread_checker_(render_thread_checker),
       drift_compensation_enabled_(false),
       metrics_enabled_(false),
@@ -116,9 +118,18 @@ int EchoCancellationImpl::ProcessRenderAudio(const AudioBuffer* audio) {
     }
   }
 
-  // Check of success is temporarily disabled as it breaks a unit test.
-  // TODO(peah): Will be fixed in the next CL.
-  (void)render_signal_queue_->Insert(&render_queue_buffer_);
+  // Insert the samples into the queue.
+  bool success = render_signal_queue_->Insert(&render_queue_buffer_);

[hlundin-webrtc, 2015/11/05 16:11:22]

You don't need success.
if (!render_signal_queue_->Insert(&render_queue_buffer_)) {
...

[peah-webrtc, 2015/11/06 09:54:32]

Done.

+  if (!success) {
+    // 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);
+  }
 
   return apm_->kNoError;
 }
@@ -210,7 +221,9 @@ int EchoCancellationImpl::ProcessCaptureAudio(AudioBuffer* audio) {
 }
 
 int EchoCancellationImpl::Enable(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  // Run in a single-threaded manner.
+  rtc::CritScope cs_render(crit_render_);
+  rtc::CritScope cs_capture(crit_capture_);
   // Ensure AEC and AECM are not both enabled.
   if (enable && apm_->echo_control_mobile()->is_enabled()) {
     return apm_->kBadParameterError;
@@ -220,11 +233,12 @@ int EchoCancellationImpl::Enable(bool enable) {
 }
 
 bool EchoCancellationImpl::is_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return is_component_enabled();
 }
 
 int EchoCancellationImpl::set_suppression_level(SuppressionLevel level) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (MapSetting(level) == -1) {
     return apm_->kBadParameterError;
   }
@@ -235,42 +249,47 @@ int EchoCancellationImpl::set_suppression_level(SuppressionLevel level) {
 
 EchoCancellation::SuppressionLevel EchoCancellationImpl::suppression_level()
     const {
+  rtc::CritScope cs(crit_capture_);
   return suppression_level_;
 }
 
 int EchoCancellationImpl::enable_drift_compensation(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   drift_compensation_enabled_ = enable;
   return Configure();
 }
 
 bool EchoCancellationImpl::is_drift_compensation_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return drift_compensation_enabled_;
 }
 
 void EchoCancellationImpl::set_stream_drift_samples(int drift) {
+  rtc::CritScope cs(crit_capture_);
   was_stream_drift_set_ = true;
   stream_drift_samples_ = drift;
 }
 
 int EchoCancellationImpl::stream_drift_samples() const {
+  rtc::CritScope cs(crit_capture_);
   return stream_drift_samples_;
 }
 
 int EchoCancellationImpl::enable_metrics(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   metrics_enabled_ = enable;
   return Configure();
 }
 
 bool EchoCancellationImpl::are_metrics_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return metrics_enabled_;
 }
 
 // TODO(ajm): we currently just use the metrics from the first AEC. Think more
 //            aboue the best way to extend this to multi-channel.
 int EchoCancellationImpl::GetMetrics(Metrics* metrics) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (metrics == NULL) {
     return apm_->kNullPointerError;
   }
@@ -313,36 +332,49 @@ int EchoCancellationImpl::GetMetrics(Metrics* metrics) {
 }
 
 bool EchoCancellationImpl::stream_has_echo() const {
-  return stream_has_echo_;
+  // This is called both from within and from outside of APM, with and
+  // without locks set. Hence the conditional lock handling below.
+  bool stream_has_echo;
+  if (crit_capture_->TryEnter()) {

[hlundin-webrtc, 2015/11/05 16:11:22]

I don't understand this code, but I wonder if rtc::TryCritScope is what you want
to use.

[peah-webrtc, 2015/11/06 09:54:32]

Sounds like that! Thanks!
Done.

+    stream_has_echo = stream_has_echo_;
+    crit_capture_->Leave();
+  } else {
+    stream_has_echo = stream_has_echo_;
+  }
+  return stream_has_echo;
 }
 
 int EchoCancellationImpl::enable_delay_logging(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   delay_logging_enabled_ = enable;
   return Configure();
 }
 
 bool EchoCancellationImpl::is_delay_logging_enabled() const {
+  rtc::CritScope cs(crit_capture_);
   return delay_logging_enabled_;
 }
 
 bool EchoCancellationImpl::is_delay_agnostic_enabled() const {
+  // Only called from within APM, hence no locking is needed.
   return delay_agnostic_enabled_;
 }
 
 bool EchoCancellationImpl::is_extended_filter_enabled() const {
+  // Only called from within APM, hence no locking is needed.
   return extended_filter_enabled_;
 }
 
 // TODO(bjornv): How should we handle the multi-channel case?
 int EchoCancellationImpl::GetDelayMetrics(int* median, int* std) {
+  rtc::CritScope cs(crit_capture_);
   float fraction_poor_delays = 0;
   return GetDelayMetrics(median, std, &fraction_poor_delays);
 }
 
 int EchoCancellationImpl::GetDelayMetrics(int* median, int* std,
                                           float* fraction_poor_delays) {
-  CriticalSectionScoped crit_scoped(crit_);
+  rtc::CritScope cs(crit_capture_);
   if (median == NULL) {
     return apm_->kNullPointerError;
   }
@@ -365,7 +397,7 @@ int EchoCancellationImpl::GetDelayMetrics(int* median, int* std,
 }
 
 struct AecCore* EchoCancellationImpl::aec_core() const {
-  CriticalSectionScoped crit_scoped(crit_);
+  // Only called from within APM, hence no locking is needed.
   if (!is_component_enabled()) {
     return NULL;
   }
@@ -374,6 +406,7 @@ struct AecCore* EchoCancellationImpl::aec_core() const {
 }
 
 int EchoCancellationImpl::Initialize() {
+  // Only called from within APM, hence no locking is needed.
   int err = ProcessingComponent::Initialize();
   if (err != apm_->kNoError || !is_component_enabled()) {
     return err;
@@ -385,6 +418,7 @@ int EchoCancellationImpl::Initialize() {
 }
 
 void EchoCancellationImpl::AllocateRenderQueue() {
+  // Only called from within APM, hence no locking is needed.
   const size_t max_frame_size = std::max(kAllowedValuesOfSamplesPerFrame1,
                                          kAllowedValuesOfSamplesPerFrame2);
 
@@ -412,21 +446,30 @@ void EchoCancellationImpl::AllocateRenderQueue() {
 }
 
 void EchoCancellationImpl::SetExtraOptions(const Config& config) {
+  // This is called both from within and from outside of APM, with and
+  // without locks set. Hence the conditional lock handling below.
+  bool locked_locally = crit_capture_->TryEnter();
   extended_filter_enabled_ = config.Get<ExtendedFilter>().enabled;
   delay_agnostic_enabled_ = config.Get<DelayAgnostic>().enabled;
   Configure();
+  if (locked_locally) {
+    crit_capture_->Leave();
+  }
 }
 
 void* EchoCancellationImpl::CreateHandle() const {
+  // Only called from within APM, hence no locking is needed.
   return WebRtcAec_Create();
 }
 
 void EchoCancellationImpl::DestroyHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   assert(handle != NULL);
   WebRtcAec_Free(static_cast<Handle*>(handle));
 }
 
 int EchoCancellationImpl::InitializeHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   assert(handle != NULL);
   // TODO(ajm): Drift compensation is disabled in practice. If restored, it
   // should be managed internally and not depend on the hardware sample rate.
@@ -437,6 +480,7 @@ int EchoCancellationImpl::InitializeHandle(void* handle) const {
 }
 
 int EchoCancellationImpl::ConfigureHandle(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   assert(handle != NULL);
   AecConfig config;
   config.metricsMode = metrics_enabled_;
@@ -454,11 +498,13 @@ int EchoCancellationImpl::ConfigureHandle(void* handle) const {
 }
 
 int EchoCancellationImpl::num_handles_required() const {
+  // Only called from within APM, hence no locking is needed.
   return apm_->num_output_channels() *
          apm_->num_reverse_channels();
 }
 
 int EchoCancellationImpl::GetHandleError(void* handle) const {
+  // Only called from within APM, hence no locking is needed.
   assert(handle != NULL);
   return AudioProcessing::kUnspecifiedError;
 }