Allow any MediaStream to be passed to RTCPeerConnection.addTrack

RESOLVED FIXED in Firefox 49

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

({dev-doc-complete, site-compat})

48 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(10 attachments)

58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
bwc
: review+
jib
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
jib
: review+
Details
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.
Probably easy
Rank: 25
Priority: -- → P2
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/53916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53916/
Attachment #8754373 - Flags: review?(jib)
Attachment #8754374 - Flags: review?(martin.thomson)
Attachment #8754375 - Flags: review?(jib)
Attachment #8754375 - Flags: review?(docfaraday)
Attachment #8754376 - Flags: review?(docfaraday)
Attachment #8754377 - Flags: review?(jib)
Attachment #8754378 - Flags: review?(docfaraday)
Attachment #8754379 - Flags: review?(jib)
Attachment #8754379 - Flags: review?(bugs)
Attachment #8754380 - Flags: review?(jib)
Attachment #8754381 - Flags: review?(jib)
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/
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/
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 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.
Attachment #8754375 - Flags: review?(docfaraday) → review+
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
Attachment #8754376 - Flags: review?(docfaraday) → review+
Comment on attachment 8754378 [details]
MozReview Request: Bug 1271669 - Remove GetVideoTrackByTrackId(). r?bwc

https://reviewboard.mozilla.org/r/53926/#review50660
Attachment #8754378 - Flags: review?(docfaraday) → review+
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 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 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.
Attachment #8754379 - Flags: review?(bugs)
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.)
Attachment #8754373 - Flags: review?(jib) → review+
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 ( : ) ?
Attachment #8754375 - Flags: review?(jib) → review+
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
Attachment #8754377 - Flags: review?(jib) → review+
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).
Attachment #8754379 - Flags: review?(jib) → review+
Comment on attachment 8754380 [details]
MozReview Request: Bug 1271669 - Remove test_peerConnection_addTrack.html. r?jib

https://reviewboard.mozilla.org/r/53930/#review50706
Attachment #8754380 - Flags: review?(jib) → review+
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
Attachment #8754381 - Flags: review?(jib) → review+
Comment on attachment 8754374 [details]
MozReview Request: Bug 1271669 - Change AnyLocalStreamHasPeerIdentity to be per-track. r?mt

https://reviewboard.mozilla.org/r/53918/#review50752
Attachment #8754374 - Flags: review?(martin.thomson) → review+
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.
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/
Attachment #8754379 - Attachment description: MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib, r?smaug → MozReview Request: Bug 1271669 - Remove wallpaper constraints to the MediaStream argument to RTCPeerConnection.addTrack(). r?jib
Attachment #8754711 - Flags: review?(jib)
Attachment #8754711 - Flags: review?(bugs)
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 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 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 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 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 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 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 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 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/
Attachment #8754711 - Flags: review?(bugs) → review+
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 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.
Attachment #8754711 - Flags: review?(jib) → review+
(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.
Documentation updated:

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

Also updated Firefox 49 for developers.
What about minor of Firefox version 48. Can Firefox v48.1 or v48.2 fix this bug?
You need to log in before you can comment on or make changes to this bug.