Lock scheme #8: Introduced the new locking scheme

webrtc/modules/audio_processing/echo_cancellation_impl.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/audio_processing/echo_cancellation_impl.h"
 
 #include <assert.h>
 #include <string.h>
 
 extern "C" {
 #include "webrtc/modules/audio_processing/aec/aec_core.h"
 }
 #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 {
 
 typedef void Handle;
 
 namespace {
 int16_t MapSetting(EchoCancellation::SuppressionLevel level) {
   switch (level) {
     case EchoCancellation::kLowSuppression:
       return kAecNlpConservative;
@@ skipping 25 matching lines @@
 // Maximum length that a frame of samples can have.
 static const size_t kMaxAllowedValuesOfSamplesPerFrame = 160;
 // Maximum number of frames to buffer in the render queue.
 // TODO(peah): Decrease this once we properly handle hugely unbalanced
 // reverse and forward call numbers.
 static const size_t kMaxNumFramesToBuffer = 100;
 }  // namespace
 
 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),
       suppression_level_(kModerateSuppression),
       stream_drift_samples_(0),
       was_stream_drift_set_(false),
       stream_has_echo_(false),
       delay_logging_enabled_(false),
       extended_filter_enabled_(false),
       delay_agnostic_enabled_(false),
       render_queue_element_max_size_(0) {}

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

crit_render and crit_capture must be set; that's part of this class' invariant.
You should:
RTC_DCHECK(crit_render_);
RTC_DCHECK(crit_capture_);

Here, and the other modules that also take these as arguments.

 
 EchoCancellationImpl::~EchoCancellationImpl() {}
 
 int EchoCancellationImpl::ProcessRenderAudio(const AudioBuffer* audio) {
   RTC_DCHECK(render_thread_checker_->CalledOnValidThread());
   if (!is_component_enabled()) {
     return apm_->kNoError;
   }
 
   assert(audio->num_frames_per_band() <= 160);
@@ skipping 20 matching lines @@
       // Buffer the samples in the render queue.
       render_queue_buffer_.insert(render_queue_buffer_.end(),
                                   audio->split_bands_const_f(j)[kBand0To8kHz],
                                   (audio->split_bands_const_f(j)[kBand0To8kHz] +
                                    audio->num_frames_per_band()));
     }
   }
 
   // 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);
   }
 
   return apm_->kNoError;
 }
 
 // Read chunks of data that were received and queued on the render side from
 // a queue. All the data chunks are buffered into the farend signal of the AEC.
@@ skipping 74 matching lines @@
 
       handle_index++;
     }
   }
 
   was_stream_drift_set_ = false;
   return apm_->kNoError;
 }
 
 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;
   }
 
   return EnableComponent(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;
   }
 
   suppression_level_ = level;
   return Configure();
 }
 
 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;
   }
 
   if (!is_component_enabled() || !metrics_enabled_) {
     return apm_->kNotEnabledError;
   }
 
   AecMetrics my_metrics;
   memset(&my_metrics, 0, sizeof(my_metrics));
@@ skipping 22 matching lines @@
 
   metrics->a_nlp.instant = my_metrics.aNlp.instant;
   metrics->a_nlp.average = my_metrics.aNlp.average;
   metrics->a_nlp.maximum = my_metrics.aNlp.max;
   metrics->a_nlp.minimum = my_metrics.aNlp.min;
 
   return apm_->kNoError;
 }
 
 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;
   }
   if (std == NULL) {
     return apm_->kNullPointerError;
   }
 
   if (!is_component_enabled() || !delay_logging_enabled_) {
     return apm_->kNotEnabledError;
   }
 
   Handle* my_handle = static_cast<Handle*>(handle(0));
   const int err =
       WebRtcAec_GetDelayMetrics(my_handle, median, std, fraction_poor_delays);
   if (err != apm_->kNoError) {
     return MapError(err);
   }
 
   return apm_->kNoError;
 }
 
 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;
   }
   Handle* my_handle = static_cast<Handle*>(handle(0));
   return WebRtcAec_aec_core(my_handle);
 }
 
 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;
   }
 
   AllocateRenderQueue();
 
   return apm_->kNoError;
 }
 
@@ skipping 15 matching lines @@
             RenderQueueItemVerifier<float>(render_queue_element_max_size_)));
 
     render_queue_buffer_.resize(render_queue_element_max_size_);
     capture_queue_buffer_.resize(render_queue_element_max_size_);
   } else {
     render_signal_queue_->Clear();
   }
 }
 
 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.
   // For now, just hardcode a 48 kHz value.
   return WebRtcAec_Init(static_cast<Handle*>(handle),
                        apm_->proc_sample_rate_hz(),
                        48000);
 }
 
 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_;
   config.nlpMode = MapSetting(suppression_level_);
   config.skewMode = drift_compensation_enabled_;
   config.delay_logging = delay_logging_enabled_;
   WebRtcAec_enable_extended_filter(
       WebRtcAec_aec_core(static_cast<Handle*>(handle)),
       extended_filter_enabled_ ? 1 : 0);
   WebRtcAec_enable_delay_agnostic(
       WebRtcAec_aec_core(static_cast<Handle*>(handle)),
       delay_agnostic_enabled_ ? 1 : 0);
   return WebRtcAec_set_config(static_cast<Handle*>(handle), config);
 }
 
 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;
 }
 }  // namespace webrtc