Replace AudioConferenceMixer with AudioMixer.

webrtc/audio/audio_receive_stream.cc

Index: webrtc/audio/audio_receive_stream.cc
diff --git a/webrtc/audio/audio_receive_stream.cc b/webrtc/audio/audio_receive_stream.cc
index 12b8b3f7ad568582c1fb45b912c328b7b74b2deb..ab06537562745e16c7dcc917ad5dd81fe6cf7018 100644
--- a/webrtc/audio/audio_receive_stream.cc
+++ b/webrtc/audio/audio_receive_stream.cc
@@ -154,16 +154,42 @@ AudioReceiveStream::~AudioReceiveStream() {
 }
 
 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);
 }