Added locking when getting echo likelihood stats.

webrtc/modules/audio_processing/audio_processing_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.
  */
 
@@ skipping 306 matching lines @@
 #if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS)
       capture_(false,
 #else
       capture_(config.Get<ExperimentalNs>().enabled,
 #endif
                config.Get<Beamforming>().array_geometry,
                config.Get<Beamforming>().target_direction),
       capture_nonlocked_(config.Get<Beamforming>().enabled,
                          config.Get<Intelligibility>().enabled) {
   {
     rtc::CritScope cs_render(&crit_render_);

[tommi, 2017/03/15 16:14:45]

... any idea why we're grabbing these locks in the constructor?

Maybe the public/private modules could have a constructor that accepts all these
pointers, RAII style?

[ivoc, 2017/03/15 17:14:15]

Good question. I didn't write this code, so I'm not completely sure, but it
might be to make sure that no external threads are calling these submodules
while they're being constructed (the public submodules are exposed to other
threads). Although I guess that might still happen before this lock is taken (so
this would be a race).

[tommi, 2017/03/15 17:27:59]

OK, thanks. I didn't mean to derail the review :)  Constructing an object should
be an operation during which the object should not have to worry about its state
being poked at from other threads.  So if these locks are actually protecting
against something like that, then we might have a bigger problem. 
If you do happen to find out something, then it would be nice to add a comment
here :)

[ivoc, 2017/03/15 19:30:44]

I agree that this looks fishy. I'm sure that when we refactor the APM locking
this can be solved.

     rtc::CritScope cs_capture(&crit_capture_);
 
     public_submodules_->echo_cancellation.reset(
         new EchoCancellationImpl(&crit_render_, &crit_capture_));
     public_submodules_->echo_control_mobile.reset(
         new EchoControlMobileImpl(&crit_render_, &crit_capture_));
     public_submodules_->gain_control.reset(
         new GainControlImpl(&crit_capture_, &crit_capture_));
     public_submodules_->level_estimator.reset(
         new LevelEstimatorImpl(&crit_capture_));
@@ skipping 1271 matching lines @@
   EchoCancellation::Metrics metrics;
   int success = public_submodules_->echo_cancellation->GetMetrics(&metrics);
   if (success == Error::kNoError) {
     stats.a_nlp.Set(metrics.a_nlp);
     stats.divergent_filter_fraction = metrics.divergent_filter_fraction;
     stats.echo_return_loss.Set(metrics.echo_return_loss);
     stats.echo_return_loss_enhancement.Set(
         metrics.echo_return_loss_enhancement);
     stats.residual_echo_return_loss.Set(metrics.residual_echo_return_loss);
   }
-  stats.residual_echo_likelihood =
-      private_submodules_->residual_echo_detector->echo_likelihood();
-  stats.residual_echo_likelihood_recent_max =
-      private_submodules_->residual_echo_detector->echo_likelihood_recent_max();
+  {
+    rtc::CritScope cs_capture(&crit_capture_);
+    stats.residual_echo_likelihood =
+        private_submodules_->residual_echo_detector->echo_likelihood();

[tommi, 2017/03/15 16:14:45]

could we instead make sure that either these stats get updated on the thread
where GetStatistics is called on or that stats from |private_submodules_| are
queried from the thread where those stats are updated on?

As is, this feels like AudioProcessingImpl is applying synchronization outside
of its responsibilities. Knowing about implementation details of several classes
who's pointer is stored in another struct.

[ivoc, 2017/03/15 17:14:15]

This function is called from a Chromium thread (I think the render thread), so I
don't think it's possible to do the audio processing on the same thread as
getting the stats, some sort of synchronization is inevitable.

I agree that the locking in APM and its submodules is more complicated than it
needs to be. One of the main reasons is that the internal submodules are exposed
and used from various different threads (some of which are Chromium threads). I
think it makes sense to refactor APM so that all submodules are private, and
only APM is exposed externally. That way we can solve the locking/threading
issues in this class, and make the submodules lock free. 

[tommi, 2017/03/15 17:27:59]

From what I can tell, it's actually called from our signaling thread:

https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?...

It would be nice if we could start adding some thread checks as we fix thread
issues, that both document the code and help us catching regressions.  As we do
that, the bigger picture becomes a little clearer bit by bit and when we have
those things laid out, it becomes easy to remove locks where we don't need them
and at the same time document the code.

To make what I mean a little clearer, here are a couple of examples. Look for
"TODO(tommi)" in these diffs:
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...

[ivoc, 2017/03/15 19:30:43]

Yes, that's one place this is called from. Another is here:
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_audi...
Thanks for the examples, those will be useful when we refactor the locking in
APM. 

+    stats.residual_echo_likelihood_recent_max =
+        private_submodules_->residual_echo_detector
+            ->echo_likelihood_recent_max();
+  }
   public_submodules_->echo_cancellation->GetDelayMetrics(
       &stats.delay_median, &stats.delay_standard_deviation,
       &stats.fraction_poor_delays);
   return stats;
 }
 
 EchoCancellation* AudioProcessingImpl::echo_cancellation() const {
   return public_submodules_->echo_cancellation.get();
 }
 
@@ skipping 352 matching lines @@
       capture_processing_format(kSampleRate16kHz),
       split_rate(kSampleRate16kHz) {}
 
 AudioProcessingImpl::ApmCaptureState::~ApmCaptureState() = default;
 
 AudioProcessingImpl::ApmRenderState::ApmRenderState() = default;
 
 AudioProcessingImpl::ApmRenderState::~ApmRenderState() = default;
 
 }  // namespace webrtc