Bug 1136871 - Part 2: Clear up some inconsistencies with ReplaceTrack r=jib,smaug

dom/media/PeerConnection.js

@@ -862,7 +862,7 @@ RTCPeerConnection.prototype = {
       this._onReplaceTrackWithTrack = withTrack;
       this._onReplaceTrackSuccess = resolve;
       this._onReplaceTrackFailure = reject;
-      this._impl.replaceTrack(sender.track, withTrack, sender._stream);
+      this._impl.replaceTrack(sender.track, withTrack);
     });
   },
 

dom/webidl/PeerConnectionImpl.webidl

@@ -44,8 +44,7 @@ interface PeerConnectionImpl  {
   [Throws]
   void removeTrack(MediaStreamTrack track);
   [Throws]
-  void replaceTrack(MediaStreamTrack thisTrack, MediaStreamTrack withTrack,
-                    MediaStream stream);
+  void replaceTrack(MediaStreamTrack thisTrack, MediaStreamTrack withTrack);
   [Throws]
   void closeStreams();
 

media/webrtc/signaling/src/jsep/JsepSession.h

@@ -93,8 +93,10 @@ public:
   virtual nsresult AddTrack(const RefPtr<JsepTrack>& track) = 0;
   virtual nsresult RemoveTrack(const std::string& streamId,
                                const std::string& trackId) = 0;
-  virtual nsresult ReplaceTrack(size_t track_index,
-                                const RefPtr<JsepTrack>& track) = 0;
+  virtual nsresult ReplaceTrack(const std::string& oldStreamId,
+                                const std::string& oldTrackId,
+                                const std::string& newStreamId,
+                                const std::string& newTrackId) = 0;
 
   virtual std::vector<RefPtr<JsepTrack>> GetLocalTracks() const = 0;
 

media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp

@@ -233,6 +233,33 @@ GetTracks(const std::vector<T>& wrappedTracks)
   return result;
 }
 
+nsresult
+JsepSessionImpl::ReplaceTrack(const std::string& oldStreamId,
+                              const std::string& oldTrackId,
+                              const std::string& newStreamId,
+                              const std::string& newTrackId)
+{
+  auto it = FindTrackByIds(mLocalTracks, oldStreamId, oldTrackId);
+
+  if (it == mLocalTracks.end()) {
+    JSEP_SET_ERROR("Track " << oldStreamId << "/" << oldTrackId
+                   << " was never added.");
+    return NS_ERROR_INVALID_ARG;
+  }
+
+  if (FindTrackByIds(mLocalTracks, newStreamId, newTrackId) !=
+      mLocalTracks.end()) {
+    JSEP_SET_ERROR("Track " << newStreamId << "/" << newTrackId
+                   << " was already added.");
+    return NS_ERROR_INVALID_ARG;
+  }
+
+  it->mTrack->SetStreamId(newStreamId);
+  it->mTrack->SetTrackId(newTrackId);
+
+  return NS_OK;
+}
+
 std::vector<RefPtr<JsepTrack>>
 JsepSessionImpl::GetLocalTracks() const
 {

media/webrtc/signaling/src/jsep/JsepSessionImpl.h

@@ -79,12 +79,10 @@ public:
     return mCodecs;
   }
 
-  virtual nsresult
-  ReplaceTrack(size_t trackIndex, const RefPtr<JsepTrack>& track) MOZ_OVERRIDE
-  {
-    mLastError.clear();
-    MOZ_CRASH(); // Stub
-  }
+  virtual nsresult ReplaceTrack(const std::string& oldStreamId,
+                                const std::string& oldTrackId,
+                                const std::string& newStreamId,
+                                const std::string& newTrackId) MOZ_OVERRIDE;
 
   virtual std::vector<RefPtr<JsepTrack>> GetLocalTracks() const MOZ_OVERRIDE;
 

media/webrtc/signaling/src/jsep/JsepTrack.h

@@ -68,12 +68,24 @@ public:
     return mStreamId;
   }
 
+  virtual void
+  SetStreamId(const std::string& id)
+  {
+    mStreamId = id;
+  }
+
   virtual const std::string&
   GetTrackId() const
   {
     return mTrackId;
   }
 
+  virtual void
+  SetTrackId(const std::string& id)
+  {
+    mTrackId = id;
+  }
+
   virtual const std::string&
   GetCNAME() const
   {
@@ -137,8 +149,8 @@ protected:
 
 private:
   const mozilla::SdpMediaSection::MediaType mType;
-  const std::string mStreamId;
-  const std::string mTrackId;
+  std::string mStreamId;
+  std::string mTrackId;
   std::string mCNAME;
   const Direction mDirection;
   UniquePtr<JsepTrackNegotiatedDetails> mNegotiatedDetails;

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp

@@ -2086,8 +2086,7 @@ PeerConnectionImpl::RemoveTrack(MediaStreamTrack& aTrack) {
 
 NS_IMETHODIMP
 PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack,
-                                 MediaStreamTrack& aWithTrack,
-                                 DOMMediaStream& aStream) {
+                                 MediaStreamTrack& aWithTrack) {
   PC_AUTO_ENTER_API_CALL(true);
 
   JSErrorResult jrv;
@@ -2099,52 +2098,33 @@ PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack,
   std::string origTrackId = PeerConnectionImpl::GetTrackId(aThisTrack);
   std::string newTrackId = PeerConnectionImpl::GetTrackId(aWithTrack);
 
-  // TODO: Do an aStream.HasTrack() check on both track args someday.
-  //
-  // The proposed API will be that both tracks must already be in the same
-  // stream. However, since our MediaStreams currently are limited to one
-  // track per type, we allow replacement with an outside track not already
-  // in the same stream. This works because sync happens receiver-side and
-  // timestamps are tied to capture.
+  std::string origStreamId =
+    PeerConnectionImpl::GetStreamId(*aThisTrack.GetStream());
+  std::string newStreamId =
+    PeerConnectionImpl::GetStreamId(*aWithTrack.GetStream());
 
-  if (!aStream.HasTrack(aThisTrack)) {
-    CSFLogError(logTag, "Track to replace (%s) is not in stream",
-                        origTrackId.c_str());
-    pco->OnReplaceTrackError(kInvalidMediastreamTrack,
-                             ObString("Track to replace is not in stream"),
-                             jrv);
-    return NS_OK;
-  }
+  nsresult rv = mJsepSession->ReplaceTrack(origStreamId,
+                                           origTrackId,
+                                           newStreamId,
+                                           newTrackId);
 
-  // XXX This MUST be addressed when we add multiple tracks of a type!!
-  // This is needed because the track IDs used by MSG are from TrackUnion
-  // (for getUserMedia streams) and aren't the same as the values the source tracks
-  // have.  Solution is to have JsepSession read track ids and use those.
-
-  // Because DirectListeners see the SourceMediaStream's TrackID's, and not the
-  // TrackUnionStream's TrackID's, this value won't currently match what is used in
-  // MediaPipelineTransmit.  Bug 1056652
-  //  TrackID thisID = aThisTrack.GetTrackID();
-  //
-
-  std::string streamId = PeerConnectionImpl::GetStreamId(aStream);
-  nsRefPtr<LocalSourceStreamInfo> info = media()->GetLocalStreamById(streamId);
-
-  if (!info || !info->HasTrack(origTrackId)) {
-    CSFLogError(logTag, "Track to replace (%s) was never added",
-                        origTrackId.c_str());
-    pco->OnReplaceTrackError(kInvalidMediastreamTrack,
-                             ObString("Track to replace was never added"),
-                             jrv);
-    return NS_OK;
-  }
-
-  nsresult rv =
-    info->ReplaceTrack(origTrackId, aWithTrack.GetStream(), newTrackId);
   if (NS_FAILED(rv)) {
-    CSFLogError(logTag, "Failed to replace track (%s)",
-                        origTrackId.c_str());
-    pco->OnReplaceTrackError(kInternalError,
+    pco->OnReplaceTrackError(kInvalidMediastreamTrack,
+                             ObString(mJsepSession->GetLastError().c_str()),
+                             jrv);
+    return NS_OK;
+  }
+
+  rv = media()->ReplaceTrack(origStreamId,
+                             origTrackId,
+                             aWithTrack.GetStream(),
+                             newStreamId,
+                             newTrackId);
+
+  if (NS_FAILED(rv)) {
+    CSFLogError(logTag, "Unexpected error in ReplaceTrack: %d",
+                        static_cast<int>(rv));
+    pco->OnReplaceTrackError(kInvalidMediastreamTrack,
                              ObString("Failed to replace track"),
                              jrv);
     return NS_OK;

media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h

@@ -418,10 +418,9 @@ public:
 
   NS_IMETHODIMP_TO_ERRORRESULT(ReplaceTrack, ErrorResult &rv,
                                mozilla::dom::MediaStreamTrack& aThisTrack,
-                               mozilla::dom::MediaStreamTrack& aWithTrack,
-                               DOMMediaStream& aStream)
+                               mozilla::dom::MediaStreamTrack& aWithTrack)
   {
-    rv = ReplaceTrack(aThisTrack, aWithTrack, aStream);
+    rv = ReplaceTrack(aThisTrack, aWithTrack);
   }
 
   nsresult GetPeerIdentity(nsAString& peerIdentity)

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp

@@ -44,26 +44,35 @@ using namespace dom;
 
 static const char* logTag = "PeerConnectionMedia";
 
-nsresult LocalSourceStreamInfo::ReplaceTrack(const std::string& oldTrackId,
-                                             DOMMediaStream* aNewStream,
-                                             const std::string& newTrackId)
+nsresult
+PeerConnectionMedia::ReplaceTrack(const std::string& aOldStreamId,
+                                  const std::string& aOldTrackId,
+                                  DOMMediaStream* aNewStream,
+                                  const std::string& aNewStreamId,
+                                  const std::string& aNewTrackId)
 {
-  RefPtr<MediaPipeline> pipeline = mPipelines[oldTrackId];
+  RefPtr<LocalSourceStreamInfo> oldInfo(GetLocalStreamById(aOldStreamId));
 
-  if (!pipeline || !mTracks.count(oldTrackId)) {
-    CSFLogError(logTag, "Failed to find track id %s", oldTrackId.c_str());
+  if (!oldInfo) {
+    CSFLogError(logTag, "Failed to find stream id %s", aOldStreamId.c_str());
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  nsresult rv =
-    static_cast<MediaPipelineTransmit*>(pipeline.get())->ReplaceTrack(
-        aNewStream, newTrackId);
+  nsresult rv = AddTrack(aNewStream, aNewStreamId, aNewTrackId);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  mTracks.erase(oldTrackId);
-  mTracks.insert(newTrackId);
+  RefPtr<LocalSourceStreamInfo> newInfo(GetLocalStreamById(aNewStreamId));
 
-  return NS_OK;
+  if (!newInfo) {
+    CSFLogError(logTag, "Failed to add track id %s", aNewTrackId.c_str());
+    MOZ_ASSERT(false);
+    return NS_ERROR_FAILURE;
+  }
+
+  rv = newInfo->TakePipelineFrom(oldInfo, aOldTrackId, aNewTrackId);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return RemoveLocalTrack(aOldStreamId, aOldTrackId);
 }
 
 static void
@@ -977,6 +986,37 @@ PeerConnectionMedia::ConnectDtlsListener_s(const RefPtr<TransportFlow>& aFlow)
   }
 }
 
+nsresult
+LocalSourceStreamInfo::TakePipelineFrom(RefPtr<LocalSourceStreamInfo>& info,
+                                        const std::string& oldTrackId,
+                                        const std::string& newTrackId)
+{
+  if (mPipelines.count(newTrackId)) {
+    CSFLogError(logTag, "%s: Pipeline already exists for %s/%s",
+                __FUNCTION__, mId.c_str(), newTrackId.c_str());
+    return NS_ERROR_INVALID_ARG;
+  }
+
+  RefPtr<MediaPipeline> pipeline(info->ForgetPipelineByTrackId_m(oldTrackId));
+
+  if (!pipeline) {
+    // Replacetrack can potentially happen in the middle of offer/answer, before
+    // the pipeline has been created.
+    CSFLogInfo(logTag, "%s: Replacing track before the pipeline has been "
+                       "created, nothing to do.", __FUNCTION__);
+    return NS_OK;
+  }
+
+  nsresult rv =
+    static_cast<MediaPipelineTransmit*>(pipeline.get())->ReplaceTrack(
+        mMediaStream, newTrackId);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  mPipelines[newTrackId] = pipeline;
+
+  return NS_OK;
+}
+
 #ifdef MOZILLA_INTERNAL_API
 /**
  * Tells you if any local streams is isolated to a specific peer identity.
@@ -1153,4 +1193,25 @@ RefPtr<MediaPipeline> SourceStreamInfo::GetPipelineByTrackId_m(
   return nullptr;
 }
 
+TemporaryRef<MediaPipeline>
+LocalSourceStreamInfo::ForgetPipelineByTrackId_m(const std::string& trackId)
+{
+  ASSERT_ON_THREAD(mParent->GetMainThread());
+
+  // Refuse to hand out references if we're tearing down.
+  // (Since teardown involves a dispatch to and from STS before MediaPipelines
+  // are released, it is safe to start other dispatches to and from STS with a
+  // RefPtr<MediaPipeline>, since that reference won't be the last one
+  // standing)
+  if (mMediaStream) {
+    if (mPipelines.count(trackId)) {
+      RefPtr<MediaPipeline> pipeline(mPipelines[trackId]);
+      mPipelines.erase(trackId);
+      return pipeline.forget();
+    }
+  }
+
+  return nullptr;
+}
+
 }  // namespace mozilla

media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h

@@ -126,12 +126,9 @@ public:
                         const std::string& aId)
      : SourceStreamInfo(aMediaStream, aParent, aId) {}
 
-  // XXX NOTE: does not change mMediaStream, even if it replaces the last track
-  // in a LocalSourceStreamInfo.  Revise when we have support for multiple tracks
-  // of a type.
-  nsresult ReplaceTrack(const std::string& oldTrackId,
-                        DOMMediaStream* aNewStream,
-                        const std::string& aNewTrack);
+  nsresult TakePipelineFrom(RefPtr<LocalSourceStreamInfo>& info,
+                            const std::string& oldTrackId,
+                            const std::string& newTrackId);
 
 #ifdef MOZILLA_INTERNAL_API
   void UpdateSinkIdentity_m(nsIPrincipal* aPrincipal,
@@ -139,6 +136,10 @@ public:
 #endif
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(LocalSourceStreamInfo)
+
+private:
+  TemporaryRef<MediaPipeline> ForgetPipelineByTrackId_m(
+      const std::string& trackId);
 };
 
 class RemoteSourceStreamInfo : public SourceStreamInfo {
@@ -290,6 +291,12 @@ class PeerConnectionMedia : public sigslot::has_slots<> {
   // Add a remote stream.
   nsresult AddRemoteStream(nsRefPtr<RemoteSourceStreamInfo> aInfo);
 
+  nsresult ReplaceTrack(const std::string& oldStreamId,
+                        const std::string& oldTrackId,
+                        DOMMediaStream* aNewStream,
+                        const std::string& newStreamId,
+                        const std::string& aNewTrack);
+
 #ifdef MOZILLA_INTERNAL_API
   // In cases where the peer isn't yet identified, we disable the pipeline (not
   // the stream, that would potentially affect others), so that it sends