Bug 1136871 - cleanup RtpSenders accounting to not rely on streams r=mt, a=lizzard
authorJan-Ivar Bruaroey <jib@mozilla.com>
Wed, 11 Mar 2015 12:24:38 -0400
changeset 257729 8bbd2e9d75f9dd524749f2c40fa82e0f74ee06f6
parent 257728 d4a23c33ce27b241976a4c5d8c0e303df4ea8433
child 257730 8b92dc52aab2ecbb98b62e1f8c9f25ba2a2c9000
push id4610
push userjlund@mozilla.com
push dateMon, 30 Mar 2015 18:32:55 +0000
treeherdermozilla-beta@4df54044d9ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmt, lizzard
bugs1136871
milestone38.0a2
Bug 1136871 - cleanup RtpSenders accounting to not rely on streams r=mt, a=lizzard
dom/media/PeerConnection.js
dom/media/tests/mochitest/mochitest.ini
dom/media/tests/mochitest/pc.js
dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html
dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
--- a/dom/media/PeerConnection.js
+++ b/dom/media/PeerConnection.js
@@ -827,27 +827,37 @@ RTCPeerConnection.prototype = {
     if (stream.currentTime === undefined) {
       throw new this._win.DOMException("invalid stream.", "InvalidParameterError");
     }
     if (stream.getTracks().indexOf(track) < 0) {
       throw new this._win.DOMException("track is not in stream.",
                                        "InvalidParameterError");
     }
     this._checkClosed();
+    this._senders.forEach(sender => {
+      if (sender.track == track) {
+        throw new this._win.DOMException("already added.",
+                                         "InvalidParameterError");
+      }
+    });
     this._impl.addTrack(track, stream);
     let sender = this._win.RTCRtpSender._create(this._win,
                                                 new RTCRtpSender(this, track,
                                                                  stream));
-    this._senders.push({ sender: sender, stream: stream });
+    this._senders.push(sender);
     return sender;
   },
 
   removeTrack: function(sender) {
     this._checkClosed();
-    this._impl.removeTrack(sender.track);
+    var i = this._senders.indexOf(sender);
+    if (i >= 0) {
+      this._senders.splice(i, 1);
+      this._impl.removeTrack(sender.track); // fires negotiation needed
+    }
   },
 
   _replaceTrack: function(sender, withTrack) {
     // TODO: Do a (sender._stream.getTracks().indexOf(track) < 0) 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
@@ -883,43 +893,21 @@ RTCPeerConnection.prototype = {
   },
 
   getRemoteStreams: function() {
     this._checkClosed();
     return this._impl.getRemoteStreams();
   },
 
   getSenders: function() {
-    this._checkClosed();
-    let streams = this._impl.getLocalStreams();
-    let senders = [];
-    // prune senders in case any streams have disappeared down below
-    for (let i = this._senders.length - 1; i >= 0; i--) {
-      if (streams.indexOf(this._senders[i].stream) != -1) {
-        senders.push(this._senders[i].sender);
-      } else {
-        this._senders.splice(i,1);
-      }
-    }
-    return senders;
+    return this._senders;
   },
 
   getReceivers: function() {
-    this._checkClosed();
-    let streams = this._impl.getRemoteStreams();
-    let receivers = [];
-    // prune receivers in case any streams have disappeared down below
-    for (let i = this._receivers.length - 1; i >= 0; i--) {
-      if (streams.indexOf(this._receivers[i].stream) != -1) {
-        receivers.push(this._receivers[i].receiver);
-      } else {
-        this._receivers.splice(i,1);
-      }
-    }
-    return receivers;
+    return this._receivers;
   },
 
   get localDescription() {
     this._checkClosed();
     let sdp = this._impl.localDescription;
     if (sdp.length == 0) {
       return null;
     }
@@ -1056,17 +1044,17 @@ PeerConnectionObserver.prototype = {
   },
 
   newError: function(message, code) {
     // These strings must match those defined in the WebRTC spec.
     const reasonName = [
       "",
       "InternalError",
       "InvalidCandidateError",
-      "InvalidParameter",
+      "InvalidParameterError",
       "InvalidStateError",
       "InvalidSessionDescriptionError",
       "IncompatibleSessionDescriptionError",
       "InternalError",
       "IncompatibleMediaStreamTrackError",
       "InternalError"
     ];
     let name = reasonName[Math.min(code, reasonName.length - 1)];
--- a/dom/media/tests/mochitest/mochitest.ini
+++ b/dom/media/tests/mochitest/mochitest.ini
@@ -159,16 +159,18 @@ skip-if = toolkit == 'gonk' # b2g (Bug 1
 [test_peerConnection_removeThenAddAudioTrack.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_addSecondVideoStream.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_removeVideoTrack.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_removeThenAddVideoTrack.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
+[test_peerConnection_replaceVideoThenRenegotiate.html]
+skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_addSecondAudioStreamNoBundle.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_removeThenAddAudioTrackNoBundle.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_addSecondVideoStreamNoBundle.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
 [test_peerConnection_removeThenAddVideoTrackNoBundle.html]
 skip-if = toolkit == 'gonk' # b2g (Bug 1059867)
--- a/dom/media/tests/mochitest/pc.js
+++ b/dom/media/tests/mochitest/pc.js
@@ -889,16 +889,23 @@ PeerConnectionWrapper.prototype = {
   },
 
   removeSender : function(index) {
     var sender = this._pc.getSenders()[index];
     delete this.expectedLocalTrackTypesById[sender.track.id];
     this._pc.removeTrack(sender);
   },
 
+  senderReplaceTrack : function(index, withTrack) {
+    var sender = this._pc.getSenders()[index];
+    delete this.expectedLocalTrackTypesById[sender.track.id];
+    this.expectedLocalTrackTypesById[withTrack.id] = withTrack.kind;
+    return sender.replaceTrack(withTrack);
+  },
+
   /**
    * Requests all the media streams as specified in the constrains property.
    *
    * @param {array} constraintsList
    *        Array of constraints for GUM calls
    */
   getAllUserMedia : function(constraintsList) {
     if (constraintsList.length === 0) {
--- a/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html
+++ b/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html
@@ -13,17 +13,16 @@
 
   var test;
   runNetworkTest(function (options) {
     test = new PeerConnectionTest(options);
     addRenegotiation(test.chain,
       [
         function PC_LOCAL_REMOVE_AUDIO_TRACK(test) {
           test.setOfferOptions({ offerToReceiveAudio: true });
-          test.setMediaConstraints([], [{audio: true}]);
           return test.pcLocal.removeSender(0);
         },
       ]
     );
 
     // TODO(bug 1093835): figure out how to verify that media stopped flowing from pcLocal
 
     test.setMediaConstraints([{audio: true}], [{audio: true}]);
--- a/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
+++ b/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
@@ -1,9 +1,9 @@
-<!DOCTYPE HTML>
+<!DOCTYPE HTML>
 <html>
 <head>
   <script type="application/javascript" src="pc.js"></script>
 </head>
 <body>
 <pre id="test">
 <script type="application/javascript;version=1.8">
   createHTML({
@@ -11,59 +11,93 @@
     title: "Replace video track",
     visible: true
   });
 
   function isSenderOfTrack(sender) {
     return sender.track == this;
   }
 
-  // Test basically just verifies that success callback is called at this point
-
   runNetworkTest(function () {
     test = new PeerConnectionTest();
     test.setMediaConstraints([{video: true}], [{video: true}]);
     test.chain.removeAfter("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
     var flowtest = test.chain.remove("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
     test.chain.append(flowtest);
 
     var replacetest = [ function PC_LOCAL_REPLACE_VIDEOTRACK(test) {
-      var stream = test.pcLocal._pc.getLocalStreams()[0];
-      var track = stream.getVideoTracks()[0];
-      var sender = test.pcLocal._pc.getSenders().find(isSenderOfTrack, track);
+      var oldstream = test.pcLocal._pc.getLocalStreams()[0];
+      var oldtrack = oldstream.getVideoTracks()[0];
+      var sender = test.pcLocal._pc.getSenders().find(isSenderOfTrack, oldtrack);
       ok(sender, "track has a sender");
       var newtrack;
-      return navigator.mediaDevices.getUserMedia({video:true, fake: true})
-        .then(newStream => {
-          newtrack = newStream.getVideoTracks()[0];
-          isnot(newtrack, track, "replacing with a different track");
-          isnot(newStream, stream, "from a different stream");
+      var audiotrack;
+      return navigator.mediaDevices.getUserMedia({video:true, audio:true, fake:true})
+        .then(newstream => {
+          newtrack = newstream.getVideoTracks()[0];
+          audiotrack = newstream.getAudioTracks()[0];
+          isnot(newtrack, oldtrack, "replacing with a different track");
+          isnot(newstream, oldstream, "from a different stream");
           return sender.replaceTrack(newtrack);
         })
         .then(() => {
           is(sender.track, newtrack, "sender.track has been replaced");
           var stream = test.pcLocal._pc.getLocalStreams()[0];
           var track = stream.getVideoTracks()[0];
           is(track, newtrack, "track has been replaced in stream");
+          return sender.replaceTrack(audiotrack)
+            .then(() => ok(false, "replacing with different kind should fail"),
+                  e => is(e.name, "IncompatibleMediaStreamTrackError",
+                          "replacing with different kind should fail"));
         });
     } ];
-    // Do it twice to make sure it still works.
+    // Do it twice to make sure it still works (does audio twice too, but hey)
     test.chain.append(replacetest);
     test.chain.append(flowtest);
     test.chain.append(replacetest);
     test.chain.append(flowtest);
     test.chain.append([
-      function PC_LOCAL_INVALID_REPLACE_VIDEOTRACK(test) {
+      function PC_LOCAL_REPLACE_VIDEOTRACK_WITHSAME(test) {
+        var oldstream = test.pcLocal._pc.getLocalStreams()[0];
+        var oldtrack = oldstream.getVideoTracks()[0];
+        var sender = test.pcLocal._pc.getSenders().find(isSenderOfTrack, oldtrack);
+        return sender.replaceTrack(oldtrack) // same track
+          .then(() => ok(true, "replacing with itself should succeed"));
+      }
+    ]);
+    test.chain.append(flowtest);
+    test.chain.append([
+      function PC_LOCAL_INVALID_ADD_VIDEOTRACKS(test) {
         var stream = test.pcLocal._pc.getLocalStreams()[0];
         var track = stream.getVideoTracks()[0];
-        var sender = test.pcLocal._pc.getSenders().find(isSenderOfTrack, track);
-        return sender.replaceTrack(track) // same track
-          .then(() => ok(false, "replacing with itself should fail"),
-                e => is(e.name, "InvalidParameter",
-                        "replacing with itself should fail"));
+        try {
+          test.pcLocal._pc.addTrack(track, stream);
+          ok(false, "addTrack existing track should fail");
+        } catch (e) {
+          is(e.name, "InvalidParameterError",
+             "addTrack existing track should fail");
+        }
+        try {
+          test.pcLocal._pc.addTrack(track, stream);
+          ok(false, "addTrack existing track should fail");
+        } catch (e) {
+          is(e.name, "InvalidParameterError",
+             "addTrack existing track should fail");
+        }
+        return navigator.mediaDevices.getUserMedia({video:true, fake: true})
+          .then(differentStream => {
+            var track = differentStream.getVideoTracks()[0];
+            try {
+              test.pcLocal._pc.addTrack(track, stream);
+              ok(false, "addTrack w/wrong stream should fail");
+            } catch (e) {
+              is(e.name, "InvalidParameterError",
+                 "addTrack w/wrong stream should fail");
+            }
+          });
       }
     ]);
     test.run();
   });
 </script>
 </pre>
 </body>
 </html>
new file mode 100644
--- /dev/null
+++ b/dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html
@@ -0,0 +1,46 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <script type="application/javascript" src="pc.js"></script>
+</head>
+<body>
+<pre id="test">
+<script type="application/javascript">
+  createHTML({
+    bug: "1017888",
+    title: "Renegotiation: replaceTrack followed by adding a second video stream"
+  });
+
+  var test;
+  runNetworkTest(function (options) {
+    test = new PeerConnectionTest(options);
+    test.setMediaConstraints([{video:true}], [{video:true}]);
+    addRenegotiation(test.chain,
+      [
+        function PC_LOCAL_REPLACE_VIDEO_TRACK_THEN_ADD_SECOND_STREAM(test) {
+          var oldstream = test.pcLocal._pc.getLocalStreams()[0];
+          var oldtrack = oldstream.getVideoTracks()[0];
+          var sender = test.pcLocal._pc.getSenders()[0];
+          return navigator.mediaDevices.getUserMedia({video:true, fake:true})
+            .then(newstream => {
+              var newtrack = newstream.getVideoTracks()[0];
+              return test.pcLocal.senderReplaceTrack(0, newtrack);
+            })
+            .then(() => {
+              test.setMediaConstraints([{video: true}, {video: true}],
+                                       [{video: true}]);
+              return test.pcLocal.getAllUserMedia([{video: true}]);
+            });
+        },
+      ]
+    );
+
+    // TODO(bug 1093835):
+    // figure out how to verify if media flows through the new stream
+    // figure out how to verify that media stopped flowing from old stream
+    test.run();
+  });
+</script>
+</pre>
+</body>
+</html>
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -2075,61 +2075,92 @@ PeerConnectionImpl::RemoveTrack(MediaStr
   return NS_OK;
 }
 
 NS_IMETHODIMP
 PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack,
                                  MediaStreamTrack& aWithTrack) {
   PC_AUTO_ENTER_API_CALL(true);
 
-  JSErrorResult jrv;
   nsRefPtr<PeerConnectionObserver> pco = do_QueryObjectReferent(mPCObserver);
   if (!pco) {
     return NS_ERROR_UNEXPECTED;
   }
-
+  JSErrorResult jrv;
+
+#ifdef MOZILLA_INTERNAL_API
+  if (&aThisTrack == &aWithTrack) {
+    pco->OnReplaceTrackSuccess(jrv);
+    if (jrv.Failed()) {
+      CSFLogError(logTag, "Error firing replaceTrack success callback");
+      return NS_ERROR_UNEXPECTED;
+    }
+    return NS_OK;
+  }
+
+  nsString thisKind;
+  aThisTrack.GetKind(thisKind);
+  nsString withKind;
+  aWithTrack.GetKind(withKind);
+
+  if (thisKind != withKind) {
+    pco->OnReplaceTrackError(kIncompatibleMediaStreamTrack,
+                             ObString(mJsepSession->GetLastError().c_str()),
+                             jrv);
+    if (jrv.Failed()) {
+      CSFLogError(logTag, "Error firing replaceTrack success callback");
+      return NS_ERROR_UNEXPECTED;
+    }
+    return NS_OK;
+  }
+#endif
   std::string origTrackId = PeerConnectionImpl::GetTrackId(aThisTrack);
   std::string newTrackId = PeerConnectionImpl::GetTrackId(aWithTrack);
 
   std::string origStreamId =
     PeerConnectionImpl::GetStreamId(*aThisTrack.GetStream());
   std::string newStreamId =
     PeerConnectionImpl::GetStreamId(*aWithTrack.GetStream());
 
   nsresult rv = mJsepSession->ReplaceTrack(origStreamId,
                                            origTrackId,
                                            newStreamId,
                                            newTrackId);
-
   if (NS_FAILED(rv)) {
     pco->OnReplaceTrackError(kInvalidMediastreamTrack,
                              ObString(mJsepSession->GetLastError().c_str()),
                              jrv);
+    if (jrv.Failed()) {
+      CSFLogError(logTag, "Error firing replaceTrack error callback");
+      return NS_ERROR_UNEXPECTED;
+    }
     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);
+    if (jrv.Failed()) {
+      CSFLogError(logTag, "Error firing replaceTrack error callback");
+      return NS_ERROR_UNEXPECTED;
+    }
     return NS_OK;
   }
-
   pco->OnReplaceTrackSuccess(jrv);
-
   if (jrv.Failed()) {
-    CSFLogError(logTag, "Error firing replaceTrack callback");
+    CSFLogError(logTag, "Error firing replaceTrack success callback");
     return NS_ERROR_UNEXPECTED;
   }
 
   return NS_OK;
 }
 
 nsresult
 PeerConnectionImpl::CalculateFingerprint(