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.
  */
 
@@ skipping 136 matching lines @@
   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/10/27 10:06:45]

This is nice, but should all thread checking in this class use _RUN_ON instead
of checking CalledOnValidThread()?

[aleloi, 2016/11/01 15:17:34]

I added an ACCESS_ON annotation on playout_. Now every method that accesses it
needs to acquire (lockable) thread_checker_, which is done by the
RTC_DCHECK_RUN_ON macro in debug mode (I think the locking is only there to
trick Clang++, and no real mutex locks are acquired at runtime: in
thread_checker.h EXCLUSIVE_LOCK_FUNCTION seems to have an empty implementation).

tldr, if I change it back, I have to remove the guard annotation from playout_.

+  if (playing_) {

[ossu, 2016/10/25 14:13:33]

Is this fine or does it indicate a usage error?
I.e. would it be useful to be told that you've messed up your Start()/Stop()
calls rather than just swallowing any combination of them?

[the sun, 2016/10/27 10:06:45]

Since this is going to be part of the public API I think a defensive approach is
preferable. For anything internal, DCHECKs should be  used though.

[aleloi, 2016/11/01 15:17:34]

webrtcvoiceengine runs Stop on everything in SetPlayout without bothering to
check. If I were to add error logging or checks, we'd have to fix it first.

+    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;
+    return;
+  }
+
+  playing_ = true;
+
+  auto* const audio_state =

[the sun, 2016/10/27 10:06:45]

Make a class private method internal::AudioState* audio_state() which does does
this casting for you. Use it in all the places we cast.

[aleloi, 2016/11/01 15:17:34]

That's a design tip from Scott Meyers! I read that just a few days ago :)

Fixed in next patch set.

+      static_cast<internal::AudioState*>(audio_state_.get());

[ossu, 2016/10/25 14:13:33]

I wonder about the cast here. I see it's being done in other parts of
AudioReceiveStream as well. Do you know why and does that apply to your case?
Should the AudioState interface not expose the mixer?

[the sun, 2016/10/27 10:06:45]

AudioState is intended as (for clients of the Call API) an opaque type which
keeps the (audio) state that's shared between multiple Call instances. The
solution here is in line with that design choice.

+
+  if (!audio_state->mixer()->AddSource(this)) {
+    LOG(LS_ERROR) << "Failed to add source to mixer.";

[the sun, 2016/10/27 10:06:45]

Does that mean we're not actually playing and we should have left playing_ ==
false so that we try again?

Or does it mean the result from AddSource() is actually pointless?

[aleloi, 2016/11/01 15:17:35]

Thanks! It's an error that is never supposed to happen. If we are actually
playing, the the receive stream is in an invalid state. I think a NOTREACHED
assertion fits here.

   }
 }
 
 void AudioReceiveStream::Stop() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
+
+  if (!playing_) {
+    return;
+  }
+  playing_ = false;
+
+  auto* const audio_state =
+      static_cast<internal::AudioState*>(audio_state_.get());
+  if (!audio_state->mixer()->RemoveSource(this)) {

[the sun, 2016/10/27 10:06:45]

Uhm, sorry, I missed there was a return code for RemoveSource() - I should have
opposed that...

[aleloi, 2016/11/01 15:17:35]

It's not too late to change! But why? Isn't it the same as with AddSource: we
leave a possibility for mixer implementation to report that something is
seriously wrong? It's not supposed to happen anyway, so I've added NOTREACHED
here as well.

+    LOG(LS_ERROR) << "Failed to remove stream from mixer.";
+  }
+
   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 107 matching lines @@
 
 VoiceEngine* AudioReceiveStream::voice_engine() const {
   internal::AudioState* audio_state =
       static_cast<internal::AudioState*>(audio_state_.get());
   VoiceEngine* voice_engine = audio_state->voice_engine();
   RTC_DCHECK(voice_engine);
   return voice_engine;
 }
 }  // namespace internal
 }  // namespace webrtc