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 c50ff1159c6dcc4ae64d9043fc858a8dadff88d2..63f5f98be78a39f12630c9143bcbe933152c5b40 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -33,7 +33,7 @@
 #include "webrtc/call/rtc_event_log.h"
 #include "webrtc/common.h"
 #include "webrtc/media/base/audioframe.h"
-#include "webrtc/media/base/audiorenderer.h"
+#include "webrtc/media/base/audiosource.h"
 #include "webrtc/media/base/mediaconstants.h"
 #include "webrtc/media/base/streamparams.h"
 #include "webrtc/media/engine/webrtcmediaengine.h"
@@ -1136,7 +1136,7 @@ int WebRtcVoiceEngine::CreateVoEChannel() {
 }
 
 class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
-    : public AudioRenderer::Sink {
+    : public AudioSource::Sink {
  public:
   WebRtcAudioSendStream(int ch, webrtc::AudioTransport* voe_audio_transport,
                         uint32_t ssrc, const std::string& c_name,
@@ -1158,7 +1158,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
 
   ~WebRtcAudioSendStream() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    Stop();
+    ClearSource();
     call_->DestroyAudioSendStream(stream_);
   }
 
@@ -1188,33 +1188,38 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     return stream_->GetStats();
   }
 
-  // Starts the rendering by setting a sink to the renderer to get data
-  // callback.
+  bool HasSource() const {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    return source_ != nullptr;
+  }
+
+  // Starts the sending by setting ourselves as a sink to the AudioSource to
+  // get data callbacks.
   // This method is called on the libjingle worker thread.
   // TODO(xians): Make sure Start() is called only once.
-  void Start(AudioRenderer* renderer) {
+  void SetSource(AudioSource* source) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    RTC_DCHECK(renderer);
-    if (renderer_) {
-      RTC_DCHECK(renderer_ == renderer);
+    RTC_DCHECK(source);
+    if (source_) {
+      RTC_DCHECK(source_ == source);
       return;
     }
-    renderer->SetSink(this);
-    renderer_ = renderer;
+    source->SetSink(this);
+    source_ = source;
   }
 
-  // Stops rendering by setting the sink of the renderer to nullptr. No data
+  // Stops sending by setting the sink of the AudioSource to nullptr. No data
   // callback will be received after this method.
   // This method is called on the libjingle worker thread.
-  void Stop() {
+  void ClearSource() {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    if (renderer_) {
-      renderer_->SetSink(nullptr);
-      renderer_ = nullptr;
+    if (source_) {
+      source_->SetSink(nullptr);
+      source_ = nullptr;
     }
   }
 
-  // AudioRenderer::Sink implementation.
+  // AudioSource::Sink implementation.
   // This method is called on the audio thread.
   void OnData(const void* audio_data,
               int bits_per_sample,
@@ -1232,13 +1237,13 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
                                  number_of_frames);
   }
 
-  // Callback from the |renderer_| when it is going away. In case Start() has
+  // Callback from the |source_| when it is going away. In case Start() has
   // never been called, this callback won't be triggered.
   void OnClose() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    // Set |renderer_| to nullptr to make sure no more callback will get into
-    // the renderer.
-    renderer_ = nullptr;
+    // Set |source_| to nullptr to make sure no more callback will get into
+    // the source.
+    source_ = nullptr;
   }
 
   // Accessor to the VoE channel ID.
@@ -1257,10 +1262,10 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
   // configuration changes.
   webrtc::AudioSendStream* stream_ = nullptr;
 
-  // Raw pointer to AudioRenderer owned by LocalAudioTrackHandler.
+  // Raw pointer to AudioSource owned by LocalAudioTrackHandler.
   // PeerConnection will make sure invalidating the pointer before the object
   // goes away.
-  AudioRenderer* renderer_ = nullptr;
+  AudioSource* source_ = nullptr;
 
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(WebRtcAudioSendStream);
 };
@@ -1804,34 +1809,19 @@ bool WebRtcVoiceMediaChannel::ChangePlayout(bool playout) {
 }
 
 bool WebRtcVoiceMediaChannel::SetSend(SendFlags send) {
-  desired_send_ = send;
-  if (!send_streams_.empty()) {
-    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) {
+  // Apply channel specific options when channel is enabled for sending
+  // and we have at least one stream.
+  if (!send_streams_.empty() && send == SEND_MICROPHONE) {

[the sun, 2016/03/03 15:15:25]

nit: can we consistently check "send == SEND_..." first in these expressions?

[Taylor Brandstetter, 2016/03/04 16:06:56]

Done.

     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)) {
       return false;
     }
   }
@@ -1840,14 +1830,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) {

[the sun, 2016/03/03 15:15:25]

You could remove the SendFlags enum while you're at it. See:
https://codereview.webrtc.org/1499903002/

[Taylor Brandstetter, 2016/03/04 16:06:56]

Done.

+  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->HasSource()) {
     if (engine()->voe()->base()->StartSend(channel) == -1) {
       LOG_RTCERR1(StartSend, channel);
       return false;
     }
-  } else {  // SEND_NOTHING
-    RTC_DCHECK(send == SEND_NOTHING);
+  } else {  // send == SEND_NOTHING || !it->second->HasSource()
     if (engine()->voe()->base()->StopSend(channel) == -1) {
       LOG_RTCERR1(StopSend, channel);
       return false;
@@ -1860,16 +1859,21 @@ bool WebRtcVoiceMediaChannel::ChangeSend(int channel, SendFlags send) {
 bool WebRtcVoiceMediaChannel::SetAudioSend(uint32_t ssrc,
                                            bool enable,
                                            const AudioOptions* options,
-                                           AudioRenderer* renderer) {
+                                           AudioSource* source) {
   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)) {
+  if (!SetLocalSource(ssrc, source)) {
     return false;
   }
   if (!MuteStream(ssrc, !enable)) {
     return false;
   }
+  // If the source was set or unset we may need to update the sending
+  // state of the voe::Channel.
+  if (!UpdateChannelSendState(ssrc, send_)) {
+    return false;
+  }
   if (enable && options) {
     return SetOptions(*options);
   }
@@ -1951,7 +1955,12 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
     }
   }
 
-  return ChangeSend(channel, desired_send_);
+  // Apply channel specific options when channel is enabled for sending
+  // and we have at least one stream.

[the sun, 2016/03/03 15:15:25]

nit: at least -> exactly

[Taylor Brandstetter, 2016/03/04 16:06:56]

Done.

+  if (send_streams_.size() == 1u && send_ == SEND_MICROPHONE) {
+    engine()->ApplyOptions(options_);
+  }
+  return UpdateChannelSendState(ssrc, send_);
 }
 
 bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
@@ -1965,10 +1974,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;
@@ -1976,9 +1985,6 @@ bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
   if (!DeleteVoEChannel(channel)) {
     return false;
   }
-  if (send_streams_.empty()) {
-    ChangeSend(SEND_NOTHING);
-  }

[the sun, 2016/03/03 15:15:25]

Note that this had the effect of setting send_ = SEND_NOTHING, so that the next
new stream added won't automatically be enabled for sending. Am I missing the
logic where that behavior is maintained, or is there a reason for changing it
which eludes me?

[Taylor Brandstetter, 2016/03/04 16:06:56]

Actually, the new stream was created with desired_send_, so as far as I can tell
the only effect of ChangeSend(SEND_NOTHING) is that the next call of
SetSend(SEND_MICROPHONE) (while there are streams) will call ApplyOptions. I
tried to retain this logic by calling ApplyOptions when the first send stream is
created, though I removed it when merging your CL.

   return true;
 }
 
@@ -2088,13 +2094,13 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32_t ssrc) {
   return DeleteVoEChannel(channel);
 }
 
-bool WebRtcVoiceMediaChannel::SetLocalRenderer(uint32_t ssrc,
-                                               AudioRenderer* renderer) {
+bool WebRtcVoiceMediaChannel::SetLocalSource(uint32_t ssrc,
+                                             AudioSource* source) {
   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;
+    if (source) {
+      // Return an error if trying to set a valid source with an invalid ssrc.
+      LOG(LS_ERROR) << "SetLocalSource failed with ssrc " << ssrc;
       return false;
     }
 
@@ -2102,10 +2108,10 @@ bool WebRtcVoiceMediaChannel::SetLocalRenderer(uint32_t ssrc,
     return true;
   }
 
-  if (renderer) {
-    it->second->Start(renderer);
+  if (source) {
+    it->second->SetSource(source);
   } else {
-    it->second->Stop();
+    it->second->ClearSource();
   }
 
   return true;