Andreas Pehrson [:pehrsons] Assignee
	Description • 8 years ago

Currently RTCPeerConnection.addTrack requires the passed in MediaStream to be the owning stream of the passed track.

With the change to track listeners in MediaPipeline in bug 1208371, we should be able to remove this constraint.

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 8 years ago

Probably easy

	Andreas Pehrson [:pehrsons] Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53916/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 3 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53918/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 4 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53920/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 5 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53922/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 6 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53924/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 7 • 8 years ago

We have GetTrackById() now that does essentially the same thing.
Also, GetVideoTrackByTrackId() was too focused on the owning MediaStream and
didn't work.

Review commit: https://reviewboard.mozilla.org/r/53926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53926/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 8 • 8 years ago

This change RTCPeerConnection.addTrack() to allow any MediaStream as argument.

The MediaStream is in the end used as an identifier of how the tracks will be
grouped together on the receiving side of the peer connection.

Review commit: https://reviewboard.mozilla.org/r/53928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53928/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 9 • 8 years ago

This tested that we were throwing on adding a track with a constructed
MediaStream, which we now allow.

Review commit: https://reviewboard.mozilla.org/r/53930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53930/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 10 • 8 years ago

It is now valid, with its own test.

Review commit: https://reviewboard.mozilla.org/r/53932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53932/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 11 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c95747567b66

	Byron Campen [:bwc]
	Comment 12 • 8 years ago

Comment on attachment 8754375 [details]
MozReview Request: Bug 1271669 - Don't rely on streams for getStats(). r?jib,bwc

https://reviewboard.mozilla.org/r/53920/#review50650

I would be perfectly happy if we just used a loop with an if (!aSelector || /* trackId matches */), since the performance difference is going to be negligible, but this works too.

	Byron Campen [:bwc]
	Comment 13 • 8 years ago

Comment on attachment 8754376 [details]
MozReview Request: Bug 1271669 - Clean up some stream-centered code in MediaPipelineFactory. r?bwc

https://reviewboard.mozilla.org/r/53922/#review50658

	Andreas Pehrson [:pehrsons] Assignee
	Comment 14 • 8 years ago

This should be better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5171d2df400a

	Byron Campen [:bwc]
	Comment 15 • 8 years ago

Comment on attachment 8754378 [details]
MozReview Request: Bug 1271669 - Remove GetVideoTrackByTrackId(). r?bwc

https://reviewboard.mozilla.org/r/53926/#review50660

	Andreas Pehrson [:pehrsons] Assignee
	Comment 16 • 8 years ago

Comment on attachment 8754380 [details]
MozReview Request: Bug 1271669 - Remove test_peerConnection_addTrack.html. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53930/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 17 • 8 years ago

Comment on attachment 8754381 [details]
MozReview Request: Bug 1271669 - Don't test invalid pc.addTrack() in test_pc_replaceTrack.html. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53932/diff/1-2/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 8 years ago

Comment on attachment 8754379 [details]
MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib

I don't know why my review would be needed for this.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 19 • 8 years ago

Comment on attachment 8754373 [details]
MozReview Request: Bug 1271669 - Test that we can pc.addTrack() with any MediaStream. r?jib

https://reviewboard.mozilla.org/r/53916/#review50676

::: dom/media/tests/mochitest/test_peerConnection_constructedStream.html:21
(Diff revision 1)
> +  test.setMediaConstraints([ {audio: true, video: true}
> +                           , {audio: true}
> +                           , {video: true}
> +                           ], []);

Novel comma placement.

::: dom/media/tests/mochitest/test_peerConnection_constructedStream.html:28
(Diff revision 1)
> +        .then(stream => constructedStream = new MediaStream(stream.getTracks()))
> +        .then(() => test.pcLocal.attachLocalStream(constructedStream));

Nit: not a fan of synchronous .then (middle then's not returning a promise). Can we use {} or merge to one line?

Once bug 1185106 lands and we redo all our tests, these wont trip us up.

::: dom/media/tests/mochitest/test_peerConnection_constructedStream.html:33
(Diff revision 1)
> +        .then(stream => constructedStream = new MediaStream(stream.getTracks()))
> +        .then(() => test.pcLocal.attachLocalStream(constructedStream));
> +    },
> +    function PC_LOCAL_GUM_DUMMY_STREAM(test) {
> +      return getUserMedia(test.pcLocal.constraints[1])
> +        .then(stream => Array.prototype.push.apply(dummyStreamTracks, stream.getTracks()))

ES6's spread operator is nicer here:

    dummyStreamTracks.push(...stream.getTracks())

::: dom/media/tests/mochitest/test_peerConnection_constructedStream.html:46
(Diff revision 1)
> +  let checkSentTracksReceived = (sentStreamId, sentTracks) => {
> +    let receivedStream =
> +      test.pcRemote._pc.getRemoteStreams().find(s => s.id == sentStreamId);
> +    ok(receivedStream,
> +       "We should receive a stream with with the sent stream's id (" + sentStreamId + ")");
> +    if (!receivedStream) { return; }

Can we assert instead? Defensive test-skips make me nervous.

(If not: style-guide nit: return on next line.)

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 20 • 8 years ago

Comment on attachment 8754375 [details]
MozReview Request: Bug 1271669 - Don't rely on streams for getStats(). r?jib,bwc

https://reviewboard.mozilla.org/r/53920/#review50696

lgtm.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3364
(Diff revision 1)
> -      }
> +    }
> -    } else {
> +  } else {
> +    // No selector provided. Add all pipelines available.
> +    for (int i = 0, len = mMedia->LocalStreamsLength(); i < len; i++) {
> +      auto& pipelines = mMedia->GetLocalStreamByIndex(i)->GetPipelines();
>        for (auto it = pipelines.begin(); it != pipelines.end(); ++it) {

Maybe make the (now nested) for loop use for ( : ) ?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 21 • 8 years ago

Comment on attachment 8754377 [details]
MozReview Request: Bug 1271669 - Ensure media element flow is tested for all local tracks. r?jib

https://reviewboard.mozilla.org/r/53924/#review50698

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 22 • 8 years ago

Comment on attachment 8754379 [details]
MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib

https://reviewboard.mozilla.org/r/53928/#review50702

::: dom/media/PeerConnection.js
(Diff revision 1)
> -    if (stream.getTracks().indexOf(track) < 0) {
> -      throw new this._win.DOMException("track is not in stream.",
> -                                       "InvalidParameterError");
> -    }

Note that the spec doesn't explicitly address this yet, though we seem to be leaning this way in https://github.com/w3c/webrtc-pc/issues/369 so we're probably good.

::: dom/media/PeerConnection.js
(Diff revision 1)
> -    try {
> -      this._impl.addTrack(track, stream);
> +    this._impl.addTrack(track, stream);
> -    } catch (e if (e.result == Cr.NS_ERROR_NOT_IMPLEMENTED)) {
> -      throw new this._win.DOMException(
> -          "track in constructed stream not yet supported (see Bug 1259236).",
> -          "NotSupportedError");
> -    }

This is good, but reminds me to check that bug 1245983 doesn't regress when we remove this try/catch block (in hindsight I'm not 100% sure it was a clean dup).

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 23 • 8 years ago

Comment on attachment 8754380 [details]
MozReview Request: Bug 1271669 - Remove test_peerConnection_addTrack.html. r?jib

https://reviewboard.mozilla.org/r/53930/#review50706

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 24 • 8 years ago

Comment on attachment 8754381 [details]
MozReview Request: Bug 1271669 - Don't test invalid pc.addTrack() in test_pc_replaceTrack.html. r?jib

https://reviewboard.mozilla.org/r/53932/#review50708

	Martin Thomson [:mt:]
	Comment 25 • 8 years ago

Comment on attachment 8754374 [details]
MozReview Request: Bug 1271669 - Change AnyLocalStreamHasPeerIdentity to be per-track. r?mt

https://reviewboard.mozilla.org/r/53918/#review50752

	Andreas Pehrson [:pehrsons] Assignee
	Comment 26 • 8 years ago

https://reviewboard.mozilla.org/r/53916/#review50676

> Novel comma placement.

Novel? Tssk, this is the best take-away from having coded Haskell :-P

> ES6's spread operator is nicer here:
> 
>     dummyStreamTracks.push(...stream.getTracks())

Ooh, this was new to me.

> Can we assert instead? Defensive test-skips make me nervous.
> 
> (If not: style-guide nit: return on next line.)

Yeah.. one line above.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 27 • 8 years ago

https://reviewboard.mozilla.org/r/53920/#review50650

Makes it much nicer to read!

	Andreas Pehrson [:pehrsons] Assignee
	Comment 28 • 8 years ago

getLocalStreams and getRemoteStreams have been removed from the spec.

Also, getLocalStreams becomes a problem now that addTrack takes any MediaStream
as the container for the added track.

The addTrack()'ed MediaStream (perhaps not containing the track sent over the
pc - perhaps containing other tracks not sent over the pc!) will be returned
from getLocalStreams.

Review commit: https://reviewboard.mozilla.org/r/54206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54206/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 29 • 8 years ago

Comment on attachment 8754373 [details]
MozReview Request: Bug 1271669 - Test that we can pc.addTrack() with any MediaStream. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53916/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 30 • 8 years ago

Comment on attachment 8754374 [details]
MozReview Request: Bug 1271669 - Change AnyLocalStreamHasPeerIdentity to be per-track. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53918/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 31 • 8 years ago

Comment on attachment 8754375 [details]
MozReview Request: Bug 1271669 - Don't rely on streams for getStats(). r?jib,bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53920/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 32 • 8 years ago

Comment on attachment 8754376 [details]
MozReview Request: Bug 1271669 - Clean up some stream-centered code in MediaPipelineFactory. r?bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53922/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 33 • 8 years ago

Comment on attachment 8754377 [details]
MozReview Request: Bug 1271669 - Ensure media element flow is tested for all local tracks. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53924/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 34 • 8 years ago

Comment on attachment 8754378 [details]
MozReview Request: Bug 1271669 - Remove GetVideoTrackByTrackId(). r?bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53926/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 35 • 8 years ago

Comment on attachment 8754379 [details]
MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53928/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 36 • 8 years ago

Comment on attachment 8754380 [details]
MozReview Request: Bug 1271669 - Remove test_peerConnection_addTrack.html. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53930/diff/2-3/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 37 • 8 years ago

Comment on attachment 8754381 [details]
MozReview Request: Bug 1271669 - Don't test invalid pc.addTrack() in test_pc_replaceTrack.html. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53932/diff/2-3/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 38 • 8 years ago

Comment on attachment 8754711 [details]
MozReview Request: Bug 1271669 - Deprecate RTCPeerConnection.getLocalStreams/getRemoteStreams. r?jib, r?smaug

https://reviewboard.mozilla.org/r/54206/#review50902

A bit sad to see relatively new API (WebRTC) deprecating already some methods.  Oh well.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 39 • 8 years ago

Comment on attachment 8754711 [details]
MozReview Request: Bug 1271669 - Deprecate RTCPeerConnection.getLocalStreams/getRemoteStreams. r?jib, r?smaug

https://reviewboard.mozilla.org/r/54206/#review50938

We're out front on this, as Chrome still hasn't pivoted to tracks. Unfortunately, the track APIs are hard to polyfill so developers will be stuck with warnings for a while, but we already have deprecation warnings on addStream riding the trains, so this doesn't make it much worse. It trust these will still do the right thing (with a warning) for the basic cases.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 40 • 8 years ago

(In reply to Jan-Ivar Bruaroey [:jib] from comment #39)
> It trust
> these will still do the right thing (with a warning) for the basic cases.

They will! They'll only get into weird states if addTrack() is used to its full capabilities - like passing in a track and an empty MediaStream. getLocalStreams() will return the empty stream and you can't find the track through that path.

	Pulsebot
	Comment 41 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f75699d18
https://hg.mozilla.org/integration/mozilla-inbound/rev/565f32248745
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4950e6a5ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/724788b0b920
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee47773d700f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe82e83c7252
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ac4527fbbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7dbc0411d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5af54804e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df301649c70

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4e6f75699d18
https://hg.mozilla.org/mozilla-central/rev/565f32248745
https://hg.mozilla.org/mozilla-central/rev/2c4950e6a5ee
https://hg.mozilla.org/mozilla-central/rev/724788b0b920
https://hg.mozilla.org/mozilla-central/rev/ee47773d700f
https://hg.mozilla.org/mozilla-central/rev/fe82e83c7252
https://hg.mozilla.org/mozilla-central/rev/b9ac4527fbbb
https://hg.mozilla.org/mozilla-central/rev/2a7dbc0411d1
https://hg.mozilla.org/mozilla-central/rev/dd5af54804e8
https://hg.mozilla.org/mozilla-central/rev/5df301649c70

	Kohei Yoshino [:kohei]
	Comment 43 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/rtcpeerconnection-getlocalstreams-getremotestreams-have-been-deprecated/

	Eric Shepherd [:sheppy]
	Comment 44 • 8 years ago

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addTrack

Also updated Firefox 49 for developers.

	HoangNK
	Comment 45 • 8 years ago

What about minor of Firefox version 48. Can Firefox v48.1 or v48.2 fix this bug?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 46 • 8 years ago

That won't happen. See the rules at https://wiki.mozilla.org/Release_Management/Uplift_rules#Release_Uplift