Bug 1136871 - cleanup RtpSenders accounting to not rely on streams
--- 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
@@ -161,16 +161,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
@@ -892,16 +892,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
@@ -2084,61 +2084,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(