Prevent a voice channel from sending data before a renderer is set.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index 28c50793aa3518cefa4fe35ca073e4f474022642..ac72ba3371f3719873a289be015fbee302a57773 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -1158,7 +1158,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
 
   ~WebRtcAudioSendStream() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    Stop();
+    StopRendering();
     call_->DestroyAudioSendStream(stream_);
   }
 
@@ -1188,11 +1188,16 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     return stream_->GetStats();
   }
 
+  bool IsRendering() const {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    return renderer_ != nullptr;
+  }
+
   // Starts the rendering by setting a sink to the renderer to get data
   // callback.
   // This method is called on the libjingle worker thread.
   // TODO(xians): Make sure Start() is called only once.
-  void Start(AudioRenderer* renderer) {
+  void StartRendering(AudioRenderer* renderer) {

[pthatcher1, 2016/03/02 22:18:40]

Since the AudioRenderer is really acting as a source, can we call this source_,
SetSource(), ClearSource(), and HasSource() instead of IsRendering,
StartRendering, StopRendering?

Call it rendering is very confusing in this context because it makes it sound
like audio going from the network to local playout, when in fact it is audio
from a local source going out to the network.

[Taylor Brandstetter, 2016/03/03 00:45:03]

I agree. I renamed it in a new patch, though it does change quite a few files.

     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     RTC_DCHECK(renderer);
     if (renderer_) {
@@ -1206,7 +1211,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
   // Stops rendering by setting the sink of the renderer to nullptr. No data
   // callback will be received after this method.
   // This method is called on the libjingle worker thread.
-  void Stop() {
+  void StopRendering() {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (renderer_) {
       renderer_->SetSink(nullptr);
@@ -1805,7 +1810,9 @@ bool WebRtcVoiceMediaChannel::ChangePlayout(bool playout) {
 
 bool WebRtcVoiceMediaChannel::SetSend(SendFlags send) {
   desired_send_ = send;
-  if (!send_streams_.empty()) {
+  if (send_streams_.empty()) {
+    send_ = send;

[pthatcher1, 2016/03/02 22:18:40]

Why do we have desired_send_ and send_?  Can't we just use desired_send_?

[Taylor Brandstetter, 2016/03/03 00:45:03]

For PauseSend/ResumeSend below. Though if Hangouts isn't using this, I don't
know who is, so maybe we can remove it.

Oh, I found one other place it's used. If the last stream is removed, "send_"
goes to SEND_NOTHING while desired_send_ stays at SEND_MICROPHONE. This has the
effect that when SetSend is called after a stream is added again,
engine()->ApplyOptions gets called. I think this could be accomplished by just
calling ApplyOptions when the first stream is added though, if this is really
the desired behavior.

+  } else {
     return ChangeSend(desired_send_);
   }
   return true;
@@ -1830,8 +1837,8 @@ bool WebRtcVoiceMediaChannel::ChangeSend(SendFlags send) {
   }
 
   // Change the settings on each send channel.
-  for (const auto& ch : send_streams_) {
-    if (!ChangeSend(ch.second->channel(), send)) {
+  for (const auto& kv : send_streams_) {
+    if (!UpdateChannelSendState(kv.first, send)) {

[pthatcher1, 2016/03/02 22:18:39]

This requires a lookup for every entry while we're doing an iteration.  A better
way to do this would be to have two UpdateChannelSendState methods: one that
does the ssrc => channel lookup for you and one that allows you to pass in the
already known channel.  The first can call the second.

[Taylor Brandstetter, 2016/03/03 00:45:03]

Just as an optimization, you mean? I really don't think this lookup should be
that expensive, and having one method is simpler than having two.

       return false;
     }
   }
@@ -1840,14 +1847,23 @@ bool WebRtcVoiceMediaChannel::ChangeSend(SendFlags send) {
   return true;
 }
 
-bool WebRtcVoiceMediaChannel::ChangeSend(int channel, SendFlags send) {
-  if (send == SEND_MICROPHONE) {
+bool WebRtcVoiceMediaChannel::UpdateChannelSendState(uint32_t ssrc,
+                                                     SendFlags send) {
+  RTC_DCHECK(send == SEND_NOTHING || send == SEND_MICROPHONE);
+
+  auto it = send_streams_.find(ssrc);
+  if (it == send_streams_.end()) {
+    RTC_DCHECK(false && "UpdateChannelSendState called with invalid SSRC.");
+    return false;
+  }
+
+  int channel = it->second->channel();
+  if (send == SEND_MICROPHONE && it->second->IsRendering()) {

[pthatcher1, 2016/03/02 22:18:39]

Yeah, this would be a lot easier to read:

send == SEND_MICROPHONE && it->second->HasSource() 

[Taylor Brandstetter, 2016/03/03 00:45:03]

Done.

     if (engine()->voe()->base()->StartSend(channel) == -1) {
       LOG_RTCERR1(StartSend, channel);
       return false;
     }
-  } else {  // SEND_NOTHING
-    RTC_DCHECK(send == SEND_NOTHING);
+  } else {  // SEND_NOTHING || !it->second->IsRendering()
     if (engine()->voe()->base()->StopSend(channel) == -1) {
       LOG_RTCERR1(StopSend, channel);
       return false;
@@ -1870,6 +1886,11 @@ bool WebRtcVoiceMediaChannel::SetAudioSend(uint32_t ssrc,
   if (!MuteStream(ssrc, !enable)) {
     return false;
   }
+  // If the renderer was set or unset we may need to update the sending
+  // state of the voe::Channel.

[pthatcher1, 2016/03/02 22:18:40]

And this would be a lot more clear as well: 

"If the source was set or cleared we may need to update the sending state"

[Taylor Brandstetter, 2016/03/03 00:45:03]

Done.

+  if (!UpdateChannelSendState(ssrc, send_)) {
+    return false;
+  }
   if (enable && options) {
     return SetOptions(*options);
   }
@@ -1951,7 +1972,7 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
     }
   }
 
-  return ChangeSend(channel, desired_send_);
+  return UpdateChannelSendState(ssrc, send_);
 }
 
 bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
@@ -1965,10 +1986,10 @@ bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
     return false;
   }
 
-  int channel = it->second->channel();
-  ChangeSend(channel, SEND_NOTHING);
+  UpdateChannelSendState(ssrc, SEND_NOTHING);
 
   // Clean up and delete the send stream+channel.
+  int channel = it->second->channel();
   LOG(LS_INFO) << "Removing audio send stream " << ssrc
                << " with VoiceEngine channel #" << channel << ".";
   delete it->second;
@@ -2103,9 +2124,9 @@ bool WebRtcVoiceMediaChannel::SetLocalRenderer(uint32_t ssrc,
   }
 
   if (renderer) {
-    it->second->Start(renderer);
+    it->second->StartRendering(renderer);
   } else {
-    it->second->Stop();
+    it->second->StopRendering();
   }
 
   return true;