Rewrote UpdateToMix in the audio mixer.

webrtc/modules/audio_mixer/source/new_audio_conference_mixer_impl.cc

 /*
  *  Copyright (c) 2016 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/source/new_audio_conference_mixer_impl.h"
 
 #include <algorithm>
 
 #include "webrtc/modules/audio_conference_mixer/source/audio_frame_manipulator.h"
 #include "webrtc/modules/audio_mixer/include/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 {
 
-struct ParticipantFrameStruct {
-  ParticipantFrameStruct(MixerAudioSource* p, AudioFrame* a, bool m)
-      : participant(p), audioFrame(a), muted(m) {}
-  MixerAudioSource* participant;
-  AudioFrame* audioFrame;
-  bool muted;
+class ParticipantFrameStruct {

[ossu, 2016/07/08 11:02:14]

I don't think we suffix structs with Struct. Probably something from way back.
Should probably be just SourceFrame or something (since MixerParticipant is no
longer a thing).

[aleloi, 2016/07/08 13:45:23]

Done.

+ public:
+  ParticipantFrameStruct(MixerAudioSource* p,
+                         AudioFrame* a,
+                         bool m,
+                         bool was_mixed_before)
+      : participant_(p),
+        audio_frame_(a),
+        muted_(m),
+        was_mixed_before_(was_mixed_before) {
+    if (!muted_) {
+      energy_ = CalculateEnergy(*a);
+    }
+  }
+
+  // a.shouldMixAfter(b) is used to select mixer participants.
+  bool shouldMixAfter(const ParticipantFrameStruct& other) const {
+    if (muted_ != other.muted_) {
+      return muted_ && !other.muted_;

[ossu, 2016/07/08 11:02:14]

wh... what? Returning muted should be enough here.

[aleloi, 2016/07/08 13:45:23]

Done. You are right, it's shorter.

+    }
+
+    auto our_activity = audio_frame_->vad_activity_,
+         other_activity = other.audio_frame_->vad_activity_;

[ossu, 2016/07/08 11:02:14]

I'm not sure if this is in the style guide, but I'd prefer it if these were two
separate statements:
auto our_activity = ...;
auto other_activity = ...;

[aleloi, 2016/07/08 13:45:24]

Done.

+    if (our_activity != other_activity) {
+      return (our_activity == AudioFrame::kVadPassive ||
+              our_activity == AudioFrame::kVadUnknown) &&
+             other_activity == AudioFrame::kVadActive;
+    }
+
+    return energy_ < other.energy_;
+  }
+
+  MixerAudioSource* participant_;

[ossu, 2016/07/08 11:02:13]

participant or source? Pick one! :)

+  AudioFrame* audio_frame_;
+  bool muted_;
+  uint32_t energy_;
+  bool was_mixed_before_;
 };
 
 typedef std::list<ParticipantFrameStruct*> ParticipantFrameStructList;
 
 // Mix |frame| into |mixed_frame|, with saturation protection and upmixing.
 // These effects are applied to |frame| itself prior to mixing. Assumes that
 // |mixed_frame| always has at least as many channels as |frame|. Supports
 // stereo at most.
 //
 // TODO(andrew): consider not modifying |frame| here.
@@ skipping 113 matching lines @@
   if (_limiter->gain_control()->enable_limiter(true) != _limiter->kNoError)
     return false;
 
   if (_limiter->gain_control()->Enable(true) != _limiter->kNoError)
     return false;
 
   return true;
 }
 
 void NewAudioConferenceMixerImpl::Mix(AudioFrame* audio_frame_for_mixing) {
   size_t remainingParticipantsAllowedToMix = kMaximumAmountOfMixedParticipants;

[ossu, 2016/07/08 11:02:13]

Just in general, if we're now using the term MixerAudioSource instead of
MixerParticipant, all uses should be updated to source rather than participant,
imo.

[aleloi, 2016/07/08 13:45:23]

I'll change upstream.

   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 
   AudioFrameList mixList;

[ossu, 2016/07/08 11:02:14]

Move down to where it's actually initialized.

[aleloi, 2016/07/08 13:45:24]

It's assigned to while we're holding the lock. And the lock is a scope lock, so
it's inside its own scope. 

[ossu, 2016/07/08 13:58:34]

Oh, right! Silly me!

   AudioFrameList rampOutList;

[ossu, 2016/07/08 11:02:14]

Not used?

[aleloi, 2016/07/08 13:45:24]

Yes! Thanks for noticing!

   AudioFrameList additionalFramesList;

[ossu, 2016/07/08 11:02:14]

Probably move this one down as well.

[aleloi, 2016/07/08 13:45:24]

Changes are also guarded by lock. 

   std::map<int, MixerAudioSource*> mixedParticipantsMap;
   {
     CriticalSectionScoped cs(_cbCrit.get());
 
     int32_t lowFreq = GetLowestMixingFrequency();
     // SILK can run in 12 kHz and 24 kHz. These frequencies are not
     // supported so use the closest higher frequency to not lose any
     // information.
     // TODO(henrike): this is probably more appropriate to do in
     //                GetLowestMixingFrequency().
     if (lowFreq == 12000) {
       lowFreq = 16000;
     } else if (lowFreq == 24000) {
       lowFreq = 32000;
     }
     if (lowFreq <= 0) {
-      RTC_DCHECK(thread_checker_.CalledOnValidThread());

[ossu, 2016/07/08 11:02:14]

Why is this no longer useful?

       return;
     } else {
       switch (lowFreq) {
         case 8000:
           if (OutputFrequency() != kNbInHz) {
             SetOutputFrequency(kNbInHz);
           }
           break;
         case 16000:
           if (OutputFrequency() != kWbInHz) {
             SetOutputFrequency(kWbInHz);
           }
           break;
         case 32000:
           if (OutputFrequency() != kSwbInHz) {
             SetOutputFrequency(kSwbInHz);
           }
           break;
         case 48000:
           if (OutputFrequency() != kFbInHz) {
             SetOutputFrequency(kFbInHz);
           }
           break;
         default:
           RTC_DCHECK(false);
 
-          RTC_DCHECK(thread_checker_.CalledOnValidThread());
           return;
       }
     }
 
-    UpdateToMix(&mixList, &rampOutList, &mixedParticipantsMap,
-                &remainingParticipantsAllowedToMix);
-
+    mixList = UpdateToMix(remainingParticipantsAllowedToMix);

[ossu, 2016/07/08 11:02:14]

You could just use kMaximumAmountOfMixedParticipants, it seems.

[aleloi, 2016/07/08 13:45:24]

Done.

+    remainingParticipantsAllowedToMix -= mixList.size();

[ossu, 2016/07/08 11:02:14]

Who cares? It's never used again, is it?

[aleloi, 2016/07/08 13:45:24]

Removed. Thanks!

     GetAdditionalAudio(&additionalFramesList);
-    UpdateMixedStatus(mixedParticipantsMap);
   }
 
   // TODO(henrike): it might be better to decide the number of channels
   //                with an API instead of dynamically.
 
   // Find the max channels over all mixing lists.
   const size_t num_mixed_channels = std::max(
       MaxNumChannels(&mixList), std::max(MaxNumChannels(&additionalFramesList),
                                          MaxNumChannels(&rampOutList)));
 
@@ skipping 18 matching lines @@
     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_ = _sampleSize;
       audio_frame_for_mixing->Mute();
     } else {
       // Only call the limiter if we have something to mix.
       LimitMixedAudio(audio_frame_for_mixing);
     }
   }
 
-  ClearAudioFrameList(&mixList);

[ossu, 2016/07/08 11:02:14]

So I guess this used to be necessary to hand the frames back to the pool?

-  ClearAudioFrameList(&rampOutList);
-  ClearAudioFrameList(&additionalFramesList);
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return;
 }
 
 int32_t NewAudioConferenceMixerImpl::SetOutputFrequency(
     const Frequency& frequency) {
   CriticalSectionScoped cs(_crit.get());
 
   _outputFrequency = frequency;
   _sampleSize =
       static_cast<size_t>((_outputFrequency * kProcessPeriodicityInMs) / 1000);
@@ skipping 144 matching lines @@
   for (MixerAudioSourceList::const_iterator iter = mixList.begin();
        iter != mixList.end(); ++iter) {
     const int32_t neededFrequency = (*iter)->NeededFrequency(_id);
     if (neededFrequency > highestFreq) {
       highestFreq = neededFrequency;
     }
   }
   return highestFreq;
 }
 
-void NewAudioConferenceMixerImpl::UpdateToMix(
-    AudioFrameList* mixList,
-    AudioFrameList* rampOutList,
-    std::map<int, MixerAudioSource*>* mixParticipantList,
-    size_t* maxAudioFrameCounter) const {
-  WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
-               "UpdateToMix(mixList,rampOutList,mixParticipantList,%d)",
-               *maxAudioFrameCounter);
-  const size_t mixListStartSize = mixList->size();
-  AudioFrameList activeList;
-  // Struct needed by the passive lists to keep track of which AudioFrame
-  // belongs to which MixerAudioSource.
-  ParticipantFrameStructList passiveWasNotMixedList;
-  ParticipantFrameStructList passiveWasMixedList;
-  for (MixerAudioSourceList::const_iterator participant =
-           _participantList.begin();
-       participant != _participantList.end(); ++participant) {
-    // Stop keeping track of passive participants if there are already
-    // enough participants available (they wont be mixed anyway).
-    bool mustAddToPassiveList =
-        (*maxAudioFrameCounter >
-         (activeList.size() + passiveWasMixedList.size() +
-          passiveWasNotMixedList.size()));
+AudioFrameList NewAudioConferenceMixerImpl::UpdateToMix(
+    size_t maxAudioFrameCounter) const {
+  AudioFrameList result;
+  std::vector<ParticipantFrameStruct> participantMixingDataList;
 
-    bool wasMixed = false;
-    wasMixed = (*participant)->_mixHistory->WasMixed();
+  // Get participant audio and put it in the struct vector.
+  for (MixerAudioSource* participant : _participantList) {
+    auto audio_frame_with_info = participant->GetAudioFrameWithMuted(

[ossu, 2016/07/08 11:02:14]

Hmm... reading this, I'm thinking AudioFrameWithInfo() should be called
AudioFrameWithMuted(), since the method to get one is called
GetAudioFrameWithMuted. Muted is also a better description of that enum than
"info".

[aleloi, 2016/07/08 13:45:24]

I agree. Changed.

+        _id, static_cast<int>(_outputFrequency));
 
-    auto audio_frame_with_info =
-        (*participant)->GetAudioFrameWithMuted(_id, _outputFrequency);
+    auto audio_frame_info = audio_frame_with_info.audio_frame_info;
+    AudioFrame* participant_audio_frame = audio_frame_with_info.audio_frame;
 
-    auto ret = audio_frame_with_info.audio_frame_info;
-    AudioFrame* audio_frame = audio_frame_with_info.audio_frame;
-    if (ret == MixerAudioSource::AudioFrameInfo::kError) {
+    if (audio_frame_info == MixerAudioSource::AudioFrameInfo::kError) {
+      WEBRTC_TRACE(kTraceWarning, kTraceAudioMixerServer, _id,
+                   "failed to GetAudioFrameWithMuted() from participant");
       continue;
     }
-    const bool muted = (ret == MixerAudioSource::AudioFrameInfo::kMuted);
-    if (_participantList.size() != 1) {
-      // TODO(wu): Issue 3390, add support for multiple participants case.
-      audio_frame->ntp_time_ms_ = -1;
+    participantMixingDataList.emplace_back(
+        participant, participant_audio_frame,
+        audio_frame_info == MixerAudioSource::AudioFrameInfo::kMuted,
+        participant->_mixHistory->WasMixed());
+  }
+
+  // Sort frames by sorting function.
+  std::sort(

[ossu, 2016/07/08 11:02:13]

Suggestion: rework shouldMixAfter into shouldMixBefore and use it directly,
rather than adding a lambda around it. shouldMixBefore is more congruent with
using operator< for comparisons as well.

[aleloi, 2016/07/08 13:45:24]

Done. Btw, I got the impression from reading the style guide that I should
probably not define an operator< for the participant struct. Because it's not
clear how it should behave, I only use it once, and I don't define all the other
comparison operators. Do you agree with me here?

[ossu, 2016/07/08 13:58:34]

That makes perfect sense. Reading up on that, I might have to go back and revise
one of my CLs. You could probably hide it even more by not making it a member of
(what once was) ParticipantFrameStruct but just defining it here, possibly as a
named lambda. I'm not going to insist on it, though. As long as it has a name or
a comment describing how things are being sorted, I'm happy! 

+      participantMixingDataList.begin(), participantMixingDataList.end(),
+      [](const ParticipantFrameStruct& a, const ParticipantFrameStruct& b) {
+        return b.shouldMixAfter(a);
+      });
+
+  // Go through list in order and put things in mixList.
+  for (ParticipantFrameStruct& p : participantMixingDataList) {
+    // Filter muted.
+    if (p.muted_) {
+      p.participant_->_mixHistory->SetIsMixed(false);
+      continue;
     }
 
-    // TODO(henrike): this assert triggers in some test cases where SRTP is
-    // used which prevents NetEQ from making a VAD. Temporarily disable this
-    // assert until the problem is fixed on a higher level.
-    // RTC_DCHECK(audio_frame->vad_activity_ != AudioFrame::kVadUnknown);
-    if (audio_frame->vad_activity_ == AudioFrame::kVadUnknown) {
-      WEBRTC_TRACE(kTraceWarning, kTraceAudioMixerServer, _id,
-                   "invalid VAD state from participant");
+    // Add frame to result vectorfor mixing.

[ossu, 2016/07/08 11:02:14]

vectorfor -> vector for

[aleloi, 2016/07/08 13:45:23]

Done.

+    bool is_mixed = false;
+    if (maxAudioFrameCounter > 0) {
+      --maxAudioFrameCounter;
+      if (!p.was_mixed_before_) {
+        RampIn(*p.audio_frame_);
+      }
+      result.emplace_back(p.audio_frame_, false);
+      is_mixed = true;
     }
 
-    if (audio_frame->vad_activity_ == AudioFrame::kVadActive) {
-      if (!wasMixed && !muted) {
-        RampIn(*audio_frame);
-      }
+    // Ramp out unmuted.
+    if (p.was_mixed_before_ && !is_mixed) {
+      RampOut(*p.audio_frame_);
+      result.emplace_back(p.audio_frame_, false);
+    }
 
-      if (activeList.size() >= *maxAudioFrameCounter) {
-        // There are already more active participants than should be
-        // mixed. Only keep the ones with the highest energy.
-        AudioFrameList::iterator replaceItem;
-        uint32_t lowestEnergy = muted ? 0 : CalculateEnergy(*audio_frame);
-
-        bool found_replace_item = false;
-        for (AudioFrameList::iterator iter = activeList.begin();
-             iter != activeList.end(); ++iter) {
-          const uint32_t energy = muted ? 0 : CalculateEnergy(*iter->frame);
-          if (energy < lowestEnergy) {
-            replaceItem = iter;
-            lowestEnergy = energy;
-            found_replace_item = true;
-          }
-        }
-        if (found_replace_item) {
-          RTC_DCHECK(!muted);  // Cannot replace with a muted frame.
-          FrameAndMuteInfo replaceFrame = *replaceItem;
-
-          bool replaceWasMixed = false;
-          std::map<int, MixerAudioSource*>::const_iterator it =
-              mixParticipantList->find(replaceFrame.frame->id_);
-
-          // When a frame is pushed to |activeList| it is also pushed
-          // to mixParticipantList with the frame's id. This means
-          // that the Find call above should never fail.
-          RTC_DCHECK(it != mixParticipantList->end());
-          replaceWasMixed = it->second->_mixHistory->WasMixed();
-
-          mixParticipantList->erase(replaceFrame.frame->id_);
-          activeList.erase(replaceItem);
-
-          activeList.push_front(FrameAndMuteInfo(audio_frame, muted));
-          (*mixParticipantList)[audio_frame->id_] = *participant;
-          RTC_DCHECK(mixParticipantList->size() <=
-                     kMaximumAmountOfMixedParticipants);
-
-          if (replaceWasMixed) {
-            if (!replaceFrame.muted) {
-              RampOut(*replaceFrame.frame);
-            }
-            rampOutList->push_back(replaceFrame);
-            RTC_DCHECK(rampOutList->size() <=
-                       kMaximumAmountOfMixedParticipants);
-          }
-        } else {
-          if (wasMixed) {
-            if (!muted) {
-              RampOut(*audio_frame);
-            }
-            rampOutList->push_back(FrameAndMuteInfo(audio_frame, muted));
-            assert(rampOutList->size() <= kMaximumAmountOfMixedParticipants);
-          }
-        }
-      } else {
-        activeList.push_front(FrameAndMuteInfo(audio_frame, muted));
-        (*mixParticipantList)[audio_frame->id_] = *participant;
-        RTC_DCHECK(mixParticipantList->size() <=
-                   kMaximumAmountOfMixedParticipants);
-      }
-    } else {
-      if (wasMixed) {
-        ParticipantFrameStruct* part_struct =
-            new ParticipantFrameStruct(*participant, audio_frame, muted);
-        passiveWasMixedList.push_back(part_struct);
-      } else if (mustAddToPassiveList) {
-        if (!muted) {
-          RampIn(*audio_frame);
-        }
-        ParticipantFrameStruct* part_struct =
-            new ParticipantFrameStruct(*participant, audio_frame, muted);
-        passiveWasNotMixedList.push_back(part_struct);
-      }
-    }
+    p.participant_->_mixHistory->SetIsMixed(is_mixed);
   }
-  RTC_DCHECK(activeList.size() <= *maxAudioFrameCounter);
-  // At this point it is known which participants should be mixed. Transfer
-  // this information to this functions output parameters.
-  for (AudioFrameList::const_iterator iter = activeList.begin();
-       iter != activeList.end(); ++iter) {
-    mixList->push_back(*iter);
-  }
-  activeList.clear();
-  // Always mix a constant number of AudioFrames. If there aren't enough
-  // active participants mix passive ones. Starting with those that was mixed
-  // last iteration.
-  for (ParticipantFrameStructList::const_iterator iter =
-           passiveWasMixedList.begin();
-       iter != passiveWasMixedList.end(); ++iter) {
-    if (mixList->size() < *maxAudioFrameCounter + mixListStartSize) {
-      mixList->push_back(FrameAndMuteInfo((*iter)->audioFrame, (*iter)->muted));
-      (*mixParticipantList)[(*iter)->audioFrame->id_] = (*iter)->participant;
-      RTC_DCHECK(mixParticipantList->size() <=
-                 kMaximumAmountOfMixedParticipants);
-    }
-    delete *iter;
-  }
-  // And finally the ones that have not been mixed for a while.
-  for (ParticipantFrameStructList::const_iterator iter =
-           passiveWasNotMixedList.begin();
-       iter != passiveWasNotMixedList.end(); ++iter) {
-    if (mixList->size() < *maxAudioFrameCounter + mixListStartSize) {
-      mixList->push_back(FrameAndMuteInfo((*iter)->audioFrame, (*iter)->muted));
-      (*mixParticipantList)[(*iter)->audioFrame->id_] = (*iter)->participant;
-      RTC_DCHECK(mixParticipantList->size() <=
-                 kMaximumAmountOfMixedParticipants);
-    }
-    delete *iter;
-  }
-  RTC_DCHECK(*maxAudioFrameCounter + mixListStartSize >= mixList->size());
-  *maxAudioFrameCounter += mixListStartSize - mixList->size();
+  return result;
 }
 
 void NewAudioConferenceMixerImpl::GetAdditionalAudio(

[ossu, 2016/07/08 11:02:14]

Not necessary for this CL, but this should be changed to returning an
AudioFrameList rather than taking one as an output parameter.

[aleloi, 2016/07/08 13:45:23]

Acknowledged.

     AudioFrameList* additionalFramesList) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
                "GetAdditionalAudio(additionalFramesList)");
   // The GetAudioFrameWithMuted() callback may result in the participant being
   // removed from additionalParticipantList_. If that happens it will
   // invalidate any iterators. Create a copy of the participants list such
   // that the list of participants can be traversed safely.
   MixerAudioSourceList additionalParticipantList;
   additionalParticipantList.insert(additionalParticipantList.begin(),
                                    _additionalParticipantList.begin(),
@@ skipping 13 matching lines @@
     }
     if (audio_frame->samples_per_channel_ == 0) {
       // Empty frame. Don't use it.
       continue;
     }
     additionalFramesList->push_back(FrameAndMuteInfo(
         audio_frame, ret == MixerAudioSource::AudioFrameInfo::kMuted));
   }
 }
 
-void NewAudioConferenceMixerImpl::UpdateMixedStatus(
-    const std::map<int, MixerAudioSource*>& mixedParticipantsMap) const {
-  WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
-               "UpdateMixedStatus(mixedParticipantsMap)");
-  RTC_DCHECK(mixedParticipantsMap.size() <= kMaximumAmountOfMixedParticipants);
-
-  // Loop through all participants. If they are in the mix map they
-  // were mixed.
-  for (MixerAudioSourceList::const_iterator participant =
-           _participantList.begin();
-       participant != _participantList.end(); ++participant) {
-    bool isMixed = false;
-    for (std::map<int, MixerAudioSource*>::const_iterator it =
-             mixedParticipantsMap.begin();
-         it != mixedParticipantsMap.end(); ++it) {
-      if (it->second == *participant) {
-        isMixed = true;
-        break;
-      }
-    }
-    (*participant)->_mixHistory->SetIsMixed(isMixed);
-  }
-}
-
-void NewAudioConferenceMixerImpl::ClearAudioFrameList(
-    AudioFrameList* audioFrameList) const {
-  WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
-               "ClearAudioFrameList(audioFrameList)");
-  audioFrameList->clear();
-}
-
 bool NewAudioConferenceMixerImpl::IsParticipantInList(
     const MixerAudioSource& participant,
     const MixerAudioSourceList& participantList) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
                "IsParticipantInList(participant,participantList)");
   for (MixerAudioSourceList::const_iterator iter = participantList.begin();
        iter != participantList.end(); ++iter) {
     if (&participant == *iter) {
       return true;
     }
@@ skipping 115 matching lines @@
 
   if (error != _limiter->kNoError) {
     WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, _id,
                  "Error from AudioProcessing: %d", error);
     RTC_DCHECK(false);
     return false;
   }
   return true;
 }
 }  // namespace webrtc