From d132e365398a3adb2ad73f9f5ca96d85dfda6795 Mon Sep 17 00:00:00 2001 From: Jan-Ivar Bruaroey Date: Wed, 11 Mar 2015 12:24:38 -0400 Subject: [PATCH] Bug 1136871 - cleanup RtpSenders accounting to not rely on streams r=mt --- dom/media/PeerConnection.js | 42 ++++------- dom/media/tests/mochitest/mochitest.ini | 2 + dom/media/tests/mochitest/pc.js | 7 ++ .../test_peerConnection_removeAudioTrack.html | 1 - .../test_peerConnection_replaceTrack.html | 70 ++++++++++++++----- ...onnection_replaceVideoThenRenegotiate.html | 46 ++++++++++++ .../src/peerconnection/PeerConnectionImpl.cpp | 41 +++++++++-- 7 files changed, 158 insertions(+), 51 deletions(-) create mode 100644 dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html diff --git a/dom/media/PeerConnection.js b/dom/media/PeerConnection.js index ea7ebf1a6de8..bfd32e63d31a 100644 --- a/dom/media/PeerConnection.js +++ b/dom/media/PeerConnection.js @@ -832,17 +832,27 @@ RTCPeerConnection.prototype = { "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) { @@ -888,33 +898,11 @@ RTCPeerConnection.prototype = { }, 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() { @@ -1061,7 +1049,7 @@ PeerConnectionObserver.prototype = { "", "InternalError", "InvalidCandidateError", - "InvalidParameter", + "InvalidParameterError", "InvalidStateError", "InvalidSessionDescriptionError", "IncompatibleSessionDescriptionError", diff --git a/dom/media/tests/mochitest/mochitest.ini b/dom/media/tests/mochitest/mochitest.ini index fd6cf241a814..a68723334ef9 100644 --- a/dom/media/tests/mochitest/mochitest.ini +++ b/dom/media/tests/mochitest/mochitest.ini @@ -166,6 +166,8 @@ skip-if = toolkit == 'gonk' # b2g (Bug 1059867) 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] diff --git a/dom/media/tests/mochitest/pc.js b/dom/media/tests/mochitest/pc.js index 9958f78578e5..4b8c183b230e 100644 --- a/dom/media/tests/mochitest/pc.js +++ b/dom/media/tests/mochitest/pc.js @@ -897,6 +897,13 @@ PeerConnectionWrapper.prototype = { 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. * diff --git a/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html b/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html index 98e70801f98b..dfb5c5b4cc3f 100644 --- a/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html +++ b/dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html @@ -18,7 +18,6 @@ [ function PC_LOCAL_REMOVE_AUDIO_TRACK(test) { test.setOfferOptions({ offerToReceiveAudio: true }); - test.setMediaConstraints([], [{audio: true}]); return test.pcLocal.removeSender(0); }, ] diff --git a/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html b/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html index 0510c97a9af5..c535ead3e557 100644 --- a/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html +++ b/dom/media/tests/mochitest/test_peerConnection_replaceTrack.html @@ -1,4 +1,4 @@ - + @@ -16,8 +16,6 @@ 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}]); @@ -26,16 +24,18 @@ 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(() => { @@ -43,22 +43,56 @@ 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(); diff --git a/dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html b/dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html new file mode 100644 index 000000000000..f7532afffd05 --- /dev/null +++ b/dom/media/tests/mochitest/test_peerConnection_replaceVideoThenRenegotiate.html @@ -0,0 +1,46 @@ + + + + + + +
+
+
+ + diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp index f1bc915229bd..5b3baebe9f65 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ -2089,12 +2089,38 @@ PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack, MediaStreamTrack& aWithTrack) { PC_AUTO_ENTER_API_CALL(true); - JSErrorResult jrv; nsRefPtr 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); @@ -2107,11 +2133,14 @@ PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack, 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; } @@ -2127,13 +2156,15 @@ PeerConnectionImpl::ReplaceTrack(MediaStreamTrack& aThisTrack, 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; }