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 <utility>
 
+#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 87 matching lines @@
 int32_t NewMixHistory::SetIsMixed(const bool mixed) {
   is_mixed_ = mixed;
   return 0;
 }
 
 void NewMixHistory::ResetMixedStatus() {
   is_mixed_ = false;
 }
 
 std::unique_ptr<AudioMixer> AudioMixer::Create(int id) {
-  AudioMixerImpl* mixer = new AudioMixerImpl(id);
-  if (!mixer->Init()) {
-    delete mixer;
-    return NULL;
-  }
-  return std::unique_ptr<AudioMixer>(mixer);
+  return AudioMixerImpl::Create(id);
 }
 
-AudioMixerImpl::AudioMixerImpl(int id)
-    : id_(id),
-      output_frequency_(kDefaultFrequency),
-      sample_size_(0),
+AudioMixerImpl::AudioMixerImpl(int id, std::unique_ptr<AudioProcessing> limiter)
+    : crit_(new CriticalSectionWrapper()),

[aleloi, 2016/09/02 08:51:21]

Comments in critical_section_wrapper.h request to use the crit sec constructor.

[kwiberg-webrtc, 2016/09/02 09:00:26]

Are you referring to this comment?

  // Legacy factory method, being deprecated. Please use the constructor.
  // TODO(tommi): Remove the CriticalSectionWrapper class and move users over
  // to using rtc::CriticalSection.

I think the preferred thing to do is to use rtc::CriticalSection.

[aleloi, 2016/09/02 09:27:28]

I did a 'git grep' for CriticalSection. The constructor is not used anywhere at
all and everyone still use the Wrapper. Do you think it's safe to use it? Maybe
we should ask tommi@?

[kwiberg-webrtc, 2016/09/02 09:44:00]

I'm guessing that although no one explicitly calls the constructor, it's still
being called. My grep finds a bunch of classes with rtc::CriticalSection
members, and a class's constructor will always call the constructors of all
member variables one way or another.

+      id_(id),
       audio_source_list_(),
       additional_audio_source_list_(),
       num_mixed_audio_sources_(0),
       use_limiter_(true),
-      time_stamp_(0) {
+      time_stamp_(0),
+      limiter_(std::move(limiter)) {
+  SetOutputFrequency(kDefaultFrequency);
   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;
-
+std::unique_ptr<AudioMixer> AudioMixerImpl::Create(int id) {
   Config config;
   config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
-  limiter_.reset(AudioProcessing::Create(config));
-  if (!limiter_.get())
-    return false;
+  std::unique_ptr<AudioProcessing> limiter(AudioProcessing::Create(config));
+  if (!limiter.get())
+    return nullptr;
 
-  if (SetOutputFrequency(kDefaultFrequency) == -1)
-    return false;
-
-  if (limiter_->gain_control()->set_mode(GainControl::kFixedDigital) !=
-      limiter_->kNoError)
-    return false;
+  if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) !=
+      limiter->kNoError)
+    return nullptr;
 
   // We smoothly limit the mixed frame to -7 dbFS. -6 would correspond to the
   // divide-by-2 but -7 is used instead to give a bit of headroom since the
   // AGC is not a hard limiter.
-  if (limiter_->gain_control()->set_target_level_dbfs(7) != limiter_->kNoError)
-    return false;
+  if (limiter->gain_control()->set_target_level_dbfs(7) != limiter->kNoError)
+    return nullptr;
 
-  if (limiter_->gain_control()->set_compression_gain_db(0) !=
-      limiter_->kNoError)
-    return false;
+  if (limiter->gain_control()->set_compression_gain_db(0) != limiter->kNoError)
+    return nullptr;
 
-  if (limiter_->gain_control()->enable_limiter(true) != limiter_->kNoError)
-    return false;
+  if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError)
+    return nullptr;
 
-  if (limiter_->gain_control()->Enable(true) != limiter_->kNoError)
-    return false;
+  if (limiter->gain_control()->Enable(true) != limiter->kNoError)
+    return nullptr;
 
-  return true;
+  return std::unique_ptr<AudioMixer>(
+      new AudioMixerImpl(id, std::move(limiter)));
 }
 
 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());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+  std::map<int, MixerAudioSource*> mixedAudioSourcesMap;
+
+  if (sample_rate != kNbInHz && sample_rate != kWbInHz &&
+      sample_rate != kSwbInHz && sample_rate != kFbInHz) {
+    WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
+                 "Invalid frequency: %d", sample_rate);
+    RTC_NOTREACHED();
+    return;
+  }
+
+  if (OutputFrequency() != sample_rate) {
+    SetOutputFrequency(static_cast<Frequency>(sample_rate));
+  }
+
   AudioFrameList mixList;
   AudioFrameList additionalFramesList;
-  std::map<int, MixerAudioSource*> mixedAudioSourcesMap;
+  int num_mixed_audio_sources;
   {
-    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);
+    num_mixed_audio_sources = static_cast<int>(num_mixed_audio_sources_);
   }
 
   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;
+  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);
-
-    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);
-    }
+  MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList);
+  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_RUN_ON(&thread_checker_);
   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_RUN_ON(&thread_checker_);
   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 {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   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 38 matching lines @@
       result.emplace_back(p.audio_frame_, false);
     }
 
     p.audio_source_->_mixHistory->SetIsMixed(is_mixed);
   }
   return result;
 }
 
 void AudioMixerImpl::GetAdditionalAudio(
     AudioFrameList* additionalFramesList) const {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   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 87 matching lines @@
     position++;
   }
 
   return 0;
 }
 
 // TODO(andrew): consolidate this function with MixFromList.
 int32_t AudioMixerImpl::MixAnonomouslyFromList(
     AudioFrame* mixedAudio,
     const AudioFrameList& audioFrameList) const {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "MixAnonomouslyFromList(mixedAudio, audioFrameList)");
 
   if (audioFrameList.empty())
     return 0;
 
   for (AudioFrameList::const_iterator iter = audioFrameList.begin();
        iter != audioFrameList.end(); ++iter) {
     if (!iter->muted) {
       MixFrames(mixedAudio, iter->frame, use_limiter_);
     }
   }
   return 0;
 }
 
 bool AudioMixerImpl::LimitMixedAudio(AudioFrame* mixedAudio) const {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   if (!use_limiter_) {
     return true;
   }
 
   // Smoothly limit the mixed frame.
   const int error = limiter_->ProcessStream(mixedAudio);
 
   // And now we can safely restore the level. This procedure results in
   // some loss of resolution, deemed acceptable.
   //
   // It's possible to apply the gain in the AGC (with a target level of 0 dbFS
   // and compression gain of 6 dB). However, in the transition frame when this
   // is enabled (moving from one to two audio sources) it has the potential to
   // create discontinuities in the mixed frame.
   //
   // Instead we double the frame (with addition since left-shifting a
   // negative value is undefined).
   *mixedAudio += *mixedAudio;
 
   if (error != limiter_->kNoError) {
     WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
                  "Error from AudioProcessing: %d", error);
     RTC_NOTREACHED();
     return false;
   }
   return true;
 }
 
 int AudioMixerImpl::GetOutputAudioLevel() {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   const int level = audio_level_.Level();
   WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_,
                "GetAudioOutputLevel() => level=%d", level);
   return level;
 }
 
 int AudioMixerImpl::GetOutputAudioLevelFullRange() {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   const int level = audio_level_.LevelFullRange();
   WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_,
                "GetAudioOutputLevelFullRange() => level=%d", level);
   return level;
 }
 }  // namespace webrtc