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 e75930ef76c9c2fa919fb5661ca17615929d25d2..460803a3e91a435bd429450b1aa36a1f0ae31007 100644
--- a/webrtc/modules/audio_processing/echo_cancellation_impl.cc
+++ b/webrtc/modules/audio_processing/echo_cancellation_impl.cc
@@ -18,7 +18,6 @@ extern "C" {
 }
 #include "webrtc/modules/audio_processing/aec/echo_cancellation.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
-#include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 
 namespace webrtc {
 
@@ -64,11 +63,13 @@ static const size_t kMaxNumFramesToBuffer = 100;
 
 EchoCancellationImpl::EchoCancellationImpl(
     const AudioProcessing* apm,
-    CriticalSectionWrapper* crit,
+    rtc::CriticalSection* crit_render,
+    rtc::CriticalSection* crit_capture,
     const 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),
@@ -120,7 +121,11 @@ int EchoCancellationImpl::ProcessRenderAudio(const 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);
@@ -215,7 +220,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;
@@ -225,11 +232,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;
   }
@@ -240,42 +248,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_);

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

It's great that you have added these. Thank you!

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

You are welcome! :-)
Acknowledged.

   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;
   }
@@ -318,36 +331,53 @@ int EchoCancellationImpl::GetMetrics(Metrics* metrics) {
 }
 
 bool EchoCancellationImpl::stream_has_echo() const {
+  // This is called both from within and from outside of APM, with and
+  // without locks set. Hence the conditional lock handling below.
+  rtc::TryCritScope cs(crit_capture_);

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

I don't understand why you are using TryCritScope in this way. You are not sure
after instantiating this object that you have taken the critical section, and
below you do not verify, using TryCritSect::locked() that you have the lock,
before operating on the data that, presumably, you set out to protect.

For instance, on Windows, TryEnterCriticalSection() is used and here's
documentation for that:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686857(v=vs.85).aspx

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

Agree, something is wrong with this. I'll revise. Please check so that it makes
sense now.

+
+  // Call the lock function (without this call TryCritScope will DCHECK).

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

TryCritScope::locked() does *not* call any lock function. It returns a bool
telling the client whether the lock was taken or not. The DCHECK fails so that
you don't forget to check whether the lock was taken. Here it looks like you're
bypassing that check by ignoring the return value.

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

Of course you are correct, I'll revise. Please re-review!

+  (void)cs.locked();
+

[kwiberg-webrtc, 2015/11/23 22:15:11]

This doesn't look right. The effect here is to take the lock if no one else is
holding it; this leaves the member access below possibly unprotected.

It seems better to have two variants of this method: one that locks
(unconditionally), and one that documents that the caller must already hold the
lock.

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

I agree on both, it was not correct, but I revised it now so now it should be
ok. I think it worked out fine anyway because these are most likely anyway
called by the same thread but the code was definitely not correct as it was.


I agree about the two variants of the method, but please look first to see
whether the refactored code is fine. 
I'd rather avoid two variants of the method if possible, as this then would need
to apply to several methods, those methods would need to be made public in the
API, and therefore anyone outside the API could call both methods (a DCHECK will
partially ensure proper behavior but it is not a 100% guarantee that this method
will not be called in Release mode) . This would increase the public APM API in
a messy way when what we really want is to go in the direction of decreasing the
public APM API.

   return stream_has_echo_;
 }
 
 int EchoCancellationImpl::enable_delay_logging(bool enable) {
-  CriticalSectionScoped crit_scoped(crit_);
+  // This is called both from within and from outside of APM, with and
+  // without locks set. Hence the conditional lock handling below.
+  rtc::TryCritScope cs(crit_capture_);
+
+  // Call the lock function (without this call TryCritScope will DCHECK).
+  (void)cs.locked();
+

[kwiberg-webrtc, 2015/11/23 22:15:11]

Same.

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

Agree, please look at the revised solution.

   delay_logging_enabled_ = enable;
   return Configure();

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

You may be setting delay_logging_enabled_ and calling Configure() without having
a lock here.

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

Agree, that was a bad design, but the revised code works I think.

 }
 
 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;
   }
@@ -370,7 +400,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;
   }
@@ -379,6 +409,7 @@ struct AecCore* EchoCancellationImpl::aec_core() const {
 }
 
 int EchoCancellationImpl::Initialize() {
+  // Only called from within APM, hence no locking is needed.

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

What do you mean with "called from within APM"? All of this is "within APM",
right? :)
The machine and compiler do not read these comments. Assert your preconditions
instead.

For instance, you can:
1. Call IsLocked() on your critical sections to verify they have been taken by
an outer layer.
2. Use ThreadChecker.
3. Build your own logic to verify the context is what you intend.

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

No, it is actually not, although I think I see your point. What I mean is that
this function is only called from within APM, which means that it is called
within a locking scheme that is owned by APM (a bit fuzzily phrased, but please
ask if it was not clear). For some of the functions, that is not the case as
they may be called directly from functions that do not belong to APM (due to the
scheme of APM allowing pointers to submodules being passed to other functions
that then can use them to access the public APIs to the subfunctions).

After today's discussion, I think it is best to address this in one more chained
CL. Does that sound ok?

   int err = ProcessingComponent::Initialize();
   if (err != apm_->kNoError || !is_component_enabled()) {
     return err;
@@ -414,21 +445,31 @@ 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.
+  rtc::TryCritScope cs(crit_capture_);
+
+  // Call the lock function (without this call TryCritScope will DCHECK).
+  (void)cs.locked();
+

[kwiberg-webrtc, 2015/11/23 22:15:11]

Same.

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

Agree. Please see the revised version.

   extended_filter_enabled_ = config.Get<ExtendedFilter>().enabled;
   delay_agnostic_enabled_ = config.Get<DelayAgnostic>().enabled;
   Configure();
 }
 
 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.
@@ -439,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.

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

This function is called from ProcessingComponent::Configure(), which is
triggered from EchoCancellationImpl::enable_delay_logging() and
EchoCancellationImpl::enable_delay_logging(), where you are using TryCritScope
in a way I don't understand and which looks like you are not actually taking the
lock.

Please, explain why this works and what I misunderstand.

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

You are right in that the lock was not properly acquired. But with the current
construction it should be fine.

   assert(handle != NULL);
   AecConfig config;
   config.metricsMode = metrics_enabled_;
@@ -455,11 +497,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;
 }