1232082 - Add RTCRtpReceiver and fire ontrack for remote tracks

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Comment 1

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

Attachment: MozReview Request: Bug 1232082 - add RTCRtpReceiver for each remote track.

Bug 1232082 - add RTCRtpReceiver for each remote track (WIP).

Comment 2

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

Comment on attachment 8697737 [details]
MozReview Request: Bug 1232082 - add RTCRtpReceiver for each remote track.

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

Comment 3

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

FYI, needed for Simulcast work

Comment 4

[Nils Ohlmeier [:drno]]

Just added jib's patch from this bug to the test from bug 1231507 and it works. So at least we get RTPReceivers and Byron's code for switching between incoming RTP streams seems to be happy about it.

Comment 5

[Nils Ohlmeier [:drno]]

@jesup: FYI I think this does not need an extra test per se, because it is already covered through existing code here: https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1432

Comment 6

[Nils Ohlmeier [:drno]]

The only code which seems to be "nice to have" fixed when this lands is: https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1455

Comment 7

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

Comment on attachment 8697737 [details]
MozReview Request: Bug 1232082 - add RTCRtpReceiver for each remote track.

https://reviewboard.mozilla.org/r/27709/#review26007

r+, should land with other simulcast code (simulcast mochitests) which will implicitly test it

Comment 8

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

Comment on attachment 8697737 [details]
MozReview Request: Bug 1232082 - add RTCRtpReceiver for each remote track.

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

Comment 9

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

Attachment: MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

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

Comment 10

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

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

Comment 11

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

https://reviewboard.mozilla.org/r/29503/#review26317

::: dom/webidl/RTCTrackEvent.webidl:10
(Diff revision 1)
> +dictionary RTCTrackEventInit : EventInit {
> +    required RTCRtpReceiver        receiver;
> +    required MediaStreamTrack      track;
> +    required sequence<MediaStream> streams;

See https://github.com/w3c/webrtc-pc/pull/446

Comment 12

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

https://reviewboard.mozilla.org/r/29503/#review26319

Looks good; my only question is do we need to retain tests for the deprecated interfaces until they're removed?  I'd think so....

Comment 13

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

I did not change all tests, so there are tests remaining of the deprecated interfaces. In fact, I gave up fixing pc.js (since the stream assumptions in setupAddStreamEventHandler and attachMedia started to fan out the more I touched them), so most tests effectively still rely on onaddstream for now.

Comment 14

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

https://reviewboard.mozilla.org/r/29503/#review26359

Comment 15

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

Thanks for comment 11.

Comment 16

[Byron Campen [:bwc]]

Any luck figuring out the oranges on that try run?

Comment 17

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

Yes, I addressed those in comment 10, but weirdly the link in comment 10 is still in "draft" stage in mozReview somehow, e.g. if I use private browsing and follow the link then it shows as "page missing". Odd.

In any case, I'll publish it now and do another try.

Comment 18

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

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

Comment 19

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

See, the link in comment 18 is the same as in comment 10. Straange.

Comment 20

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

Sorry, still working on the other orange.

Comment 21

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

Comment on attachment 8697737 [details]
MozReview Request: Bug 1232082 - add RTCRtpReceiver for each remote track.

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

Comment 22

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

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

Comment 23

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

Attachment: MozReview Request: Bug 1232082 - fix removal of remote tracks to update receivers.

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

Comment 24

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

Note: comment 21 and comment 22 have zero changes (I wish mozReview would quit spamming needlessly).

Comment 25

[Byron Campen [:bwc]]

Comment on attachment 8706736 [details]
MozReview Request: Bug 1232082 - fix removal of remote tracks to update receivers.

https://reviewboard.mozilla.org/r/30485/#review27285

::: dom/media/PeerConnection.js:1490
(Diff revision 1)
> -      this._receivers.splice(i, 1);
> +      pc._receivers.splice(i, 1);

This was the primary fix on the PeerConnection.js side of things, right?

::: dom/media/PeerConnection.js
(Diff revision 1)
> -    this.dispatchEvent(new this._dompc._win.MediaStreamTrackEvent("removetrack",
> -                                                                  { track: track }));

I guess we're removing this since we've never supported it, and it isn't in the latest editor's draft anyway?

Comment 26

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

> This was the primary fix on the PeerConnection.js side of things, right?

Right.

> > -    this.dispatchEvent(new this._dompc._win.MediaStreamTrackEvent("removetrack",
> > -                                                                  { track: track }));
> 
> I guess we're removing this since we've never supported it, and it isn't in
> the latest editor's draft anyway?

Right. onremovetrack was never in the WebIDL, so it never worked.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a595577a8247
https://hg.mozilla.org/integration/mozilla-inbound/rev/47de4d036349
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec89afa7163c

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d13beb8320e9 for wpt bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=19661266&repo=mozilla-inbound

Comment 29

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

Comment on attachment 8703941 [details]
MozReview Request: Bug 1232082 - add pc.ontrack and RTCTrackEvent

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

Comment 30

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

Comment on attachment 8706736 [details]
MozReview Request: Bug 1232082 - fix removal of remote tracks to update receivers.

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

Comment 31

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

(In reply to Wes Kocher (:KWierso) from comment #28)
> wpt bustage:

Forgot about those. :/ Thanks. Comment 29 has a fix (comment 30 has zero changes - mozReview artifact). Carrying forward r=jesup,smaug.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3363702d51
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ec4008871e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8afc8ea1c1

Comment 33

[Phil Ringnalda (:philor)]

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a91621bad6 for a variety of Android webrtc failures, most prominently causing bug 1239200.

Comment 34

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

What were the other ones? I only see bug 1239200 in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1c8afc8ea1c1&selectedJob=19684724

Comment 35

[Phil Ringnalda (:philor)]

What you can do to (at least try to) answer that is to take your cset hash from landing and mine from backing out and stick them into &fromchange=/&tochange= to get https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=fa3363702d51&tochange=a8a91621bad6, then click the Filters menu, "click new to filter by a specific job field" and add "failure classification, fixed by commit" to get https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=fa3363702d51&tochange=a8a91621bad6&filter-failure_classification_id=2

But what that'll tell you is just that it was probably bug 1239200 plus I starred a couple of webrtc intermittents in there as being caused by this when they were really just coincidences.

Comment 36

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

Attachment: Disable test_peerConnection_addDataChannelNoBundle.html on android debug.

Having mozReview trouble (bug 1240257) so I'm stuck with splinter atm.

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a560abedc6f

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7671c54fff
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a339251da3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8552a15e04
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9c06478275

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb7671c54fff
https://hg.mozilla.org/mozilla-central/rev/81a339251da3
https://hg.mozilla.org/mozilla-central/rev/cd8552a15e04
https://hg.mozilla.org/mozilla-central/rev/0b9c06478275