Adding the ability to create an RtpSender without a track.

talk/app/webrtc/rtpsender.cc

Index: talk/app/webrtc/rtpsender.cc
diff --git a/talk/app/webrtc/rtpsender.cc b/talk/app/webrtc/rtpsender.cc
index 3a78f4598a8d359a6c34b1a93071250a4a1a8114..d645871227a6fd00a78d7c4bb74e145d150d0cdc 100644
--- a/talk/app/webrtc/rtpsender.cc
+++ b/talk/app/webrtc/rtpsender.cc
@@ -29,6 +29,7 @@
 
 #include "talk/app/webrtc/localaudiosource.h"
 #include "talk/app/webrtc/videosourceinterface.h"
+#include "webrtc/base/helpers.h"
 
 namespace webrtc {
 
@@ -59,33 +60,52 @@ void LocalAudioSinkAdapter::SetSink(cricket::AudioRenderer::Sink* sink) {
 }
 
 AudioRtpSender::AudioRtpSender(AudioTrackInterface* track,
-                               uint32_t ssrc,
-                               AudioProviderInterface* provider)
+                               const std::string& stream_id,
+                               AudioProviderInterface* provider,
+                               StatsCollector* stats)
     : id_(track->id()),
-      track_(track),
-      ssrc_(ssrc),
+      stream_id_(stream_id),
       provider_(provider),
+      stats_(stats),
+      track_(track),
       cached_track_enabled_(track->enabled()),
       sink_adapter_(new LocalAudioSinkAdapter()) {
+  RTC_DCHECK(provider != nullptr);
   track_->RegisterObserver(this);
   track_->AddSink(sink_adapter_.get());
-  Reconfigure();
 }
 
+AudioRtpSender::AudioRtpSender(AudioProviderInterface* provider,
+                               StatsCollector* stats)
+    : id_(rtc::CreateRandomUuid()),
+      stream_id_(rtc::CreateRandomUuid()),
+      provider_(provider),
+      stats_(stats),
+      sink_adapter_(new LocalAudioSinkAdapter()) {}
+
 AudioRtpSender::~AudioRtpSender() {
-  track_->RemoveSink(sink_adapter_.get());
-  track_->UnregisterObserver(this);
   Stop();
 }
 
 void AudioRtpSender::OnChanged() {
+  RTC_DCHECK(!stopped_);
   if (cached_track_enabled_ != track_->enabled()) {
     cached_track_enabled_ = track_->enabled();
-    Reconfigure();
+    if (ssrc_) {
+      SetAudioSend();
+    }
   }
 }
 
 bool AudioRtpSender::SetTrack(MediaStreamTrackInterface* track) {
+  if (stopped_) {
+    LOG(LS_ERROR) << "SetTrack can't be called on stopped RtpSender.";

[pthatcher1, 2015/10/20 17:42:49]

on stopped => on a stopped

[Taylor Brandstetter, 2015/10/21 00:22:08]

Done.

+    return false;
+  }
+  if (!track) {
+    LOG(LS_ERROR) << "SetTrack can't be called with NULL.";
+    return false;

[pthatcher1, 2015/10/20 17:42:49]

Can we not support this?  Why not just have it cause sending to stop?

[Taylor Brandstetter, 2015/10/21 00:22:08]

I thought we weren't supposed to support this, but I guess it could end up being
useful internally.

+  }
   if (track->kind() != "audio") {
     LOG(LS_ERROR) << "SetTrack called on audio RtpSender with " << track->kind()
                   << " track.";
@@ -94,36 +114,77 @@ bool AudioRtpSender::SetTrack(MediaStreamTrackInterface* track) {
   AudioTrackInterface* audio_track = static_cast<AudioTrackInterface*>(track);
 
   // Detach from old track.
-  track_->RemoveSink(sink_adapter_.get());
-  track_->UnregisterObserver(this);
+  if (track_) {
+    track_->RemoveSink(sink_adapter_.get());
+    track_->UnregisterObserver(this);
+    if (ssrc_) {
+      if (stats_) {
+        stats_->RemoveLocalAudioTrack(track_.get(), ssrc_);
+      }
+    }
+  }
 
   // Attach to new track.
   track_ = audio_track;
   cached_track_enabled_ = track_->enabled();
   track_->RegisterObserver(this);
   track_->AddSink(sink_adapter_.get());
-  Reconfigure();
+
+  if (ssrc_) {
+    SetAudioSend();
+    if (stats_) {
+      stats_->AddLocalAudioTrack(track_.get(), ssrc_);
+    }
+  }
   return true;
 }
 
-void AudioRtpSender::Stop() {
-  // TODO(deadbeef): Need to do more here to fully stop sending packets.
-  if (!provider_) {
+void AudioRtpSender::SetSsrc(uint32_t ssrc) {

[pthatcher1, 2015/10/20 17:42:49]

Do we support changing SSRCs for a given sender?  Would it be easier to just
make the rule that you can only set it once?

And can we unset it?  That seems like something that complicates the logic a bit
here.

[Taylor Brandstetter, 2015/10/21 00:22:08]

We talked about this a bit; basically, it was being supported BEFORE the
MediaStreamSignaling refactoring, and I wanted to be wary of making incompatible
changes in these CLs.

+  if (stopped_ || ssrc == ssrc_) {
     return;
   }
-  cricket::AudioOptions options;
-  provider_->SetAudioSend(ssrc_, false, options, nullptr);
-  provider_ = nullptr;
+  // If we are already sending with a particular SSRC, stop sending.
+  if (track_ && ssrc_) {
+    cricket::AudioOptions options;
+    provider_->SetAudioSend(ssrc_, false, options, nullptr);
+    if (stats_) {
+      stats_->RemoveLocalAudioTrack(track_.get(), ssrc_);
+    }
+  }
+  ssrc_ = ssrc;
+  if (track_ && ssrc_) {
+    SetAudioSend();
+    if (stats_) {
+      stats_->AddLocalAudioTrack(track_.get(), ssrc_);
+    }

[pthatcher1, 2015/10/20 17:42:49]

Do we support stats_ to be null?  Would it be easier to just say it must be
non-null?

[Taylor Brandstetter, 2015/10/21 00:22:08]

Yes, it's for unit tests, mainly.

+  }
 }
 
-void AudioRtpSender::Reconfigure() {
-  if (!provider_) {
+void AudioRtpSender::Stop() {
+  // TODO(deadbeef): Need to do more here to fully stop sending packets.
+  if (stopped_) {
     return;
   }
+  if (track_) {
+    track_->RemoveSink(sink_adapter_.get());
+    track_->UnregisterObserver(this);
+    if (ssrc_) {
+      cricket::AudioOptions options;
+      provider_->SetAudioSend(ssrc_, false, options, nullptr);
+      if (stats_) {
+        stats_->RemoveLocalAudioTrack(track_.get(), ssrc_);
+      }
+    }
+  }
+  stopped_ = true;
+}
+
+void AudioRtpSender::SetAudioSend() {
+  RTC_DCHECK(!stopped_ && track_ && ssrc_);

[pthatcher1, 2015/10/20 17:42:49]

Would it be easier to just do the check for stopped_, track_ and ssrc_ here
rather than at all the call sites?

[Taylor Brandstetter, 2015/10/21 00:22:08]

Maybe, but it would only save a few lines of code, and it would make SetTrack
more complicated. Would SetTrack get all the way to SetAudioSend, then fail?
What about the cases where SetAudioSend isn't called? I think this way is
cleaner.

[pthatcher1, 2015/10/22 07:34:14]

This does look good.

   cricket::AudioOptions options;
   if (track_->enabled() && track_->GetSource()) {
     // TODO(xians): Remove this static_cast since we should be able to connect
-    // a remote audio track to peer connection.
+    // a remote audio track to a peer connection.
     options = static_cast<LocalAudioSource*>(track_->GetSource())->options();
   }
 
@@ -136,34 +197,45 @@ void AudioRtpSender::Reconfigure() {
 }
 
 VideoRtpSender::VideoRtpSender(VideoTrackInterface* track,
-                               uint32_t ssrc,
+                               const std::string& stream_id,
                                VideoProviderInterface* provider)
     : id_(track->id()),
-      track_(track),
-      ssrc_(ssrc),
+      stream_id_(stream_id),
       provider_(provider),
+      track_(track),
       cached_track_enabled_(track->enabled()) {
+  RTC_DCHECK(provider != nullptr);
   track_->RegisterObserver(this);
-  VideoSourceInterface* source = track_->GetSource();
-  if (source) {
-    provider_->SetCaptureDevice(ssrc_, source->GetVideoCapturer());
-  }
-  Reconfigure();
 }
 
+VideoRtpSender::VideoRtpSender(VideoProviderInterface* provider)
+    : id_(rtc::CreateRandomUuid()),
+      stream_id_(rtc::CreateRandomUuid()),
+      provider_(provider) {}
+
 VideoRtpSender::~VideoRtpSender() {
-  track_->UnregisterObserver(this);
   Stop();
 }
 
 void VideoRtpSender::OnChanged() {
+  RTC_DCHECK(!stopped_);
   if (cached_track_enabled_ != track_->enabled()) {
     cached_track_enabled_ = track_->enabled();
-    Reconfigure();
+    if (ssrc_) {
+      SetVideoSend();
+    }
   }
 }
 
 bool VideoRtpSender::SetTrack(MediaStreamTrackInterface* track) {
+  if (stopped_) {
+    LOG(LS_ERROR) << "SetTrack can't be called on stopped RtpSender.";
+    return false;
+  }
+  if (!track) {
+    LOG(LS_ERROR) << "SetTrack can't be called with NULL.";
+    return false;
+  }

[pthatcher1, 2015/10/20 17:42:49]

Similarly as with audio: can we support this?

[Taylor Brandstetter, 2015/10/21 00:22:08]

Done.

   if (track->kind() != "video") {
     LOG(LS_ERROR) << "SetTrack called on video RtpSender with " << track->kind()
                   << " track.";
@@ -172,30 +244,65 @@ bool VideoRtpSender::SetTrack(MediaStreamTrackInterface* track) {
   VideoTrackInterface* video_track = static_cast<VideoTrackInterface*>(track);
 
   // Detach from old track.
-  track_->UnregisterObserver(this);
+  if (track_) {
+    track_->UnregisterObserver(this);
+  }
 
   // Attach to new track.
   track_ = video_track;
   cached_track_enabled_ = track_->enabled();
   track_->RegisterObserver(this);
-  Reconfigure();
+
+  if (ssrc_) {
+    VideoSourceInterface* source = track_->GetSource();
+    // TODO(deadbeef): If SetTrack is called with a disabled track, and the
+    // previous track was enabled, this could cause a frame from the new track
+    // to slip out. Really, what we need is for SetCaptureDevice and
+    // SetVideoSend
+    // to be combined into one atomic operation, all the way down to
+    // WebRtcVideoSendStream.
+    provider_->SetCaptureDevice(ssrc_,
+                                source ? source->GetVideoCapturer() : nullptr);
+    SetVideoSend();
+  }
   return true;
 }
 
-void VideoRtpSender::Stop() {
-  // TODO(deadbeef): Need to do more here to fully stop sending packets.
-  if (!provider_) {
+void VideoRtpSender::SetSsrc(uint32_t ssrc) {

[pthatcher1, 2015/10/20 17:42:49]

Similarly as with audio: would it simplify things to allow only setting the SSRC
once and not unsetting it?

+  if (stopped_ || ssrc == ssrc_) {
     return;
   }
-  provider_->SetCaptureDevice(ssrc_, nullptr);
-  provider_->SetVideoSend(ssrc_, false, nullptr);
-  provider_ = nullptr;
+  // If we are already sending with a particular SSRC, stop sending.
+  if (track_ && ssrc_) {
+    provider_->SetCaptureDevice(ssrc_, nullptr);
+    provider_->SetVideoSend(ssrc_, false, nullptr);
+  }
+  ssrc_ = ssrc;
+  if (track_ && ssrc_) {
+    VideoSourceInterface* source = track_->GetSource();
+    provider_->SetCaptureDevice(ssrc_,
+                                source ? source->GetVideoCapturer() : nullptr);
+    SetVideoSend();
+  }
 }
 
-void VideoRtpSender::Reconfigure() {
-  if (!provider_) {
+void VideoRtpSender::Stop() {
+  // TODO(deadbeef): Need to do more here to fully stop sending packets.
+  if (stopped_) {
     return;
   }
+  if (track_) {
+    track_->UnregisterObserver(this);
+    if (ssrc_) {
+      provider_->SetCaptureDevice(ssrc_, nullptr);
+      provider_->SetVideoSend(ssrc_, false, nullptr);
+    }

[pthatcher1, 2015/10/20 17:42:49]

What if we have an ssrc_ but no track_?  Would we still want to call these
provider_ methods?

[Taylor Brandstetter, 2015/10/21 00:22:08]

This is inside "if (track_)" so it's safe. I rearranged the code a bit to make
this clearer though (adding a "ready_to_send()" method that returns "track_ &&
ssrc_).

[pthatcher1, 2015/10/22 07:34:14]

Looks much better.

+  }
+  stopped_ = true;
+}
+
+void VideoRtpSender::SetVideoSend() {
+  RTC_DCHECK(!stopped_);
   const cricket::VideoOptions* options = nullptr;
   VideoSourceInterface* source = track_->GetSource();
   if (track_->enabled() && source) {