Less lock acquisitions for AudioMixer.

webrtc/modules/audio_mixer/audio_mixer_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_mixer/audio_mixer_impl.h"
 
 #include <algorithm>
 #include <functional>
 
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_mixer/audio_frame_manipulator.h"
 #include "webrtc/modules/audio_mixer/audio_mixer_defines.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/utility/include/audio_frame_operations.h"
 #include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 #include "webrtc/system_wrappers/include/trace.h"
 
 namespace webrtc {
 namespace {
 
@@ skipping 114 matching lines @@
   thread_checker_.DetachFromThread();
 }
 
 AudioMixerImpl::~AudioMixerImpl() {}
 
 bool AudioMixerImpl::Init() {
   crit_.reset(CriticalSectionWrapper::CreateCriticalSection());
   if (crit_.get() == NULL)
     return false;
 
-  cb_crit_.reset(CriticalSectionWrapper::CreateCriticalSection());
-  if (cb_crit_.get() == NULL)
-    return false;
-
   Config config;
   config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
   limiter_.reset(AudioProcessing::Create(config));
   if (!limiter_.get())
     return false;
 
   if (SetOutputFrequency(kDefaultFrequency) == -1)
     return false;
 
   if (limiter_->gain_control()->set_mode(GainControl::kFixedDigital) !=
@@ skipping 17 matching lines @@
     return false;
 
   return true;
 }
 
 void AudioMixerImpl::Mix(int sample_rate,
                          size_t number_of_channels,
                          AudioFrame* audio_frame_for_mixing) {
   RTC_DCHECK(number_of_channels == 1 || number_of_channels == 2);
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  std::map<int, MixerAudioSource*> mixedAudioSourcesMap;
+  Frequency mixing_frequency;
+
+  switch (sample_rate) {

[ivoc, 2016/08/31 09:22:45]

It's not really related to the threading/locking issue in this CL, but I  find
this switch statement a bit silly. I think it would be much simpler to write it
like this:

if (sample_rate != kNbInHz && sample_rate != kWbInHz && ...) {
  WEBRTC_TRACE(...);
  RTC_NOTREACHED();
  return;
}
mixing_frequency = sample_rate;

[aleloi, 2016/08/31 11:03:10]

This is indeed simpler! Thanks!

+    case 8000:
+      mixing_frequency = kNbInHz;
+      break;
+    case 16000:
+      mixing_frequency = kWbInHz;
+      break;
+    case 32000:
+      mixing_frequency = kSwbInHz;
+      break;
+    case 48000:
+      mixing_frequency = kFbInHz;
+      break;
+    default:
+      WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
+                   "Invalid frequency: %d", sample_rate);
+      RTC_NOTREACHED();
+      return;
+  }
+
+  if (OutputFrequency() != mixing_frequency) {
+    SetOutputFrequency(mixing_frequency);
+  }
+
   AudioFrameList mixList;
   AudioFrameList additionalFramesList;
-  std::map<int, MixerAudioSource*> mixedAudioSourcesMap;
   {
-    CriticalSectionScoped cs(cb_crit_.get());
-    Frequency mixing_frequency;
-
-    switch (sample_rate) {
-      case 8000:
-        mixing_frequency = kNbInHz;
-        break;
-      case 16000:
-        mixing_frequency = kWbInHz;
-        break;
-      case 32000:
-        mixing_frequency = kSwbInHz;
-        break;
-      case 48000:
-        mixing_frequency = kFbInHz;
-        break;
-      default:
-        RTC_NOTREACHED();
-        return;
-    }
-
-    if (OutputFrequency() != mixing_frequency) {
-      SetOutputFrequency(mixing_frequency);
-    }
-
+    CriticalSectionScoped cs(crit_.get());
     mixList = UpdateToMix(kMaximumAmountOfMixedAudioSources);
     GetAdditionalAudio(&additionalFramesList);
   }
 
   for (FrameAndMuteInfo& frame_and_mute : mixList) {
     RemixFrame(frame_and_mute.frame, number_of_channels);
   }
   for (FrameAndMuteInfo& frame_and_mute : additionalFramesList) {
     RemixFrame(frame_and_mute.frame, number_of_channels);
   }
 
   audio_frame_for_mixing->UpdateFrame(
       -1, time_stamp_, NULL, 0, output_frequency_, AudioFrame::kNormalSpeech,
       AudioFrame::kVadPassive, number_of_channels);
 
   time_stamp_ += static_cast<uint32_t>(sample_size_);
 
   use_limiter_ = num_mixed_audio_sources_ > 1;
 
   // We only use the limiter if it supports the output sample rate and
   // we're actually mixing multiple streams.
   MixFromList(audio_frame_for_mixing, mixList, id_, use_limiter_);
 
-  {
-    CriticalSectionScoped cs(crit_.get());
-    MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList);

[kwiberg-webrtc, 2016/08/31 09:10:36]

Where did this line go?

[aleloi, 2016/08/31 11:03:10]

It should still be here, but be removed in the next dependent CL. The tests
didn't catch it. I've put the line back now, and added a test.

-
-    if (audio_frame_for_mixing->samples_per_channel_ == 0) {
-      // Nothing was mixed, set the audio samples to silence.
-      audio_frame_for_mixing->samples_per_channel_ = sample_size_;
-      audio_frame_for_mixing->Mute();
-    } else {
-      // Only call the limiter if we have something to mix.
-      LimitMixedAudio(audio_frame_for_mixing);
-    }
+  if (audio_frame_for_mixing->samples_per_channel_ == 0) {
+    // Nothing was mixed, set the audio samples to silence.
+    audio_frame_for_mixing->samples_per_channel_ = sample_size_;
+    audio_frame_for_mixing->Mute();
+  } else {
+    // Only call the limiter if we have something to mix.
+    LimitMixedAudio(audio_frame_for_mixing);
   }
 
   // Pass the final result to the level indicator.
   audio_level_.ComputeLevel(*audio_frame_for_mixing);
 
   return;
 }
 
 int32_t AudioMixerImpl::SetOutputFrequency(const Frequency& frequency) {
-  CriticalSectionScoped cs(crit_.get());
-
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   output_frequency_ = frequency;
   sample_size_ =
       static_cast<size_t>((output_frequency_ * kFrameDurationInMs) / 1000);
 
   return 0;
 }
 
 AudioMixer::Frequency AudioMixerImpl::OutputFrequency() const {
-  CriticalSectionScoped cs(crit_.get());
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return output_frequency_;
 }
 
 int32_t AudioMixerImpl::SetMixabilityStatus(MixerAudioSource* audio_source,
                                             bool mixable) {
   if (!mixable) {
     // Anonymous audio sources are in a separate list. Make sure that the
     // audio source is in the _audioSourceList if it is being mixed.
     SetAnonymousMixabilityStatus(audio_source, false);
   }
-  size_t numMixedAudioSources;
   {
-    CriticalSectionScoped cs(cb_crit_.get());
+    CriticalSectionScoped cs(crit_.get());
     const bool isMixed = IsAudioSourceInList(*audio_source, audio_source_list_);
     // API must be called with a new state.
     if (!(mixable ^ isMixed)) {
       WEBRTC_TRACE(kTraceWarning, kTraceAudioMixerServer, id_,
                    "Mixable is aready %s", isMixed ? "ON" : "off");
       return -1;
     }
     bool success = false;
     if (mixable) {
       success = AddAudioSourceToList(audio_source, &audio_source_list_);
     } else {
       success = RemoveAudioSourceFromList(audio_source, &audio_source_list_);
     }
     if (!success) {
       WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
                    "failed to %s audio_source", mixable ? "add" : "remove");
       RTC_NOTREACHED();
       return -1;
     }
 
     size_t numMixedNonAnonymous = audio_source_list_.size();
     if (numMixedNonAnonymous > kMaximumAmountOfMixedAudioSources) {
       numMixedNonAnonymous = kMaximumAmountOfMixedAudioSources;
     }
-    numMixedAudioSources =
+    num_mixed_audio_sources_ =
         numMixedNonAnonymous + additional_audio_source_list_.size();
   }
-  // A MixerAudioSource was added or removed. Make sure the scratch
-  // buffer is updated if necessary.
-  // Note: The scratch buffer may only be updated in Process().
-  CriticalSectionScoped cs(crit_.get());
-  num_mixed_audio_sources_ = numMixedAudioSources;
   return 0;
 }
 
 bool AudioMixerImpl::MixabilityStatus(
     const MixerAudioSource& audio_source) const {
-  CriticalSectionScoped cs(cb_crit_.get());
+  CriticalSectionScoped cs(crit_.get());
   return IsAudioSourceInList(audio_source, audio_source_list_);
 }
 
 int32_t AudioMixerImpl::SetAnonymousMixabilityStatus(
     MixerAudioSource* audio_source,
     bool anonymous) {
-  CriticalSectionScoped cs(cb_crit_.get());
+  CriticalSectionScoped cs(crit_.get());
   if (IsAudioSourceInList(*audio_source, additional_audio_source_list_)) {
     if (anonymous) {
       return 0;
     }
     if (!RemoveAudioSourceFromList(audio_source,
                                    &additional_audio_source_list_)) {
       WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
                    "unable to remove audio_source from anonymous list");
       RTC_NOTREACHED();
       return -1;
@@ skipping 13 matching lines @@
     // already registered.
     return -1;
   }
   return AddAudioSourceToList(audio_source, &additional_audio_source_list_)
              ? 0
              : -1;
 }
 
 bool AudioMixerImpl::AnonymousMixabilityStatus(
     const MixerAudioSource& audio_source) const {
-  CriticalSectionScoped cs(cb_crit_.get());
+  CriticalSectionScoped cs(crit_.get());
   return IsAudioSourceInList(audio_source, additional_audio_source_list_);
 }
 
-AudioFrameList AudioMixerImpl::UpdateToMix(size_t maxAudioFrameCounter) const {
+AudioFrameList AudioMixerImpl::UpdateToMix(size_t maxAudioFrameCounter) const
+    EXCLUSIVE_LOCKS_REQUIRED(crit_) {

[kwiberg-webrtc, 2016/08/31 09:10:36]

IIRC we always put these annotations in the .h file. That way, all lock
annotations (both method and member variable) are in the same place. Also, it
makes sense since the required locks are part of the interface when you call the
method.

Actually, now that I think of it it can't be safe to have the annotation only in
the .cc file. If someone calls this method from another compilation unit, the
compiler won't know that the caller needs to hold the lock. I'm guessing this is
a private method, so we won't hit that problem in this case, but it's another
reason for just doing the simple thing and put all the annotations in the
header.

Ooooh. Even if it's private, it can still be problematic. Consider if another
compilation unit calls a public inline method of your class, which in turn calls
this private method.

[aleloi, 2016/08/31 11:03:10]

This makes sense! Thanks! I've moved the annotation to the header.

   AudioFrameList result;
   std::vector<SourceFrame> audioSourceMixingDataList;
 
   // Get audio source audio and put it in the struct vector.
   for (MixerAudioSource* audio_source : audio_source_list_) {
     auto audio_frame_with_info = audio_source->GetAudioFrameWithMuted(
         id_, static_cast<int>(output_frequency_));
 
     auto audio_frame_info = audio_frame_with_info.audio_frame_info;
     AudioFrame* audio_source_audio_frame = audio_frame_with_info.audio_frame;
@@ skipping 36 matching lines @@
     if (p.was_mixed_before_ && !is_mixed) {
       NewMixerRampOut(p.audio_frame_);
       result.emplace_back(p.audio_frame_, false);
     }
 
     p.audio_source_->_mixHistory->SetIsMixed(is_mixed);
   }
   return result;
 }
 
-void AudioMixerImpl::GetAdditionalAudio(
-    AudioFrameList* additionalFramesList) const {
+void AudioMixerImpl::GetAdditionalAudio(AudioFrameList* additionalFramesList)
+    const EXCLUSIVE_LOCKS_REQUIRED(crit_) {

[kwiberg-webrtc, 2016/08/31 09:10:36]

Move to .h file.

[aleloi, 2016/08/31 11:03:10]

Done.

   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "GetAdditionalAudio(additionalFramesList)");
   // The GetAudioFrameWithMuted() callback may result in the audio source being
   // removed from additionalAudioFramesList_. If that happens it will
   // invalidate any iterators. Create a copy of the audio sources list such
   // that the list of participants can be traversed safely.
   MixerAudioSourceList additionalAudioSourceList;
   additionalAudioSourceList.insert(additionalAudioSourceList.begin(),
                                    additional_audio_source_list_.begin(),
                                    additional_audio_source_list_.end());
@@ skipping 145 matching lines @@
   return level;
 }
 
 int AudioMixerImpl::GetOutputAudioLevelFullRange() {
   const int level = audio_level_.LevelFullRange();
   WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_,
                "GetAudioOutputLevelFullRange() => level=%d", level);
   return level;
 }
 }  // namespace webrtc