Adding the ability to create an RtpSender without a track.

talk/app/webrtc/rtpsender.cc

 /*
  * libjingle
  * Copyright 2015 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 11 matching lines @@
  * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
  * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "talk/app/webrtc/rtpsender.h"
 
 #include "talk/app/webrtc/localaudiosource.h"
 #include "talk/app/webrtc/videosourceinterface.h"
+#include "webrtc/base/helpers.h"
 
 namespace webrtc {
 
 LocalAudioSinkAdapter::LocalAudioSinkAdapter() : sink_(nullptr) {}
 
 LocalAudioSinkAdapter::~LocalAudioSinkAdapter() {
   rtc::CritScope lock(&lock_);
   if (sink_)
     sink_->OnClose();
 }
@@ skipping 10 matching lines @@
   }
 }
 
 void LocalAudioSinkAdapter::SetSink(cricket::AudioRenderer::Sink* sink) {
   rtc::CritScope lock(&lock_);
   ASSERT(!sink || !sink_);
   sink_ = sink;
 }
 
 AudioRtpSender::AudioRtpSender(AudioTrackInterface* track,
-                               uint32_t ssrc,
-                               AudioProviderInterface* provider)
+                               const std::string& stream_id,
+                               AudioProviderInterface* provider,
+                               StatsCollector* stats)
     : id_(track->id()),
+      stream_id_(stream_id),
+      provider_(provider),
+      stats_(stats),
       track_(track),
-      ssrc_(ssrc),
-      provider_(provider),
       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.";
     return false;
   }
   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::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;
+  }
+  // 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::Stop() {
   // TODO(deadbeef): Need to do more here to fully stop sending packets.
-  if (!provider_) {
+  if (stopped_) {
     return;
   }
-  cricket::AudioOptions options;
-  provider_->SetAudioSend(ssrc_, false, options, nullptr);
-  provider_ = nullptr;
+  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::Reconfigure() {
-  if (!provider_) {
-    return;
-  }
+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();
   }
 
   // Use the renderer if the audio track has one, otherwise use the sink
   // adapter owned by this class.
   cricket::AudioRenderer* renderer =
       track_->GetRenderer() ? track_->GetRenderer() : sink_adapter_.get();
   ASSERT(renderer != nullptr);
   provider_->SetAudioSend(ssrc_, track_->enabled(), options, renderer);
 }
 
 VideoRtpSender::VideoRtpSender(VideoTrackInterface* track,
-                               uint32_t ssrc,
+                               const std::string& stream_id,
                                VideoProviderInterface* provider)
     : id_(track->id()),
+      stream_id_(stream_id),
+      provider_(provider),
       track_(track),
-      ssrc_(ssrc),
-      provider_(provider),
       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.";
     return false;
   }
   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::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;
+  }
+  // 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::Stop() {
   // TODO(deadbeef): Need to do more here to fully stop sending packets.
-  if (!provider_) {
+  if (stopped_) {
     return;
   }
-  provider_->SetCaptureDevice(ssrc_, nullptr);
-  provider_->SetVideoSend(ssrc_, false, nullptr);
-  provider_ = nullptr;
+  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::Reconfigure() {
-  if (!provider_) {
-    return;
-  }
+void VideoRtpSender::SetVideoSend() {
+  RTC_DCHECK(!stopped_);
   const cricket::VideoOptions* options = nullptr;
   VideoSourceInterface* source = track_->GetSource();
   if (track_->enabled() && source) {
     options = source->options();
   }
   provider_->SetVideoSend(ssrc_, track_->enabled(), options);
 }
 
 }  // namespace webrtc