Added locking when getting echo likelihood stats.

webrtc/modules/audio_processing/audio_processing_impl.cc

Index: webrtc/modules/audio_processing/audio_processing_impl.cc
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc
index 4c9188f7682df48404c8d70ce31e6ec2dc944ad0..1f73c5984a68af29d7dd941a64f88a834fd9e72a 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -1616,10 +1616,14 @@ AudioProcessing::AudioProcessingStatistics AudioProcessingImpl::GetStatistics()
         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);