Adding the ability to create an RtpSender without a track.

talk/app/webrtc/peerconnection.cc

 /*
  * libjingle
  * Copyright 2012 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 41 matching lines @@
 #include "webrtc/base/stringencode.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/system_wrappers/interface/field_trial.h"
 
 namespace {
 
 using webrtc::DataChannel;
 using webrtc::MediaConstraintsInterface;
 using webrtc::MediaStreamInterface;
 using webrtc::PeerConnectionInterface;
+using webrtc::RtpSenderInterface;
 using webrtc::StreamCollection;
 using webrtc::StunConfigurations;
 using webrtc::TurnConfigurations;
 typedef webrtc::PortAllocatorFactoryInterface::StunConfiguration
     StunConfiguration;
 typedef webrtc::PortAllocatorFactoryInterface::TurnConfiguration
     TurnConfiguration;
 
 static const char kDefaultStreamLabel[] = "default";
 static const char kDefaultAudioTrackLabel[] = "defaulta0";
@@ skipping 286 matching lines @@
              : std::vector<cricket::StreamParams>();
 }
 
 bool IsValidOfferToReceiveMedia(int value) {
   typedef PeerConnectionInterface::RTCOfferAnswerOptions Options;
   return (value >= Options::kUndefined) &&
          (value <= Options::kMaxOfferToReceiveMedia);
 }
 
 // Add the stream and RTP data channel info to |session_options|.
-void SetStreams(cricket::MediaSessionOptions* session_options,
-                rtc::scoped_refptr<StreamCollection> streams,
-                const std::map<std::string, rtc::scoped_refptr<DataChannel>>&
-                    rtp_data_channels) {
+void AddSendStreams(
+    cricket::MediaSessionOptions* session_options,
+    const std::vector<rtc::scoped_refptr<RtpSenderInterface>>& senders,
+    const std::map<std::string, rtc::scoped_refptr<DataChannel>>&
+        rtp_data_channels) {
   session_options->streams.clear();
-  if (streams != nullptr) {
-    for (size_t i = 0; i < streams->count(); ++i) {
-      MediaStreamInterface* stream = streams->at(i);
-      // For each audio track in the stream, add it to the MediaSessionOptions.
-      for (const auto& track : stream->GetAudioTracks()) {
-        session_options->AddSendStream(cricket::MEDIA_TYPE_AUDIO, track->id(),
-                                       stream->label());
-      }
-      // For each video track in the stream, add it to the MediaSessionOptions.
-      for (const auto& track : stream->GetVideoTracks()) {
-        session_options->AddSendStream(cricket::MEDIA_TYPE_VIDEO, track->id(),
-                                       stream->label());
-      }
+  for (const auto& sender : senders) {
+    // Add each sender to the MediaSessionOptions.
+    cricket::MediaType type;
+    if (sender->kind() == "audio") {

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

We should use a named constant for "audio" and "video".

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

Done.

+      type = cricket::MEDIA_TYPE_AUDIO;
+    } else if (sender->kind() == "video") {
+      type = cricket::MEDIA_TYPE_VIDEO;
+    } else {
+      RTC_DCHECK(false && "Invalid 'kind' for RtpSender.");
+      continue;
     }
+    session_options->AddSendStream(type, sender->id(), sender->stream_id());
   }
 
   // Check for data channels.
   for (const auto& kv : rtp_data_channels) {
     const DataChannel* channel = kv.second;
     if (channel->state() == DataChannel::kConnecting ||
         channel->state() == DataChannel::kOpen) {
       // |streamid| and |sync_label| are both set to the DataChannel label
       // here so they can be signaled the same way as MediaStreams and Tracks.
       // For MediaStreams, the sync_label is the MediaStream label and the
@@ skipping 279 matching lines @@
 rtc::scoped_refptr<StreamCollectionInterface>
 PeerConnection::local_streams() {
   return local_streams_;
 }
 
 rtc::scoped_refptr<StreamCollectionInterface>
 PeerConnection::remote_streams() {
   return remote_streams_;
 }
 
-// TODO(deadbeef): Create RtpSenders immediately here, even if local
-// description hasn't yet been set.
 bool PeerConnection::AddStream(MediaStreamInterface* local_stream) {
   if (IsClosed()) {
     return false;
   }
   if (!CanAddLocalMediaStream(local_streams_, local_stream)) {
     return false;
   }
 
   local_streams_->AddStream(local_stream);
 
-  // Find tracks that have already been configured in SDP. This can occur if a
-  // local session description that contains the MSID of these tracks is set
-  // before AddLocalStream is called. It can also occur if the local session
-  // description is not changed and RemoveLocalStream  is called and later
-  // AddLocalStream is called again with the same stream.
+  // Note: If the sender has already been configured in SDP, we call SetSsrc,
+  // which will connect the sender to the underlying transport. This can occur
+  // if a local session description that contains the ID of the sender is set
+  // before AddStream is called. It can also occur if the local session
+  // description is not changed and RemoveStream is called, and later
+  // AddStream is called again with the same stream.

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

I'm a little lost as to how this comment matches the code.  The code seems to
say "If there's already a sender for this track, call set_stream_id;  otherwise,
create one and call SetSsrc".  But the comment says "If there's already a sender
for this track, call SetSsrc".  

This comment almost seems to match code in OnLocalTrackSeen.

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

"Already configured in SDP" isn't the same as "already have a sender". This
comment is referring to the "FindTrackInfo"/"if (track_info)" part of the code.
I guess I'll move the comment down there so this is more obvious.

   for (const auto& track : local_stream->GetAudioTracks()) {
-    const TrackInfo* track_info =
-        FindTrackInfo(local_audio_tracks_, local_stream->label(), track->id());
-    if (track_info) {
-      CreateAudioSender(local_stream, track.get(), track_info->ssrc);
+    auto sender = FindSenderForTrack(track.get());
+    if (sender == senders_.end()) {
+      // Normal case; we've never seen this track before.
+      AudioRtpSender* new_sender = new AudioRtpSender(
+          track.get(), local_stream->label(), session_.get(), stats_.get());
+      senders_.push_back(new_sender);
+      const TrackInfo* track_info = FindTrackInfo(
+          local_audio_tracks_, local_stream->label(), track->id());
+      if (track_info) {
+        new_sender->SetSsrc(track_info->ssrc);
+      }
+    } else {
+      // We already have a sender for this track, so just change the stream_id
+      // so that it's correct in the next call to CreateOffer.
+      (*sender)->set_stream_id(local_stream->label());

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

Should this method be set_stream_label?

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

Since in the W3C spec it's called an "id", I figured it's best to use that
terminology as often as possible.

     }
   }
   for (const auto& track : local_stream->GetVideoTracks()) {
-    const TrackInfo* track_info =
-        FindTrackInfo(local_video_tracks_, local_stream->label(), track->id());
-    if (track_info) {
-      CreateVideoSender(local_stream, track.get(), track_info->ssrc);
+    auto sender = FindSenderForTrack(track.get());
+    if (sender == senders_.end()) {
+      // Normal case; we've never seen this track before.
+      VideoRtpSender* new_sender = new VideoRtpSender(
+          track.get(), local_stream->label(), session_.get());
+      senders_.push_back(new_sender);
+      const TrackInfo* track_info = FindTrackInfo(
+          local_video_tracks_, local_stream->label(), track->id());
+      if (track_info) {
+        new_sender->SetSsrc(track_info->ssrc);
+      }
+    } else {
+      // We already have a sender for this track, so just change the stream_id
+      // so that it's correct in the next call to CreateOffer.
+      (*sender)->set_stream_id(local_stream->label());
     }
   }
 
   stats_->AddStream(local_stream);
   observer_->OnRenegotiationNeeded();
   return true;
 }
 
 // TODO(deadbeef): Don't destroy RtpSenders here; they should be kept around
-// indefinitely.
+// indefinitely, when we have unified plan SDP.
 void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) {
   for (const auto& track : local_stream->GetAudioTracks()) {
-    const TrackInfo* track_info =
-        FindTrackInfo(local_audio_tracks_, local_stream->label(), track->id());
-    if (track_info) {
-      DestroyAudioSender(local_stream, track.get(), track_info->ssrc);
+    auto sender = FindSenderForTrack(track.get());
+    if (sender == senders_.end()) {
+      LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+                      << " doesn't exist.";
+      continue;
     }
+    (*sender)->Stop();
+    senders_.erase(sender);
   }
   for (const auto& track : local_stream->GetVideoTracks()) {
-    const TrackInfo* track_info =
-        FindTrackInfo(local_video_tracks_, local_stream->label(), track->id());
-    if (track_info) {
-      DestroyVideoSender(local_stream, track.get());
+    auto sender = FindSenderForTrack(track.get());
+    if (sender == senders_.end()) {
+      LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+                      << " doesn't exist.";
+      continue;
     }
+    (*sender)->Stop();
+    senders_.erase(sender);
   }
 
   local_streams_->RemoveStream(local_stream);
 
   if (IsClosed()) {
     return;
   }
   observer_->OnRenegotiationNeeded();
 }
 
@@ skipping 10 matching lines @@
 
   rtc::scoped_refptr<DtmfSenderInterface> sender(
       DtmfSender::Create(track, signaling_thread(), session_.get()));
   if (!sender.get()) {
     LOG(LS_ERROR) << "CreateDtmfSender failed on DtmfSender::Create.";
     return NULL;
   }
   return DtmfSenderProxy::Create(signaling_thread(), sender.get());
 }
 
+rtc::scoped_refptr<RtpSenderInterface> PeerConnection::CreateSender(
+    const std::string& kind) {
+  RtpSenderInterface* new_sender;
+  if (kind == "audio") {

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

Same here with named constants.

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

Done.

+    new_sender = new AudioRtpSender(session_.get(), stats_.get());
+  } else if (kind == "video") {
+    new_sender = new VideoRtpSender(session_.get());
+  } else {
+    return rtc::scoped_refptr<RtpSenderInterface>();
+  }
+  senders_.push_back(new_sender);
+  return RtpSenderProxy::Create(signaling_thread(), new_sender);
+}
+
 std::vector<rtc::scoped_refptr<RtpSenderInterface>> PeerConnection::GetSenders()
     const {
   std::vector<rtc::scoped_refptr<RtpSenderInterface>> senders;
   for (const auto& sender : senders_) {
     senders.push_back(RtpSenderProxy::Create(signaling_thread(), sender.get()));
   }
   return senders;
 }
 
 std::vector<rtc::scoped_refptr<RtpReceiverInterface>>
@@ skipping 496 matching lines @@
   auto it = FindReceiverForTrack(video_track);
   if (it == receivers_.end()) {
     LOG(LS_WARNING) << "RtpReceiver for track with id " << video_track->id()
                     << " doesn't exist.";
   } else {
     (*it)->Stop();
     receivers_.erase(it);
   }
 }
 
-void PeerConnection::CreateAudioSender(MediaStreamInterface* stream,
-                                       AudioTrackInterface* audio_track,
-                                       uint32_t ssrc) {
-  senders_.push_back(new AudioRtpSender(audio_track, ssrc, session_.get()));
-  stats_->AddLocalAudioTrack(audio_track, ssrc);
-}
-
-void PeerConnection::CreateVideoSender(MediaStreamInterface* stream,
-                                       VideoTrackInterface* video_track,
-                                       uint32_t ssrc) {
-  senders_.push_back(new VideoRtpSender(video_track, ssrc, session_.get()));
-}
-
-// TODO(deadbeef): Keep RtpSenders around even if track goes away in local
-// description.
-void PeerConnection::DestroyAudioSender(MediaStreamInterface* stream,
-                                        AudioTrackInterface* audio_track,
-                                        uint32_t ssrc) {
-  auto it = FindSenderForTrack(audio_track);
-  if (it == senders_.end()) {
-    LOG(LS_WARNING) << "RtpSender for track with id " << audio_track->id()
-                    << " doesn't exist.";
-    return;
-  } else {
-    (*it)->Stop();
-    senders_.erase(it);
-  }
-  stats_->RemoveLocalAudioTrack(audio_track, ssrc);
-}
-
-void PeerConnection::DestroyVideoSender(MediaStreamInterface* stream,
-                                        VideoTrackInterface* video_track) {
-  auto it = FindSenderForTrack(video_track);
-  if (it == senders_.end()) {
-    LOG(LS_WARNING) << "RtpSender for track with id " << video_track->id()
-                    << " doesn't exist.";
-    return;
-  } else {
-    (*it)->Stop();
-    senders_.erase(it);
-  }
-}
-
 void PeerConnection::OnIceConnectionChange(
     PeerConnectionInterface::IceConnectionState new_state) {
   RTC_DCHECK(signaling_thread()->IsCurrent());
   // After transitioning to "closed", ignore any additional states from
   // WebRtcSession (such as "disconnected").
   if (IsClosed()) {
     return;
   }
   ice_connection_state_ = new_state;
   observer_->OnIceConnectionChange(ice_connection_state_);
@@ skipping 51 matching lines @@
     CreateSessionDescriptionObserver* observer,
     const std::string& error) {
   CreateSessionDescriptionMsg* msg = new CreateSessionDescriptionMsg(observer);
   msg->error = error;
   signaling_thread()->Post(this, MSG_CREATE_SESSIONDESCRIPTION_FAILED, msg);
 }
 
 bool PeerConnection::GetOptionsForOffer(
     const PeerConnectionInterface::RTCOfferAnswerOptions& rtc_options,
     cricket::MediaSessionOptions* session_options) {
-  SetStreams(session_options, local_streams_, rtp_data_channels_);
+  AddSendStreams(session_options, senders_, rtp_data_channels_);
 
   if (!ConvertRtcOptionsForOffer(rtc_options, session_options)) {
     return false;
   }
 
   if (session_->data_channel_type() == cricket::DCT_SCTP && HasDataChannels()) {
     session_options->data_channel_type = cricket::DCT_SCTP;
   }
   return true;
 }
 
 bool PeerConnection::GetOptionsForAnswer(
     const MediaConstraintsInterface* constraints,
     cricket::MediaSessionOptions* session_options) {
-  SetStreams(session_options, local_streams_, rtp_data_channels_);
+  AddSendStreams(session_options, senders_, rtp_data_channels_);
   session_options->recv_audio = false;
   session_options->recv_video = false;
 
   if (!ParseConstraintsForAnswer(constraints, session_options)) {
     return false;
   }
 
   // RTP data channel is handled in MediaSessionOptions::AddStream. SCTP streams
   // are not signaled in the SDP so does not go through that path and must be
   // handled here.
   if (session_->data_channel_type() == cricket::DCT_SCTP) {
     session_options->data_channel_type = cricket::DCT_SCTP;
   }
   return true;
 }
 
 void PeerConnection::UpdateRemoteStreamsList(
     const cricket::StreamParamsVec& streams,
     cricket::MediaType media_type,
     StreamCollection* new_streams) {
   TrackInfos* current_tracks = GetRemoteTracks(media_type);
 
   // Find removed tracks. I.e., tracks where the track id or ssrc don't match
-  // the
-  // new StreamParam.
+  // the new StreamParam.
   auto track_it = current_tracks->begin();
   while (track_it != current_tracks->end()) {
     const TrackInfo& info = *track_it;
     const cricket::StreamParams* params =
         cricket::GetStreamBySsrc(streams, info.ssrc);
     if (!params || params->id != info.track_id) {
       OnRemoteTrackRemoved(info.stream_label, info.track_id, media_type);
       track_it = current_tracks->erase(track_it);
     } else {
       ++track_it;
@@ skipping 180 matching lines @@
     if (!track_info) {
       current_tracks->push_back(TrackInfo(stream_label, track_id, ssrc));
       OnLocalTrackSeen(stream_label, track_id, params.first_ssrc(), media_type);
     }
   }
 }
 
 void PeerConnection::OnLocalTrackSeen(const std::string& stream_label,
                                       const std::string& track_id,
                                       uint32_t ssrc,
                                       cricket::MediaType media_type) {

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

Should we call this OnSenderAddedToSessionDescription?

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

Well, then it would be inconsistent with OnRemoteTrackSeen... And it IS called
when a new track (TrackInfo structure) is created. I plan to clean the TrackInfo
stuff up in a future CL though; we really have no reason to be caching something
that's already readily available in the local description.

-  MediaStreamInterface* stream = local_streams_->find(stream_label);
-  if (!stream) {
-    LOG(LS_WARNING) << "An unknown local MediaStream with label "
-                    << stream_label << " has been configured.";
+  RtpSenderInterface* sender = FindSenderById(track_id);

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

Should we match the media_type/kind when finding senders as well as ID?

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

Then we wouldn't be able to tell "unknown sender" apart from "invalid media
type" at this level. Plus, with the change to using an enum for "kind", it
wouldn't save many lines of code anyway.

+  if (!sender) {
+    LOG(LS_WARNING) << "An unknown RtpSender with id " << track_id
+                    << " has been configured in the local description.";
     return;
   }
 
-  if (media_type == cricket::MEDIA_TYPE_AUDIO) {
-    AudioTrackInterface* audio_track = stream->FindAudioTrack(track_id);
-    if (!audio_track) {
-      LOG(LS_WARNING) << "An unknown local AudioTrack with id , " << track_id
-                      << " has been configured.";
-      return;
-    }
-    CreateAudioSender(stream, audio_track, ssrc);
-  } else if (media_type == cricket::MEDIA_TYPE_VIDEO) {
-    VideoTrackInterface* video_track = stream->FindVideoTrack(track_id);
-    if (!video_track) {
-      LOG(LS_WARNING) << "An unknown local VideoTrack with id , " << track_id
-                      << " has been configured.";
-      return;
-    }
-    CreateVideoSender(stream, video_track, ssrc);
-  } else {
-    RTC_DCHECK(false && "Invalid media type");
+  if (!((media_type == cricket::MEDIA_TYPE_AUDIO &&
+         sender->kind() == "audio") ||
+        (media_type == cricket::MEDIA_TYPE_VIDEO &&
+         sender->kind() == "video"))) {
+    LOG(LS_WARNING) << "An RtpSender has been configured in the local"
+                    << " description with an unexpected media type.";
+    RTC_DCHECK(false && "Invalid media type.");

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

This is an error from the application or the javascript (mismatch of SDP with
track), possibly from SDP munging.  It seems bad to cause a DCHECK in that case.
 I think an LS_WARNING is sufficient.

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

DCHECKS only do anything in a debug builds (hence the 'D').

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

But do we still want to allow bad JS to crash the tab, even in debug build?   

[Taylor Brandstetter, 2015/10/22 19:26:42]

Oh, that's a good point. Ok, it's removed.

+    return;
   }
+
+  sender->set_stream_id(stream_label);
+  sender->SetSsrc(ssrc);
 }
 
 void PeerConnection::OnLocalTrackRemoved(const std::string& stream_label,
                                          const std::string& track_id,
                                          uint32_t ssrc,
                                          cricket::MediaType media_type) {

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

Should we call this OnSenderRemovedFromSessionDescription?

-  MediaStreamInterface* stream = local_streams_->find(stream_label);
-  if (!stream) {
-    // This is the normal case. I.e., RemoveLocalStream has been called and the
+  RtpSenderInterface* sender = FindSenderById(track_id);
+  if (!sender) {
+    // This is the normal case. I.e., RemoveStream has been called and the
     // SessionDescriptions has been renegotiated.
     return;
   }
-  // A track has been removed from the SessionDescription but the MediaStream
-  // is still associated with PeerConnection. This only occurs if the SDP
-  // doesn't match with the calls to AddLocalStream and RemoveLocalStream.
-  if (media_type == cricket::MEDIA_TYPE_AUDIO) {
-    AudioTrackInterface* audio_track = stream->FindAudioTrack(track_id);
-    if (!audio_track) {
-      return;
-    }
-    DestroyAudioSender(stream, audio_track, ssrc);
-  } else if (media_type == cricket::MEDIA_TYPE_VIDEO) {
-    VideoTrackInterface* video_track = stream->FindVideoTrack(track_id);
-    if (!video_track) {
-      return;
-    }
-    DestroyVideoSender(stream, video_track);
-  } else {
+
+  // A sender has been removed from the SessionDescription but it's still
+  // associated with the PeerConnection. This only occurs if the SDP doesn't
+  // match with the calls to CreateSender, AddStream and RemoveStream.
+  if (!((media_type == cricket::MEDIA_TYPE_AUDIO &&
+         sender->kind() == "audio") ||

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

Can we change the sender->kind() to be a cricket::MediaType?  It would be easier
to compare them.

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

Done.

+        (media_type == cricket::MEDIA_TYPE_VIDEO &&
+         sender->kind() == "video"))) {
+    LOG(LS_WARNING) << "An RtpSender has been configured in the local"
+                    << " description with an unexpected media type.";
     RTC_DCHECK(false && "Invalid media type.");
+    return;
   }
+
+  sender->SetSsrc(0);
 }
 
 void PeerConnection::UpdateLocalRtpDataChannels(
     const cricket::StreamParamsVec& streams) {
   std::vector<std::string> existing_channels;
 
   // Find new and active data channels.
   for (const cricket::StreamParams& params : streams) {
     // |it->sync_label| is actually the data channel label. The reason is that
     // we use the same naming of data channels as we do for
@@ skipping 197 matching lines @@
       InternalCreateDataChannel(label, &config));
   if (!channel.get()) {
     LOG(LS_ERROR) << "Failed to create DataChannel from the OPEN message.";
     return;
   }
 
   observer_->OnDataChannel(
       DataChannelProxy::Create(signaling_thread(), channel));
 }
 
+RtpSenderInterface* PeerConnection::FindSenderById(const std::string& id) {
+  auto it =
+      std::find_if(senders_.begin(), senders_.end(),
+                   [id](const rtc::scoped_refptr<RtpSenderInterface>& sender) {
+                     return sender->id() == id;
+                   });
+  return it != senders_.end() ? it->get() : nullptr;
+}
+
 std::vector<rtc::scoped_refptr<RtpSenderInterface>>::iterator
 PeerConnection::FindSenderForTrack(MediaStreamTrackInterface* track) {
   return std::find_if(
       senders_.begin(), senders_.end(),
       [track](const rtc::scoped_refptr<RtpSenderInterface>& sender) {
         return sender->track() == track;
       });
 }
 
 std::vector<rtc::scoped_refptr<RtpReceiverInterface>>::iterator
@@ skipping 37 matching lines @@
 DataChannel* PeerConnection::FindDataChannelBySid(int sid) const {
   for (const auto& channel : sctp_data_channels_) {
     if (channel->id() == sid) {
       return channel;
     }
   }
   return nullptr;
 }
 
 }  // namespace webrtc