Removed callback between old AudioConferenceMixer and OutputMixer. The audio frame with mixed audio…

webrtc/modules/audio_mixer/source/new_audio_conference_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 93 matching lines @@
   if (!mixer->Init()) {
     delete mixer;
     return NULL;
   }
   return mixer;
 }
 
 NewAudioConferenceMixerImpl::NewAudioConferenceMixerImpl(int id)
     : _id(id),
       _minimumMixingFreq(kLowestPossible),
-      _mixReceiver(NULL),
       _outputFrequency(kDefaultFrequency),
       _sampleSize(0),
       _audioFramePool(NULL),
       _participantList(),
       _additionalParticipantList(),
       _numMixedParticipants(0),
       use_limiter_(true),
       _timeStamp(0),
       _timeScheduler(kProcessPeriodicityInMs),
       _processCalls(0) {}
@@ skipping 57 matching lines @@
     WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, _id,
                  "failed in TimeToNextUpdate() call");
     // Sanity check
     assert(false);
     return -1;
   }
   return timeUntilNextProcess;
 }
 
 void NewAudioConferenceMixerImpl::Process() {
+  RTC_NOTREACHED();

[tommi, 2016/07/06 19:42:17]

todo to remove?

[aleloi, 2016/07/07 09:20:16]

Done. (There is also a dependent CL https://codereview.webrtc.org/2109333006/
that removes this).

+}
+
+void NewAudioConferenceMixerImpl::Mix(AudioFrame* audio_frame_for_mixing) {
   size_t remainingParticipantsAllowedToMix = kMaximumAmountOfMixedParticipants;
   {
     CriticalSectionScoped cs(_crit.get());
     assert(_processCalls == 0);

[tommi, 2016/07/06 19:42:17]

can you change all assert()'s in this file to RTC_DCHECK?

[aleloi, 2016/07/07 09:20:16]

Done.

     _processCalls++;
 
     // Let the scheduler know that we are running one iteration.
     _timeScheduler.UpdateScheduler();
   }
 
   AudioFrameList mixList;
   AudioFrameList rampOutList;
   AudioFrameList additionalFramesList;
   std::map<int, MixerAudioSource*> mixedParticipantsMap;
@@ skipping 46 matching lines @@
       }
     }
 
     UpdateToMix(&mixList, &rampOutList, &mixedParticipantsMap,
                 &remainingParticipantsAllowedToMix);
 
     GetAdditionalAudio(&additionalFramesList);
     UpdateMixedStatus(mixedParticipantsMap);
   }
 
-  // Get an AudioFrame for mixing from the memory pool.
-  AudioFrame* mixedAudio = NULL;
-  if (_audioFramePool->PopMemory(mixedAudio) == -1) {
-    WEBRTC_TRACE(kTraceMemory, kTraceAudioMixerServer, _id,
-                 "failed PopMemory() call");
-    assert(false);
-    return;
-  }
-
   {
     CriticalSectionScoped cs(_crit.get());
 
     // 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)));
 
-    mixedAudio->UpdateFrame(-1, _timeStamp, NULL, 0, _outputFrequency,
-                            AudioFrame::kNormalSpeech, AudioFrame::kVadPassive,
-                            num_mixed_channels);
+    audio_frame_for_mixing->UpdateFrame(

[tommi, 2016/07/06 19:42:17]

is there a way to avoid calling UpdateFrame() while we hold the above lock?

+        -1, _timeStamp, NULL, 0, _outputFrequency, AudioFrame::kNormalSpeech,
+        AudioFrame::kVadPassive, num_mixed_channels);
 
     _timeStamp += static_cast<uint32_t>(_sampleSize);

[tommi, 2016/07/06 19:42:17]

_timeStamp is only used in this method, so possibly a lock is not required for
it.  However, if there's a chance that _sampleSize can change while this method
is being called, a lock would be needed.  If there's no chance of that though,
then guarding _sampleSize and _outputFrequency with a lock, isn't needed and
instead making sure that this callback cannot occur at the time that those
variables may be modified, could replace the lock.

[aleloi, 2016/07/07 09:20:16]

_sampleSize and _outputFrequency are only changed through calls from this method
(::Mix), and the frame is only touched by the sound card OS thread, so the lock
may be safely removed.

[tommi, 2016/07/08 12:24:17]

Acknowledged.

 
     // We only use the limiter if it supports the output sample rate and
     // we're actually mixing multiple streams.
     use_limiter_ = _numMixedParticipants > 1 &&
                    _outputFrequency <= AudioProcessing::kMaxNativeSampleRateHz;
 
-    MixFromList(mixedAudio, mixList);
-    MixAnonomouslyFromList(mixedAudio, additionalFramesList);
-    MixAnonomouslyFromList(mixedAudio, rampOutList);
+    MixFromList(audio_frame_for_mixing, mixList);
+    MixAnonomouslyFromList(audio_frame_for_mixing, additionalFramesList);
+    MixAnonomouslyFromList(audio_frame_for_mixing, rampOutList);
 
-    if (mixedAudio->samples_per_channel_ == 0) {
+    if (audio_frame_for_mixing->samples_per_channel_ == 0) {
       // Nothing was mixed, set the audio samples to silence.
-      mixedAudio->samples_per_channel_ = _sampleSize;
-      mixedAudio->Mute();
+      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(mixedAudio);
+      LimitMixedAudio(audio_frame_for_mixing);
     }
   }
 
-  {
-    CriticalSectionScoped cs(_cbCrit.get());
-    if (_mixReceiver != NULL) {
-      const AudioFrame** dummy = NULL;
-      _mixReceiver->NewMixedAudio(_id, *mixedAudio, dummy, 0);
-    }
-  }
-
-  // Reclaim all outstanding memory.
-  _audioFramePool->PushMemory(mixedAudio);
   ClearAudioFrameList(&mixList);
   ClearAudioFrameList(&rampOutList);
   ClearAudioFrameList(&additionalFramesList);
   {
     CriticalSectionScoped cs(_crit.get());
     _processCalls--;
   }
   return;
 }
 
-int32_t NewAudioConferenceMixerImpl::RegisterMixedStreamCallback(
-    OldAudioMixerOutputReceiver* mixReceiver) {
-  CriticalSectionScoped cs(_cbCrit.get());
-  if (_mixReceiver != NULL) {
-    return -1;
-  }
-  _mixReceiver = mixReceiver;
-  return 0;
-}
-
-int32_t NewAudioConferenceMixerImpl::UnRegisterMixedStreamCallback() {
-  CriticalSectionScoped cs(_cbCrit.get());
-  if (_mixReceiver == NULL) {
-    return -1;
-  }
-  _mixReceiver = NULL;
-  return 0;
-}
-
 int32_t NewAudioConferenceMixerImpl::SetOutputFrequency(
     const Frequency& frequency) {
   CriticalSectionScoped cs(_crit.get());
 
   _outputFrequency = frequency;
   _sampleSize =
       static_cast<size_t>((_outputFrequency * kProcessPeriodicityInMs) / 1000);
 
   return 0;
 }
@@ skipping 451 matching lines @@
 int32_t NewAudioConferenceMixerImpl::MixFromList(
     AudioFrame* mixedAudio,
     const AudioFrameList& audioFrameList) const {
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, _id,
                "MixFromList(mixedAudio, audioFrameList)");
   if (audioFrameList.empty())
     return 0;
 
   uint32_t position = 0;
 
   if (_numMixedParticipants == 1) {

[tommi, 2016/07/06 19:42:17]

btw, it looks like this variable is the only reason why a lock is required to
call this method.
You could consider changing the method to accept this variable's value as an
input param and make it static.  That way, where you call it, you can fetch the
value while holding the lock and have the chance of not holding the lock while
mixing.

[aleloi, 2016/07/07 09:20:16]

Thank you for the idea! I think more unnecessary locks can be removed this way.
I have implemented your suggestion. Is it OK to wait for a thorough review of
the lock usage to another CL, though? 

     mixedAudio->timestamp_ = audioFrameList.front().frame->timestamp_;
     mixedAudio->elapsed_time_ms_ =
         audioFrameList.front().frame->elapsed_time_ms_;
   } else {
     // TODO(wu): Issue 3390.
     // Audio frame timestamp is only supported in one channel case.
     mixedAudio->timestamp_ = 0;
     mixedAudio->elapsed_time_ms_ = -1;
   }
 
@@ skipping 60 matching lines @@
 
   if (error != _limiter->kNoError) {
     WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, _id,
                  "Error from AudioProcessing: %d", error);
     assert(false);
     return false;
   }
   return true;
 }
 }  // namespace webrtc