Cleanup of the mixer interface.

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.
  */
 
@@ skipping 119 matching lines @@
     AudioMixerImpl::Source const* audio_source,
     AudioMixerImpl::SourceStatusList* audio_source_list) {
   return std::find_if(audio_source_list->begin(), audio_source_list->end(),
                       [audio_source](const AudioMixerImpl::SourceStatus& p) {
                         return p.audio_source == audio_source;
                       });
 }
 
 }  // namespace
 
-std::unique_ptr<AudioMixer> AudioMixer::Create() {
-  return AudioMixerImpl::Create();
-}
-
 AudioMixerImpl::AudioMixerImpl(std::unique_ptr<AudioProcessing> limiter)
     : audio_source_list_(),
-      num_mixed_audio_sources_(0),
       use_limiter_(true),
       time_stamp_(0),
       limiter_(std::move(limiter)) {
   SetOutputFrequency(kDefaultFrequency);
   thread_checker_.DetachFromThread();
 }
 
 AudioMixerImpl::~AudioMixerImpl() {}
 
-std::unique_ptr<AudioMixerImpl> AudioMixerImpl::Create() {
+rtc::scoped_refptr<AudioMixerImpl> AudioMixerImpl::Create() {
   Config config;
   config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
   std::unique_ptr<AudioProcessing> limiter(AudioProcessing::Create(config));
   if (!limiter.get()) {
     return nullptr;
   }
 
   if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) !=
       limiter->kNoError) {
     return nullptr;
@@ skipping 12 matching lines @@
   }
 
   if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) {
     return nullptr;
   }
 
   if (limiter->gain_control()->Enable(true) != limiter->kNoError) {
     return nullptr;
   }
 
-  return std::unique_ptr<AudioMixerImpl>(
-      new AudioMixerImpl(std::move(limiter)));
+  return rtc::scoped_refptr<AudioMixerImpl>(
+      new rtc::RefCountedObject<AudioMixerImpl>(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_RUN_ON(&thread_checker_);
 
 
   if (OutputFrequency() != sample_rate) {
     SetOutputFrequency(sample_rate);
   }
 
   AudioFrameList mix_list;
-  size_t num_mixed_audio_sources;
   {
     rtc::CritScope lock(&crit_);
-    mix_list = GetNonAnonymousAudio();
-    num_mixed_audio_sources = num_mixed_audio_sources_;
+    mix_list = GetAudioFromSources();
   }
 
   for (const auto& frame : mix_list) {
     RemixFrame(frame, number_of_channels);
   }
 
   audio_frame_for_mixing->UpdateFrame(
       -1, time_stamp_, NULL, 0, OutputFrequency(), AudioFrame::kNormalSpeech,
       AudioFrame::kVadPassive, number_of_channels);
 
   time_stamp_ += static_cast<uint32_t>(sample_size_);
 
-  use_limiter_ = num_mixed_audio_sources > 1;
+  use_limiter_ = mix_list.size() > 1;

[aleloi, 2016/10/10 13:06:31]

This change slightly alters the mixing behavior. If a frame is muted, it
contributed to num_mixed_audio_sources, but not to mix_list.size(). The change
is safe, because this is how the old mixer works: exactly
num_mixed_audio_sources are always mixed.

[the sun, 2016/10/10 13:35:49]

Acknowledged.

 
   // We only use the limiter if we're actually mixing multiple streams.
   MixFromList(audio_frame_for_mixing, mix_list, use_limiter_);
 
   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);
   }
 
   return;
 }
 
 void AudioMixerImpl::SetOutputFrequency(int frequency) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   output_frequency_ = frequency;
   sample_size_ = (output_frequency_ * kFrameDurationInMs) / 1000;
 }
 
 int AudioMixerImpl::OutputFrequency() const {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   return output_frequency_;
 }
 
-int32_t AudioMixerImpl::SetMixabilityStatus(Source* audio_source,
-                                            bool mixable) {
-  {
-    rtc::CritScope lock(&crit_);
-    const bool is_mixed = FindSourceInList(audio_source, &audio_source_list_) !=
-                          audio_source_list_.end();
-    // API must be called with a new state.
-    if (!(mixable ^ is_mixed)) {
-      return -1;
-    }
-    bool success = false;
-    if (mixable) {
-      success = AddAudioSourceToList(audio_source, &audio_source_list_);
-    } else {
-      success = RemoveAudioSourceFromList(audio_source, &audio_source_list_);
-    }
-    if (!success) {
-      RTC_NOTREACHED();
-      return -1;
-    }
-
-    size_t num_mixed_non_anonymous = audio_source_list_.size();
-    if (num_mixed_non_anonymous > kMaximumAmountOfMixedAudioSources) {
-      num_mixed_non_anonymous = kMaximumAmountOfMixedAudioSources;
-    }
-    num_mixed_audio_sources_ = num_mixed_non_anonymous;
+bool AudioMixerImpl::AddSource(Source* audio_source) {

[the sun, 2016/10/10 13:35:49]

RTC_DCHECK(audio_source);

+  rtc::CritScope lock(&crit_);
+  const bool is_mixed = FindSourceInList(audio_source, &audio_source_list_) !=
+                        audio_source_list_.end();
+  if (is_mixed) {
+    LOG(LS_WARNING) << "Source already added to mixer";
+    return false;

[the sun, 2016/10/10 13:35:49]

I think you should just DCHECK(!is_mixed) and always return true from this
method:

RTC_CHECK_EQ(FindSourceInList(audio_source, &audio_source_list_),
audio_source_list_.end());

Because this is a logic error. The client code should never call the interface
like that. Note that since we're making this an api/ interface, we will still
need the return code though, as there may be other reasons the method may fail,
for other implementations.

[aleloi, 2016/10/10 14:02:27]

I agree: it's better to see this as an error instead of having clients silently
ignore a returned 'false'.

   }
-  return 0;
+  audio_source_list_.emplace_back(audio_source, false, 0);
+  return true;
 }
 
+bool AudioMixerImpl::RemoveSource(Source* audio_source) {

[the sun, 2016/10/10 13:35:49]

Same comments apply as in AddSource()

[aleloi, 2016/10/10 14:02:27]

Done.

+  rtc::CritScope lock(&crit_);
+  const bool is_mixed = FindSourceInList(audio_source, &audio_source_list_) !=
+                        audio_source_list_.end();
+  if (!is_mixed) {
+    return false;
+  }
+  const auto iter = FindSourceInList(audio_source, &audio_source_list_);
+  if (iter == audio_source_list_.end()) {
+    LOG_T_F(LS_WARNING) << "Source not present in mixer";
+    return false;
+  }
+  audio_source_list_.erase(iter);
+  return true;
+}
 
-
-AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() {
+AudioFrameList AudioMixerImpl::GetAudioFromSources() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   AudioFrameList result;
   std::vector<SourceFrame> audio_source_mixing_data_list;
   std::vector<SourceFrame> ramp_list;
 
   // Get audio source audio and put it in the struct vector.
   for (auto& source_and_status : audio_source_list_) {
     auto audio_frame_with_info =
         source_and_status.audio_source->GetAudioFrameWithMuted(
             static_cast<int>(OutputFrequency()));
@@ skipping 31 matching lines @@
       result.push_back(p.audio_frame);
       ramp_list.emplace_back(p.source_status, p.audio_frame, false, -1);
       is_mixed = true;
     }
     p.source_status->is_mixed = is_mixed;
   }
   RampAndUpdateGain(ramp_list);
   return result;
 }
 
-bool AudioMixerImpl::AddAudioSourceToList(
-    Source* audio_source,
-    SourceStatusList* audio_source_list) const {
-  audio_source_list->emplace_back(audio_source, false, 0);
-  return true;
-}
-
-bool AudioMixerImpl::RemoveAudioSourceFromList(
-    Source* audio_source,
-    SourceStatusList* audio_source_list) const {
-  const auto iter = FindSourceInList(audio_source, audio_source_list);
-  if (iter != audio_source_list->end()) {
-    audio_source_list->erase(iter);
-    return true;
-  } else {
-    return false;
-  }
-}
 
 bool AudioMixerImpl::LimitMixedAudio(AudioFrame* mixed_audio) const {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   if (!use_limiter_) {
     return true;
   }
 
   // Smoothly limit the mixed frame.
   const int error = limiter_->ProcessStream(mixed_audio);
 
@@ skipping 25 matching lines @@
   const auto non_anonymous_iter =
       FindSourceInList(audio_source, &audio_source_list_);
   if (non_anonymous_iter != audio_source_list_.end()) {
     return non_anonymous_iter->is_mixed;
   }
 
   LOG(LS_ERROR) << "Audio source unknown";
   return false;
 }
 }  // namespace webrtc