1271669 - Allow any MediaStream to be passed to RTCPeerConnection.addTrack

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Andreas Pehrson [:pehrsons]]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Probably easy

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Test that we can pc.addTrack() with any MediaStream. r?jib

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

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Change AnyLocalStreamHasPeerIdentity to be per-track. r?mt

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Don't rely on streams for getStats(). r?jib,bwc

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

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Clean up some stream-centered code in MediaPipelineFactory. r?bwc

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

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Ensure media element flow is tested for all local tracks. r?jib

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

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Remove GetVideoTrackByTrackId(). r?bwc

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/

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib

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/

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Remove test_peerConnection_addTrack.html. r?jib

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/

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Don't test invalid pc.addTrack() in test_pc_replaceTrack.html. r?jib

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/

Comment 11

[Andreas Pehrson [:pehrsons]]

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

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

[Byron Campen [:bwc]]

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

Comment 14

[Andreas Pehrson [:pehrsons]]

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

Comment 15

[Byron Campen [:bwc]]

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

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

Comment 16

[Andreas Pehrson [:pehrsons]]

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/

Comment 17

[Andreas Pehrson [:pehrsons]]

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/

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.)

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 ( : ) ?

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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).

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

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

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 25

[Martin Thomson [:mt:]]

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

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

Comment 26

[Andreas Pehrson [:pehrsons]]

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.

Comment 27

[Andreas Pehrson [:pehrsons]]

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

Makes it much nicer to read!

Comment 28

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1271669 - Deprecate RTCPeerConnection.getLocalStreams/getRemoteStreams. r?jib, r?smaug

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/

Comment 29

[Andreas Pehrson [:pehrsons]]

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/

Comment 30

[Andreas Pehrson [:pehrsons]]

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/

Comment 31

[Andreas Pehrson [:pehrsons]]

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/

Comment 32

[Andreas Pehrson [:pehrsons]]

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/

Comment 33

[Andreas Pehrson [:pehrsons]]

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/

Comment 34

[Andreas Pehrson [:pehrsons]]

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/

Comment 35

[Andreas Pehrson [:pehrsons]]

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/

Comment 36

[Andreas Pehrson [:pehrsons]]

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/

Comment 37

[Andreas Pehrson [:pehrsons]]

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/

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 40

[Andreas Pehrson [:pehrsons]]

(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.

Comment 41

[Pulsebot]

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

Comment 42

[Carsten Book [:Tomcat]]

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

Comment 43

[Kohei Yoshino]

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

Comment 44

[Eric Shepherd [:sheppy]]

Documentation updated:

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

Also updated Firefox 49 for developers.

Comment 45

[HoangNK]

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

Comment 46

[Andreas Pehrson [:pehrsons]]

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