Less lock acquisitions for AudioMixer.

webrtc/modules/audio_mixer/audio_mixer_impl.h

Index: webrtc/modules/audio_mixer/audio_mixer_impl.h
diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.h b/webrtc/modules/audio_mixer/audio_mixer_impl.h
index 6ddf9ca5e8bde5c6954d458a6cfcdba8a902f9e7..ca8bdbeaed5ffe056a8d2eecd84ab37867edda8a 100644
--- a/webrtc/modules/audio_mixer/audio_mixer_impl.h
+++ b/webrtc/modules/audio_mixer/audio_mixer_impl.h
@@ -89,7 +89,8 @@ class AudioMixerImpl : public AudioMixer {
   // Compute what audio sources to mix from audio_source_list_. Ramp in
   // and out. Update mixed status. maxAudioFrameCounter specifies how
   // many participants are allowed to be mixed.
-  AudioFrameList UpdateToMix(size_t maxAudioFrameCounter) const;
+  AudioFrameList UpdateToMix(size_t maxAudioFrameCounter) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   // Return the lowest mixing frequency that can be used without having to
   // downsample any audio.
@@ -98,7 +99,8 @@ class AudioMixerImpl : public AudioMixer {
       const MixerAudioSourceList& mixList) const;
 
   // Return the AudioFrames that should be mixed anonymously.
-  void GetAdditionalAudio(AudioFrameList* additionalFramesList) const;
+  void GetAdditionalAudio(AudioFrameList* additionalFramesList) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_);
 
   // This function returns true if it finds the MixerAudioSource in the
   // specified list of MixerAudioSources.
@@ -132,7 +134,6 @@ class AudioMixerImpl : public AudioMixer {
   int GetOutputAudioLevelFullRange() override;
 
   std::unique_ptr<CriticalSectionWrapper> crit_;
-  std::unique_ptr<CriticalSectionWrapper> cb_crit_;
 
   int32_t id_;
 
@@ -141,12 +142,12 @@ class AudioMixerImpl : public AudioMixer {
   size_t sample_size_;
 
   // List of all audio sources. Note all lists are disjunct
-  MixerAudioSourceList audio_source_list_;  // May be mixed.
+  MixerAudioSourceList audio_source_list_ GUARDED_BY(crit_);  // May be mixed.
 
   // Always mixed, anonomously.
-  MixerAudioSourceList additional_audio_source_list_;
+  MixerAudioSourceList additional_audio_source_list_ GUARDED_BY(crit_);
 
-  size_t num_mixed_audio_sources_;
+  size_t num_mixed_audio_sources_ GUARDED_BY(crit_);

[kwiberg-webrtc, 2016/09/01 11:19:00]

Good, but it makes me very suspicious that this class has a large number of
member variables that aren't GUARDED_BY(crit_).

[aleloi, 2016/09/01 11:24:39]

The other ones are only accessed by the mixer functions, which should only run
from a single thread. The entry function Mix has a checker for that.

The only multithreaded ones are add/remove/query participants, and they only
access the GUARDED_BY vars.

[kwiberg-webrtc, 2016/09/01 11:32:28]

Could you annotate them with the thread checker stuff I pointed to in an earlier
comment? (I think thread checker annotations are rather new, so I have no
personal experience of using them, but it looks like it'd be the right thing to
use here.)

Having all the member variables annotated will make it plain what's being
protected by what. With this many variables and this much code, avoiding
mistakes by hand is too hard.

[aleloi, 2016/09/01 12:33:06]

Good point! I've followed the example usage instructions in
base/thread_checker.h and also how it was implemented in e.g.
webrtc/video/rtp_streams_synchronizer.h

The id_ field is accessed everywhere, but only read from, so I made it 'const'.

The limiter_ is problematic. It is initialized in Init(), which is reasonable,
since initialization could fail. But it's then accessed on the mixing thread,
which could be different. I don't think one can express 'accessed by a single
thread starting at the first call to ::Mix()'.

It seems wrong to leave the limiter as is. If there is no better choice, I will
add a comment describing its access status.

[kwiberg-webrtc, 2016/09/02 07:45:45]

Excellent! Did you verify that you got the expected compilation errors when
accessing member variables from methods where you hadn't yet added the thread
checker call?
Excellent choice.
Hmmm. Too bad we can't just throw an exception in the constructor...

The problem is that the compiler doesn't know that you promise to call Init()
once in the beginning and then never again, so it's doing the right thing when
it complains. The root of the evil is the use of an Init() function, which
results in a special constructed-but-not-yet-initialized state that's a pain to
deal with.

Could you make the constructor private, and have a Create() function instead of
Init()? Then the Create() function could create all the stuff that might fail,
and pass them as arguments to the private constructor. That way, we get rid of
the constructed-but-not-yet-initialized state.

   // Determines if we will use a limiter for clipping protection during
   // mixing.
   bool use_limiter_;