Replace AudioConferenceMixer with AudioMixer.

webrtc/audio/audio_receive_stream.cc

 /*
  *  Copyright (c) 2015 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/audio/audio_receive_stream.h"
 
 #include <string>
 #include <utility>
 
 #include "webrtc/api/call/audio_sink.h"
-#include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "webrtc/voice_engine/channel_proxy.h"
 #include "webrtc/voice_engine/include/voe_base.h"
 #include "webrtc/voice_engine/include/voe_codec.h"
 #include "webrtc/voice_engine/include/voe_neteq_stats.h"
@@ skipping 107 matching lines @@
       congestion_controller->packet_router());
   if (UseSendSideBwe(config)) {
     remote_bitrate_estimator_ =
         congestion_controller->GetRemoteBitrateEstimator(true);
   }
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioReceiveStream: " << config_.ToString();
-  Stop();
+  if (playing_) {
+    Stop();
+  }
   channel_proxy_->DeRegisterExternalTransport();
   channel_proxy_->ResetCongestionControlObjects();
   channel_proxy_->SetRtcEventLog(nullptr);
   if (remote_bitrate_estimator_) {
     remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc);
   }
 }
 
 void AudioReceiveStream::Start() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);

[the sun, 2016/11/14 20:03:46]

I don't like that some methods in this class use
RTC_DCHECK_RUN_ON(&thread_checker_);
and some use
RTC_DCHECK(thread_checker_.CalledOnValidThread());

Please make it consistent.

[aleloi, 2016/11/15 16:56:54]

Done.

+  if (playing_) {
+    return;
+  }
+
   ScopedVoEInterface<VoEBase> base(voice_engine());
   int error = base->StartPlayout(config_.voe_channel_id);
   if (error != 0) {
-    LOG(LS_ERROR) << "AudioReceiveStream::Start failed with error: " << error;
+    RTC_NOTREACHED() << "AudioReceiveStream::Start failed with error: "

[the sun, 2016/11/14 20:03:46]

Why did you change this? We can conceivably end up here at run time, so this
should still be logging, not "NOTREACHED"

[aleloi, 2016/11/15 16:56:54]

Point, that was wrong.

+                     << error;
+    return;
+  }
+
+  playing_ = true;

[the sun, 2016/11/14 20:03:46]

Note that you're violating the AudioMixer promises of never calling
RemoveSource() unless the source was successfully added. You need to sort out
the logic and and the missing test case.

[aleloi, 2016/11/15 16:56:54]

Acknowledged.

+
+  auto* const the_audio_state = audio_state();
+
+  if (!the_audio_state->mixer()->AddSource(this)) {
+    LOG(LS_ERROR) << "Failed to add source to mixer.";
   }
 }
 
 void AudioReceiveStream::Stop() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+
+  if (!playing_) {
+    return;
+  }
+  playing_ = false;
+
+  auto* const the_audio_state = audio_state();
+  if (!the_audio_state->mixer()->RemoveSource(this)) {
+    RTC_NOTREACHED() << "Failed to remove stream from mixer.";

[the sun, 2016/11/14 20:03:46]

If the return code is *supposed* to always be true, what's the point of it.
Change the interface.

[aleloi, 2016/11/15 16:56:54]

For AudioMixerImpl it's supposed to always be true. What about other
implementations? I thought an AddSource() return code of 'false' allowed  other
impls that way have more complex initialization to signal failure. 

But then asserting that it will never happen here is wrong... I'll ask the
interested parties about changing the interface. We shouldn't land this before
we've sorted it out of course.

+  }
+
   ScopedVoEInterface<VoEBase> base(voice_engine());
   base->StopPlayout(config_.voe_channel_id);
 }
 
 webrtc::AudioReceiveStream::Stats AudioReceiveStream::GetStats() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   webrtc::AudioReceiveStream::Stats stats;
   stats.remote_ssrc = config_.rtp.remote_ssrc;
   ScopedVoEInterface<VoECodec> codec(voice_engine());
 
@@ skipping 102 matching lines @@
 }
 
 int AudioReceiveStream::PreferredSampleRate() const {
   return channel_proxy_->NeededFrequency();
 }
 
 int AudioReceiveStream::Ssrc() const {
   return config_.rtp.local_ssrc;
 }
 
+internal::AudioState* AudioReceiveStream::audio_state() const {
+  return static_cast<internal::AudioState*>(audio_state_.get());

[the sun, 2016/11/14 20:03:46]

Pull into a local variable and DCHECK it's not null before cast+return.

[aleloi, 2016/11/15 16:56:54]

Done.

+}
+
 VoiceEngine* AudioReceiveStream::voice_engine() const {
-  internal::AudioState* audio_state =
-      static_cast<internal::AudioState*>(audio_state_.get());
-  VoiceEngine* voice_engine = audio_state->voice_engine();
+  internal::AudioState* the_audio_state = audio_state();

[the sun, 2016/11/14 20:03:46]

Make that a one-liner instead

[aleloi, 2016/11/15 16:56:54]

Done.

+  VoiceEngine* voice_engine = the_audio_state->voice_engine();
   RTC_DCHECK(voice_engine);
   return voice_engine;
 }
 }  // namespace internal
 }  // namespace webrtc