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

webrtc/media/engine/webrtcvoiceengine.cc

 /*
  *  Copyright (c) 2004 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 1140 matching lines @@
     RTC_DCHECK(call);
     audio_capture_thread_checker_.DetachFromThread();
     config_.rtp.ssrc = ssrc;
     config_.rtp.c_name = c_name;
     config_.voe_channel_id = ch;
     RecreateAudioSendStream(extensions);
   }
 
   ~WebRtcAudioSendStream() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    Stop();
+    StopRendering();
     call_->DestroyAudioSendStream(stream_);
   }
 
   void RecreateAudioSendStream(
       const std::vector<webrtc::RtpExtension>& extensions) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (stream_) {
       call_->DestroyAudioSendStream(stream_);
       stream_ = nullptr;
     }
     config_.rtp.extensions = extensions;
     RTC_DCHECK(!stream_);
     stream_ = call_->CreateAudioSendStream(config_);
     RTC_CHECK(stream_);
   }
 
   bool SendTelephoneEvent(int payload_type, uint8_t event,
                           uint32_t duration_ms) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     RTC_DCHECK(stream_);
     return stream_->SendTelephoneEvent(payload_type, event, duration_ms);
   }
 
   webrtc::AudioSendStream::Stats GetStats() const {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     RTC_DCHECK(stream_);
     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_) {
       RTC_DCHECK(renderer_ == renderer);
       return;
     }
     renderer->SetSink(this);
     renderer_ = renderer;
   }
 
   // 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);
       renderer_ = nullptr;
     }
   }
 
   // AudioRenderer::Sink implementation.
   // This method is called on the audio thread.
   void OnData(const void* audio_data,
@@ skipping 578 matching lines @@
                     << ch.second->channel() << " failed";
       return false;
     }
   }
   playout_ = playout;
   return true;
 }
 
 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;
 }
 
 bool WebRtcVoiceMediaChannel::PauseSend() {
   return ChangeSend(SEND_NOTHING);
 }
 
 bool WebRtcVoiceMediaChannel::ResumeSend() {
   return ChangeSend(desired_send_);
 }
 
 bool WebRtcVoiceMediaChannel::ChangeSend(SendFlags send) {
   if (send_ == send) {
     return true;
   }
 
   // Apply channel specific options when channel is enabled for sending.
   if (send == SEND_MICROPHONE) {
     engine()->ApplyOptions(options_);
   }
 
   // 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;
     }
   }
 
   send_ = 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;
     }
   }
 
   return true;
 }
 
 bool WebRtcVoiceMediaChannel::SetAudioSend(uint32_t ssrc,
                                            bool enable,
                                            const AudioOptions* options,
                                            AudioRenderer* renderer) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   // TODO(solenberg): The state change should be fully rolled back if any one of
   //                  these calls fail.
   if (!SetLocalRenderer(ssrc, renderer)) {
     return false;
   }
   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);
   }
   return true;
 }
 
 int WebRtcVoiceMediaChannel::CreateVoEChannel() {
   int id = engine()->CreateVoEChannel();
   if (id == -1) {
     LOG_RTCERR0(CreateVoEChannel);
@@ skipping 61 matching lines @@
       if (engine()->voe()->rtp()->SetLocalSSRC(recv_channel, ssrc) != 0) {
         LOG_RTCERR2(SetLocalSSRC, recv_channel, ssrc);
         return false;
       }
       engine()->voe()->base()->AssociateSendChannel(recv_channel, channel);
       LOG(LS_INFO) << "VoiceEngine channel #" << recv_channel
                    << " is associated with channel #" << channel << ".";
     }
   }
 
-  return ChangeSend(channel, desired_send_);
+  return UpdateChannelSendState(ssrc, send_);
 }
 
 bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "RemoveSendStream: " << ssrc;
 
   auto it = send_streams_.find(ssrc);
   if (it == send_streams_.end()) {
     LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc
                     << " which doesn't exist.";
     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;
   send_streams_.erase(it);
   if (!DeleteVoEChannel(channel)) {
     return false;
   }
   if (send_streams_.empty()) {
     ChangeSend(SEND_NOTHING);
   }
@@ skipping 100 matching lines @@
   // Clean up and delete the receive stream+channel.
   LOG(LS_INFO) << "Removing audio receive stream " << ssrc
                << " with VoiceEngine channel #" << channel << ".";
   it->second->SetRawAudioSink(nullptr);
   delete it->second;
   recv_streams_.erase(it);
   return DeleteVoEChannel(channel);
 }
 
 bool WebRtcVoiceMediaChannel::SetLocalRenderer(uint32_t ssrc,
                                                AudioRenderer* renderer) {

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

Here, too, calling it SetSource, would be a lot nicer.

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

Done.

   auto it = send_streams_.find(ssrc);
   if (it == send_streams_.end()) {
     if (renderer) {
       // Return an error if trying to set a valid renderer with an invalid ssrc.
       LOG(LS_ERROR) << "SetLocalRenderer failed with ssrc "<< ssrc;
       return false;
     }
 
     // The channel likely has gone away, do nothing.
     return true;
   }
 
   if (renderer) {
-    it->second->Start(renderer);
+    it->second->StartRendering(renderer);
   } else {
-    it->second->Stop();
+    it->second->StopRendering();
   }
 
   return true;
 }
 
 bool WebRtcVoiceMediaChannel::GetActiveStreams(
     AudioInfo::StreamList* actives) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   actives->clear();
   for (const auto& ch : recv_streams_) {
@@ skipping 401 matching lines @@
     }
   } else {
     LOG(LS_INFO) << "Stopping playout for channel #" << channel;
     engine()->voe()->base()->StopPlayout(channel);
   }
   return true;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE