Adding AddTrack/RemoveTrack to native PeerConnection API.

talk/app/webrtc/peerconnection.cc

Index: talk/app/webrtc/peerconnection.cc
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index 617eb15518a18bb77730245db63b310ed591cb48..a5a895266741e83563edfb5beef953d17ab5e6cd 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -791,6 +791,72 @@ void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) {
   observer_->OnRenegotiationNeeded();
 }
 
+rtc::scoped_refptr<RtpSenderInterface> PeerConnection::AddTrack(
+    MediaStreamTrackInterface* track,
+    std::vector<MediaStreamInterface*> streams) {

[pthatcher1, 2016/01/11 17:31:51]

Would it make sense to only pass in a std::vector<std::string> with just stream
IDs?  I don't see any reason to take in streams.

Also, should we just return an error/null if 2 or more are passed in?  That's
probably better than silently breaking.

[Taylor Brandstetter, 2016/01/13 00:53:01]

The reason for it to take a stream is that I want this API to match the W3C API
as closely as possible. Maybe right *now* the only thing from the stream used is
its label, but if that changes in the future, I don't want to have to refactor
things all the way up to Chromium to change the type of the argument.

I do agree about returning an error if 2 are passed in, though. That would match
what we do about other things that are in the spec but not supported by us.

+  TRACE_EVENT0("webrtc", "PeerConnection::AddTrack");
+  if (IsClosed()) {
+    return nullptr;
+  }
+  if (FindSenderForTrack(track) != senders_.end()) {
+    LOG(LS_ERROR) << "Sender for track " << track->id() << " already exists.";
+    return nullptr;

[pthatcher1, 2016/01/11 17:31:51]

Will this be supported once we switch to Unified Plan?  It's a silly thing to
do, but I don't think it's disallowed by the standard.  Thus, should we have a
TODO?

[Taylor Brandstetter, 2016/01/13 00:53:01]

I guess I just assumed the spec didn't allow it. I'll add a TODO.

+  }
+
+  // TODO(deadbeef): Support adding a track to multiple streams.
+  rtc::scoped_refptr<RtpSenderInterface> new_sender;
+  if (track->kind() == MediaStreamTrackInterface::kAudioKind) {
+    new_sender = RtpSenderProxy::Create(
+        signaling_thread(),
+        new AudioRtpSender(static_cast<AudioTrackInterface*>(track),
+                           session_.get(), stats_.get()));

[pthatcher1, 2016/01/11 17:31:51]

So later one when we want to do something with an RtpSender within
PeerConnection, do we have to go through the RtpSenderProxy also?  Or will we
store both proxied and non-proxied?

[Taylor Brandstetter, 2016/01/13 00:53:01]

We would use the proxy object. If a method is called while already on the
signaling thread, the proxy will just pass through to the real object, so there
isn't much disadvantage. Sure, it requires a thread check and adds one method to
the call stack, but I think going to extra effort to avoid those things would
qualify as "premature optimization".

+    if (!streams.empty()) {
+      new_sender->set_stream_id(streams[0]->label());
+    }
+    const TrackInfo* track_info = FindTrackInfo(
+        local_audio_tracks_, new_sender->stream_id(), track->id());
+    if (track_info) {
+      new_sender->SetSsrc(track_info->ssrc);
+    }
+  } else if (track->kind() == MediaStreamTrackInterface::kVideoKind) {
+    new_sender = RtpSenderProxy::Create(
+        signaling_thread(),
+        new VideoRtpSender(static_cast<VideoTrackInterface*>(track),
+                           session_.get()));
+    if (!streams.empty()) {
+      new_sender->set_stream_id(streams[0]->label());
+    }
+    const TrackInfo* track_info = FindTrackInfo(
+        local_video_tracks_, new_sender->stream_id(), track->id());
+    if (track_info) {
+      new_sender->SetSsrc(track_info->ssrc);
+    }
+  } else {
+    LOG(LS_ERROR) << "CreateSender called with invalid kind: " << track->kind();
+    return rtc::scoped_refptr<RtpSenderInterface>();
+  }
+
+  senders_.push_back(new_sender);
+  observer_->OnRenegotiationNeeded();
+  return new_sender;
+}
+
+bool PeerConnection::RemoveTrack(RtpSenderInterface* sender) {
+  TRACE_EVENT0("webrtc", "PeerConnection::RemoveTrack");
+
+  auto it = std::find(senders_.begin(), senders_.end(), sender);
+  if (it == senders_.end()) {
+    LOG(LS_ERROR) << "Couldn't find sender " << sender->id() << " to remove.";
+    return false;
+  }
+  senders_.erase(it);
+
+  if (!IsClosed()) {
+    observer_->OnRenegotiationNeeded();
+  }

[pthatcher1, 2016/01/11 17:31:51]

We can RemoveTrack even if IsClosed()?

[Taylor Brandstetter, 2016/01/13 00:53:01]

I was just matching RemoveStream, but it looks like the spec explicitly
disallows this. I'll change it.

+  return true;
+}
+
 rtc::scoped_refptr<DtmfSenderInterface> PeerConnection::CreateDtmfSender(
     AudioTrackInterface* track) {
   TRACE_EVENT0("webrtc", "PeerConnection::CreateDtmfSender");
@@ -816,39 +882,32 @@ rtc::scoped_refptr<RtpSenderInterface> PeerConnection::CreateSender(
     const std::string& kind,
     const std::string& stream_id) {
   TRACE_EVENT0("webrtc", "PeerConnection::CreateSender");
-  RtpSenderInterface* new_sender;
+  rtc::scoped_refptr<RtpSenderInterface> new_sender;
   if (kind == MediaStreamTrackInterface::kAudioKind) {
-    new_sender = new AudioRtpSender(session_.get(), stats_.get());
+    new_sender = RtpSenderProxy::Create(
+        signaling_thread(), new AudioRtpSender(session_.get(), stats_.get()));
   } else if (kind == MediaStreamTrackInterface::kVideoKind) {
-    new_sender = new VideoRtpSender(session_.get());
+    new_sender = RtpSenderProxy::Create(signaling_thread(),
+                                        new VideoRtpSender(session_.get()));
   } else {
     LOG(LS_ERROR) << "CreateSender called with invalid kind: " << kind;
-    return rtc::scoped_refptr<RtpSenderInterface>();
+    return new_sender;
   }
   if (!stream_id.empty()) {
     new_sender->set_stream_id(stream_id);
   }
   senders_.push_back(new_sender);
-  return RtpSenderProxy::Create(signaling_thread(), new_sender);
+  return 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;
+  return senders_;
 }
 
 std::vector<rtc::scoped_refptr<RtpReceiverInterface>>
 PeerConnection::GetReceivers() const {
-  std::vector<rtc::scoped_refptr<RtpReceiverInterface>> receivers;
-  for (const auto& receiver : receivers_) {
-    receivers.push_back(
-        RtpReceiverProxy::Create(signaling_thread(), receiver.get()));
-  }
-  return receivers;
+  return receivers_;
 }
 
 bool PeerConnection::GetStats(StatsObserver* observer,
@@ -1318,13 +1377,17 @@ void PeerConnection::OnMessage(rtc::Message* msg) {
 void PeerConnection::CreateAudioReceiver(MediaStreamInterface* stream,
                                          AudioTrackInterface* audio_track,
                                          uint32_t ssrc) {
-  receivers_.push_back(new AudioRtpReceiver(audio_track, ssrc, session_.get()));
+  receivers_.push_back(RtpReceiverProxy::Create(
+      signaling_thread(),
+      new AudioRtpReceiver(audio_track, ssrc, session_.get())));
 }
 
 void PeerConnection::CreateVideoReceiver(MediaStreamInterface* stream,
                                          VideoTrackInterface* video_track,
                                          uint32_t ssrc) {
-  receivers_.push_back(new VideoRtpReceiver(video_track, ssrc, session_.get()));
+  receivers_.push_back(RtpReceiverProxy::Create(
+      signaling_thread(),
+      new VideoRtpReceiver(video_track, ssrc, session_.get())));
 }
 
 // TODO(deadbeef): Keep RtpReceivers around even if track goes away in remote
@@ -1416,8 +1479,9 @@ void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track,
   }
 
   // Normal case; we've never seen this track before.
-  AudioRtpSender* new_sender =
-      new AudioRtpSender(track, stream->label(), session_.get(), stats_.get());
+  rtc::scoped_refptr<RtpSenderInterface> new_sender = RtpSenderProxy::Create(
+      signaling_thread(),
+      new AudioRtpSender(track, stream->label(), session_.get(), stats_.get()));
   senders_.push_back(new_sender);
   // If the sender has already been configured in SDP, we call SetSsrc,
   // which will connect the sender to the underlying transport. This can
@@ -1457,8 +1521,9 @@ void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
   }
 
   // Normal case; we've never seen this track before.
-  VideoRtpSender* new_sender =
-      new VideoRtpSender(track, stream->label(), session_.get());
+  rtc::scoped_refptr<RtpSenderInterface> new_sender = RtpSenderProxy::Create(
+      signaling_thread(),
+      new VideoRtpSender(track, stream->label(), session_.get()));
   senders_.push_back(new_sender);
   const TrackInfo* track_info =
       FindTrackInfo(local_video_tracks_, stream->label(), track->id());