Made MixerAudioSource a pure interface.

webrtc/modules/audio_mixer/audio_mixer_impl.cc

Index: webrtc/modules/audio_mixer/audio_mixer_impl.cc
diff --git a/webrtc/modules/audio_mixer/audio_mixer_impl.cc b/webrtc/modules/audio_mixer/audio_mixer_impl.cc
index 44e04fa32d4aa6de25d5297b0cdc74154f1368ca..03b8bb13375b1ff5b38c7b6ddf8ab83e652530a3 100644
--- a/webrtc/modules/audio_mixer/audio_mixer_impl.cc
+++ b/webrtc/modules/audio_mixer/audio_mixer_impl.cc
@@ -14,6 +14,7 @@
 #include <functional>
 #include <utility>
 
+#include "webrtc/base/logging.h"
 #include "webrtc/modules/audio_mixer/audio_frame_manipulator.h"
 #include "webrtc/modules/utility/include/audio_frame_operations.h"
 #include "webrtc/system_wrappers/include/trace.h"
@@ -23,26 +24,18 @@ namespace {
 
 class SourceFrame {
  public:
-  SourceFrame(MixerAudioSource* p, AudioFrame* a, bool m, bool was_mixed_before)
-      : audio_source_(p),
-        audio_frame_(a),
-        muted_(m),
-        was_mixed_before_(was_mixed_before) {
+  SourceFrame(AudioSourceWithMixStatus* p, AudioFrame* a, bool m)
+      : audio_source_(p), audio_frame_(a), muted_(m) {
     if (!muted_) {
       energy_ = NewMixerCalculateEnergy(*a);
     }
   }
 
-  SourceFrame(MixerAudioSource* p,
+  SourceFrame(AudioSourceWithMixStatus* p,
               AudioFrame* a,
               bool m,
-              bool was_mixed_before,
               uint32_t energy)
-      : audio_source_(p),
-        audio_frame_(a),
-        muted_(m),
-        energy_(energy),
-        was_mixed_before_(was_mixed_before) {}
+      : audio_source_(p), audio_frame_(a), muted_(m), energy_(energy) {}
 
   // a.shouldMixBefore(b) is used to select mixer participants.
   bool shouldMixBefore(const SourceFrame& other) const {
@@ -60,11 +53,10 @@ class SourceFrame {
     return energy_ > other.energy_;
   }
 
-  MixerAudioSource* audio_source_;
+  AudioSourceWithMixStatus* audio_source_;
   AudioFrame* audio_frame_;
   bool muted_;
   uint32_t energy_;
-  bool was_mixed_before_;
 };
 
 // Remixes a frame between stereo and mono.
@@ -80,13 +72,13 @@ void RemixFrame(AudioFrame* frame, size_t number_of_channels) {
 void Ramp(const std::vector<SourceFrame>& mixed_sources_and_frames) {
   for (const auto& source_frame : mixed_sources_and_frames) {
     // Ramp in previously unmixed.
-    if (!source_frame.was_mixed_before_) {
+    if (!source_frame.audio_source_->WasMixed()) {
       NewMixerRampIn(source_frame.audio_frame_);
     }
 
     const bool is_mixed = source_frame.audio_source_->IsMixed();
     // Ramp out currently unmixed.
-    if (source_frame.was_mixed_before_ && !is_mixed) {
+    if (source_frame.audio_source_->WasMixed() && !is_mixed) {
       NewMixerRampOut(source_frame.audio_frame_);
     }
   }
@@ -153,7 +145,7 @@ AudioMixerImpl::AudioMixerImpl(int id, std::unique_ptr<AudioProcessing> limiter)
 
 AudioMixerImpl::~AudioMixerImpl() {}
 
-std::unique_ptr<AudioMixer> AudioMixerImpl::Create(int id) {
+std::unique_ptr<AudioMixerImpl> AudioMixerImpl::Create(int id) {
   Config config;
   config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
   std::unique_ptr<AudioProcessing> limiter(AudioProcessing::Create(config));
@@ -179,7 +171,7 @@ std::unique_ptr<AudioMixer> AudioMixerImpl::Create(int id) {
   if (limiter->gain_control()->Enable(true) != limiter->kNoError)
     return nullptr;
 
-  return std::unique_ptr<AudioMixer>(
+  return std::unique_ptr<AudioMixerImpl>(
       new AudioMixerImpl(id, std::move(limiter)));
 }
 
@@ -344,7 +336,7 @@ bool AudioMixerImpl::AnonymousMixabilityStatus(
   return IsAudioSourceInList(audio_source, additional_audio_source_list_);
 }
 
-AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
+AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "GetNonAnonymousAudio()");
@@ -353,9 +345,10 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
   std::vector<SourceFrame> ramp_list;
 
   // Get audio source audio and put it in the struct vector.
-  for (auto* const audio_source : audio_source_list_) {
-    auto audio_frame_with_info = audio_source->GetAudioFrameWithMuted(
-        id_, static_cast<int>(OutputFrequency()));
+  for (auto& source_and_status : audio_source_list_) {
+    auto audio_frame_with_info =
+        source_and_status.audio_source()->GetAudioFrameWithMuted(
+            id_, static_cast<int>(OutputFrequency()));
 
     const auto audio_frame_info = audio_frame_with_info.audio_frame_info;
     AudioFrame* audio_source_audio_frame = audio_frame_with_info.audio_frame;
@@ -366,9 +359,8 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
       continue;
     }
     audio_source_mixing_data_list.emplace_back(
-        audio_source, audio_source_audio_frame,
-        audio_frame_info == MixerAudioSource::AudioFrameInfo::kMuted,
-        audio_source->WasMixed());
+        &source_and_status, audio_source_audio_frame,
+        audio_frame_info == MixerAudioSource::AudioFrameInfo::kMuted);
   }
 
   // Sort frames by sorting function.
@@ -379,7 +371,7 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
   int max_audio_frame_counter = kMaximumAmountOfMixedAudioSources;
 
   // Go through list in order and put unmuted frames in result list.
-  for (const SourceFrame& p : audio_source_mixing_data_list) {
+  for (const auto& p : audio_source_mixing_data_list) {
     // Filter muted.
     if (p.muted_) {
       p.audio_source_->SetIsMixed(false);
@@ -391,8 +383,7 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
     if (max_audio_frame_counter > 0) {
       --max_audio_frame_counter;
       result.push_back(p.audio_frame_);
-      ramp_list.emplace_back(p.audio_source_, p.audio_frame_, false,
-                             p.was_mixed_before_, -1);
+      ramp_list.emplace_back(p.audio_source_, p.audio_frame_, false, -1);
       is_mixed = true;
     }
     p.audio_source_->SetIsMixed(is_mixed);
@@ -401,24 +392,16 @@ AudioFrameList AudioMixerImpl::GetNonAnonymousAudio() const {
   return result;
 }
 
-AudioFrameList AudioMixerImpl::GetAnonymousAudio() const {
+AudioFrameList AudioMixerImpl::GetAnonymousAudio() {

[the sun, 2016/10/04 20:36:09]

Why do we need to treat "anonymous" audio differently?

As I see it, "always included in mix" is an attribute of the source (or at least
the mixer's internal representation of a source - meta data) and this attribute
can be taken into account when selecting the set of streams to mix. Keeping a
separate "additional_audio_source_list_" seems like making things more
complicated than they need to be.

[aleloi, 2016/10/05 15:18:19]

Good point! The latest plan seems to be to remove anonymous audio altogether. I
think that's better to do in another CL.

   RTC_DCHECK_RUN_ON(&thread_checker_);
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "GetAnonymousAudio()");
-  // 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.
   std::vector<SourceFrame> ramp_list;
-  MixerAudioSourceList additional_audio_sources_list;
   AudioFrameList result;
-  additional_audio_sources_list.insert(additional_audio_sources_list.begin(),
-                                       additional_audio_source_list_.begin(),
-                                       additional_audio_source_list_.end());
-
-  for (const auto& audio_source : additional_audio_sources_list) {
+  for (auto& source_and_status : additional_audio_source_list_) {
     const auto audio_frame_with_info =
-        audio_source->GetAudioFrameWithMuted(id_, OutputFrequency());
+        source_and_status.audio_source()->GetAudioFrameWithMuted(
+            id_, OutputFrequency());
     const auto ret = audio_frame_with_info.audio_frame_info;
     AudioFrame* audio_frame = audio_frame_with_info.audio_frame;
     if (ret == MixerAudioSource::AudioFrameInfo::kError) {
@@ -428,9 +411,8 @@ AudioFrameList AudioMixerImpl::GetAnonymousAudio() const {
     }
     if (ret != MixerAudioSource::AudioFrameInfo::kMuted) {
       result.push_back(audio_frame);
-      ramp_list.emplace_back(audio_source, audio_frame, false,
-                             audio_source->IsMixed(), 0);
-      audio_source->SetIsMixed(true);
+      ramp_list.emplace_back(&source_and_status, audio_frame, false, 0);
+      source_and_status.SetIsMixed(true);
     }
   }
   Ramp(ramp_list);
@@ -442,8 +424,10 @@ bool AudioMixerImpl::IsAudioSourceInList(
     const MixerAudioSourceList& audio_source_list) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "IsAudioSourceInList(audio_source,audio_source_list)");
-  return std::find(audio_source_list.begin(), audio_source_list.end(),
-                   &audio_source) != audio_source_list.end();
+  return std::find_if(audio_source_list.begin(), audio_source_list.end(),

[ivoc, 2016/10/04 20:39:29]

Maybe it's not that important, but I think a for loop would be less code and a
bit easier to understand as well.

[aleloi, 2016/10/05 15:18:18]

Probably. The loop contains about as many characters. Lambdas in C++ add lots of
syntactical overhead...

+                      [&audio_source](const AudioSourceWithMixStatus& p) {
+                        return p.audio_source() == &audio_source;
+                      }) != audio_source_list.end();
 }
 
 bool AudioMixerImpl::AddAudioSourceToList(
@@ -451,9 +435,7 @@ bool AudioMixerImpl::AddAudioSourceToList(
     MixerAudioSourceList* audio_source_list) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "AddAudioSourceToList(audio_source, audio_source_list)");
-  audio_source_list->push_back(audio_source);
-  // Make sure that the mixed status is correct for new MixerAudioSource.
-  audio_source->ResetMixedStatus();
+  audio_source_list->emplace_back(audio_source);
   return true;
 }
 
@@ -462,12 +444,13 @@ bool AudioMixerImpl::RemoveAudioSourceFromList(
     MixerAudioSourceList* audio_source_list) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id_,
                "RemoveAudioSourceFromList(audio_source, audio_source_list)");
-  const auto iter = std::find(audio_source_list->begin(),
-                              audio_source_list->end(), audio_source);
+  const auto iter =
+      std::find_if(audio_source_list->begin(), audio_source_list->end(),
+                   [audio_source](const AudioSourceWithMixStatus& p) {
+                     return p.audio_source() == audio_source;
+                   });
   if (iter != audio_source_list->end()) {
     audio_source_list->erase(iter);
-    // AudioSource is no longer mixed, reset to default.
-    audio_source->ResetMixedStatus();
     return true;
   } else {
     return false;
@@ -519,4 +502,42 @@ int AudioMixerImpl::GetOutputAudioLevelFullRange() {
                "GetAudioOutputLevelFullRange() => level=%d", level);
   return level;
 }
+
+AudioSourceWithMixStatus* AudioMixerImpl::GetSourceWithStatus(

[the sun, 2016/10/04 20:36:09]

If this is only used for unit testing, can you mark it as such?

Just a sanity check - the unit test is not running multiple threads, is it?

[aleloi, 2016/10/05 15:18:19]

I don't understand, how should I mark it? 

The unit tests do not run multiple threads.

[the sun, 2016/10/05 19:32:10]

Well, you could either add a comment in the .h, or name it ...ForTest(), for
example.
Good! The locking had me wondering...

[aleloi, 2016/10/06 09:26:12]

I looked around in the webrtc source. ..ForTest/ForTests is used in lots of
other places, so I added that.

+    MixerAudioSource* audio_source) {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+  rtc::CritScope lock(&crit_);
+  auto iter = std::find_if(audio_source_list_.begin(), audio_source_list_.end(),

[ivoc, 2016/10/04 20:39:29]

Again, I think this code would look simpler (and possibly shorter?) with some
old-fashioned for loops.

[aleloi, 2016/10/05 15:18:18]

I tried and it was indeed shorter :)

+                           [audio_source](const AudioSourceWithMixStatus& p) {
+                             return p.audio_source() == audio_source;
+                           });
+  if (iter != audio_source_list_.end()) {
+    return &(*iter);
+  }
+
+  iter = std::find_if(additional_audio_source_list_.begin(),
+                      additional_audio_source_list_.end(),
+                      [audio_source](const AudioSourceWithMixStatus& p) {
+                        return p.audio_source() == audio_source;
+                      });
+  if (iter != additional_audio_source_list_.end()) {
+    return &(*iter);
+  } else {
+    LOG_T_F(LS_ERROR) << "Audio source unknown";
+    return nullptr;
+  }
+}
+
+bool AudioMixerImpl::GetAudioSourceMixabilityStatus(
+    MixerAudioSource* audio_source) {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+  rtc::CritScope lock(&crit_);
+  const auto* const ptr = GetSourceWithStatus(audio_source);
+  if (ptr) {
+    return ptr->IsMixed();
+  } else {
+    LOG_T_F(LS_ERROR) << "Audio source unknown";
+    return false;
+  }
+}
 }  // namespace webrtc