Closed Bug 1208328 Opened 9 years ago Closed 8 years ago

Implement addtrack event for MediaStream

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Version:
33 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Bug 1208328 - Add test for "addtrack" on recv side of RTCPeerConnection.
58 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208328 - Test that "addtrack" and "removetrack" events don't occur on manual operations.
58 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1208328 - Implement MediaStream.onaddtrack.
58 bytes, text/x-review-board-request
smaug
: review+
jib
: review+
Details
Bug 1208328 - Don't block ended tracks on removal from MediaStream.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1208328 - Test MediaStreamTrackEvent like RTCTrackEvent.
58 bytes, text/x-review-board-request
smaug
: review+
Details 
When tracks are removed by a MediaStream's source they shall be removed also in the MediaStream and a "removetrack" event shall be fired.
Likewise, "addtrack" shall be fired for tracks added by the source.
Rank: 25
Priority: -- → P2
Bump
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Some notes from implementing this:

* mediacapture-main [1] doesn't specify when "addtrack"/"removetrack" should be raised, other than that other specifications may define when to raise them.


To my knowledge, two specifications define when "addtrack" is raised:

* webrtc-pc [2] raises "addtrack" when the remote side of a peer connection adds a track to an existing stream. i.e., after renegotiation.

* mediacapture-fromelement [3] raises "addtrack" when the selected VideoTrack or enabled AudioTracks change. We don't implement this API to spec currently.


Also, one specification defines when "removetrack" is raised:

* mediacapture-fromelement [3] raises "removetrack" when the selected VideoTrack or enabled AudioTracks change, indirectly when a source changes or is reset (that causes VideoTrack and AudioTrack changes as well). We don't implement this API to spec currently.


---


Since we're not up to spec on HTMLMediaElement.captureStream() (prefixed + far behind the current spec) I intend to add the "addtrack" event for tracks it adds dynamically. Adding the "removetrack" event would mean changing our implementation to actually remove the tracks (not just end them) so I'll leave that to future work on media element capture.

I intend to implement "addtrack" for received tracks on a peer connection.

Overall there's no need to implement "removetrack" at this time since there are no users. I'll leave it to future work.


[1] http://w3c.github.io/mediacapture-main/getusermedia.html#mediastream
[2] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#processing-remote-mediastreamtracks
[3] https://w3c.github.io/mediacapture-fromelement/index.html#methods
Summary: Implement addtrack/removetrack events for MediaStreams → Implement addtrack event for MediaStream
I based this work on bug 1208373, so tests will only pass if this lands after.
Depends on: 1208373
After ending the MediaInputPort (to the playback stream) gets removed if locked
to the track specifically. Trying to remove the track at this point would try to
block it in the same MediaInputPort - but since it doesn't exist would trigger
an NS_ERROR instead.

Review commit: https://reviewboard.mozilla.org/r/56560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56560/
Comment on attachment 8758253 [details]
Bug 1208328 - Implement MediaStream.onaddtrack.

https://reviewboard.mozilla.org/r/56558/#review53110

But I see that other browsers have just .track, so hopefully it is ok.

::: dom/webidl/MediaStreamTrackEvent.webidl
(Diff revision 1)
> -[Pref="media.peerconnection.enabled",
> - Constructor(DOMString type, optional MediaStreamTrackEventInit eventInitDict)]
> +[Exposed=Window,
> + Constructor (DOMString type, MediaStreamTrackEventInit eventInitDict)]
>  interface MediaStreamTrackEvent : Event {
> -  readonly attribute RTCRtpReceiver? receiver;
> -  readonly attribute MediaStreamTrack? track;
> +    [SameObject]
> +    readonly        attribute MediaStreamTrack track;
> -  readonly attribute MediaStream? stream;

Why can we remove this attribute from the event? That is a backwards incompatible change.
Same question about receiver.
Attachment #8758253 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8758253 [details]
> MozReview Request: Bug 1208328 - Implement MediaStream.onaddtrack. r?jib,
> r?smaug
> 
> https://reviewboard.mozilla.org/r/56558/#review53110
> 
> But I see that other browsers have just .track, so hopefully it is ok.
> 
> ::: dom/webidl/MediaStreamTrackEvent.webidl
> (Diff revision 1)
> > -[Pref="media.peerconnection.enabled",
> > - Constructor(DOMString type, optional MediaStreamTrackEventInit eventInitDict)]
> > +[Exposed=Window,
> > + Constructor (DOMString type, MediaStreamTrackEventInit eventInitDict)]
> >  interface MediaStreamTrackEvent : Event {
> > -  readonly attribute RTCRtpReceiver? receiver;
> > -  readonly attribute MediaStreamTrack? track;
> > +    [SameObject]
> > +    readonly        attribute MediaStreamTrack track;
> > -  readonly attribute MediaStream? stream;
> 
> Why can we remove this attribute from the event? That is a backwards
> incompatible change.
> Same question about receiver.

I went through dxr looking for uses of MediaStreamTrackEvent and couldn't find any. Now I see we have one, quote "legacy", use in a js file at [1]. That is still only using .track so I believe we're safe. It seems like the other attributes went on to their own event RTCTrackEvent, see [2].

jib, do you know more about MediaStreamTrackEvent's history? Can we safely scrap .streams and .receiver?


[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/media/PeerConnection.js#1529
[2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/RTCTrackEvent.webidl
Flags: needinfo?(jib)
Well, the safety doesn't come only from dispatching events without setting those attributes, since it is also the existence of the attributes in the interface object which adds some compatibility risk.
Some code might do something like 
if ("stream" in MediaStreamTrackEvent.prototype) {
  ...
}

But I was looking at blinks implementation and they seem to have just track. 
Does Edge have this event? What does it look like?
Just tried Edge. It only has "track".

> console.log(MediaStreamTrackEvent.prototype)
> [object MediaStreamTrackEventPrototype]
>    {
>       [functions]: ,
>       __proto__: { },
>       AT_TARGET: 2,
>       bubbles: <Permission denied>,
>       BUBBLING_PHASE: 3,
>       cancelable: <Permission denied>,
>       cancelBubble: <Permission denied>,
>       CAPTURING_PHASE: 1,
>       currentTarget: <Permission denied>,
>       defaultPrevented: <Permission denied>,
>       eventPhase: <Permission denied>,
>       isTrusted: <Permission denied>,
>       returnValue: <Permission denied>,
>       srcElement: <Permission denied>,
>       target: <Permission denied>,
>       timeStamp: <Permission denied>,
>       track: <Permission denied>,
>       type: <Permission denied>
>    }
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> jib, do you know more about MediaStreamTrackEvent's history? Can we safely
> scrap .streams and .receiver?

I don't think there's much more, so it seems safe to scrap.
Flags: needinfo?(jib)
Attachment #8758251 - Flags: review?(jib) → review+
Comment on attachment 8758251 [details]
Bug 1208328 - Add test for "addtrack" on recv side of RTCPeerConnection.

https://reviewboard.mozilla.org/r/56554/#review53828

::: dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html:31
(Diff revision 1)
> +          let newTrack = stream.getTracks()[0];
> +
> +          let videoSenderIndex =
> +            test.pcLocal._pc.getSenders().findIndex(s => s.track.kind == "video");
> +          isnot(videoSenderIndex, -1, "Should have video sender");
> +          let oldTrack = test.pcLocal._pc.getSenders()[videoSenderIndex];

Nit: oldTrack is a sender, and unused.

::: dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html:51
(Diff revision 1)
> +          eventsPromise = expectedTrackEvent(remoteStream, "addtrack", trackEvent => {
> +            is(trackEvent.track.id, newTrack.id, "Expected track in event");
> +            is(trackEvent.track.readyState, "live",
> +               "added track should be live");
> +          });

What are the criteria for which checks go inside the expectedTrackEvent function vs. outside?

::: dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html:64
(Diff revision 1)
> +      function PC_REMOTE_CHECK_EVENTS(test) {
> +        return eventsPromise;
> +      },

Why not return this promise from the previous function?
Comment on attachment 8758252 [details]
Bug 1208328 - Test that "addtrack" and "removetrack" events don't occur on manual operations.

https://reviewboard.mozilla.org/r/56556/#review53836

lgtm.

::: dom/media/tests/mochitest/test_getUserMedia_addtrack_removetrack_events.html:13
(Diff revision 1)
> +<script type="application/javascript">
> +"use strict";
> +
> +createHTML({
> +  title: "MediaStream's 'addtrack' and 'removetrack' events shouldn't fire on manual operations",
> +  bug: "1103188"

wrong bug #?
Attachment #8758252 - Flags: review?(jib) → review+
Comment on attachment 8758253 [details]
Bug 1208328 - Implement MediaStream.onaddtrack.

https://reviewboard.mozilla.org/r/56558/#review53844

lgtm.

::: dom/media/DOMMediaStream.cpp:1238
(Diff revision 1)
> +  MOZ_ASSERT(aName == NS_LITERAL_STRING("addtrack"),
> +             "Only 'addtrack' is supported at this time");

Nit: What invariant does this assert protect?
Attachment #8758253 - Flags: review?(jib) → review+
Attachment #8758254 - Flags: review?(rjesup) → review+
https://reviewboard.mozilla.org/r/56554/#review53828

> Nit: oldTrack is a sender, and unused.

Right, it's from an early version when I tested for "removetrack" as well. Then it turned out it wasn't specced out for PeerConnection.

> What are the criteria for which checks go inside the expectedTrackEvent function vs. outside?

This is also from when I tested "removetrack". Stuff that was shared (event type) went inside, rest outside. I'll inline it now.

> Why not return this promise from the previous function?

From templates.js, `addRenegotiation()`: The steps in `commandsPeerConnectionOfferAnswer` (basically doing the renegotiation) are done in between. They are needed to resolve the promise.
https://reviewboard.mozilla.org/r/56558/#review53844

> Nit: What invariant does this assert protect?

It's to make it clear that we're only calling DispatchTrackEvent() with "addtrack" at the moment (or really that we're not implementing "removetrack").
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce08f664f9c
Add test for "addtrack" on recv side of RTCPeerConnection. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/22150caec0d2
Test that "addtrack" and "removetrack" events don't occur on manual operations. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0529f6b073
Implement MediaStream.onaddtrack. r=jib, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb4b83be583
Don't block ended tracks on removal from MediaStream. r=jesup
Review commit: https://reviewboard.mozilla.org/r/58940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58940/
Attachment #8758251 - Attachment description: MozReview Request: Bug 1208328 - Add test for "addtrack" on recv side of RTCPeerConnection. r?jib → Bug 1208328 - Add test for "addtrack" on recv side of RTCPeerConnection.
Attachment #8758252 - Attachment description: MozReview Request: Bug 1208328 - Test that "addtrack" and "removetrack" events don't occur on manual operations. r?jib → Bug 1208328 - Test that "addtrack" and "removetrack" events don't occur on manual operations.
Attachment #8758253 - Attachment description: MozReview Request: Bug 1208328 - Implement MediaStream.onaddtrack. r?jib, r?smaug → Bug 1208328 - Implement MediaStream.onaddtrack.
Attachment #8758254 - Attachment description: MozReview Request: Bug 1208328 - Don't block ended tracks on removal from MediaStream. r?jesup → Bug 1208328 - Don't block ended tracks on removal from MediaStream.
Attachment #8761979 - Flags: review?(bugs)
Comment on attachment 8758251 [details]
Bug 1208328 - Add test for "addtrack" on recv side of RTCPeerConnection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56554/diff/1-2/
Comment on attachment 8758252 [details]
Bug 1208328 - Test that "addtrack" and "removetrack" events don't occur on manual operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56556/diff/1-2/
Comment on attachment 8758253 [details]
Bug 1208328 - Implement MediaStream.onaddtrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56558/diff/1-2/
Comment on attachment 8758254 [details]
Bug 1208328 - Don't block ended tracks on removal from MediaStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56560/diff/1-2/
Comment on attachment 8761979 [details]
Bug 1208328 - Test MediaStreamTrackEvent like RTCTrackEvent.

https://reviewboard.mozilla.org/r/58940/#review55860

::: dom/media/tests/mochitest/test_getUserMedia_addtrack_removetrack_events.html:103
(Diff revision 1)
>  
>      return spinEventLoop();
> +  })
> +  .then(() => {
> +      // Test MediaStreamTrackEvent required args here.
> +      mustThrowWith("MediaStreamTrackEvent without required args",

ok, I guess mustThrowWith means that when the last param is executed, the error mentioned as 2nd param should be thrown.
(odd ordering in params)
Attachment #8761979 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/58940/#review55860

> ok, I guess mustThrowWith means that when the last param is executed, the error mentioned as 2nd param should be thrown.
> (odd ordering in params)

Correct.
Flags: needinfo?(pehrson)
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48da132353c7
Add test for "addtrack" on recv side of RTCPeerConnection. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/945141b1f8da
Test that "addtrack" and "removetrack" events don't occur on manual operations. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/d48bc6eca9c3
Implement MediaStream.onaddtrack. r=jib, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa209325161f
Don't block ended tracks on removal from MediaStream. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/3160ce8f8759
Test MediaStreamTrackEvent like RTCTrackEvent. r=smaug
Backed out this patch stack for frequent failures in test_peerConnection_addtrack_removetrack_events.html on Android 4.3 debug:

bug 1208373: https://hg.mozilla.org/integration/mozilla-inbound/rev/d433bc994ae2885a7269df4ef0969af89888c1ee
bug 1274221: https://hg.mozilla.org/integration/mozilla-inbound/rev/57453dbd063c0711348d6631cd7fc330346ef5f0
bug 1208328: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8017df8752dc1bb7862dbdfe2fc0c99bfa2c146

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30078129&repo=mozilla-inbound
09:27:59     INFO -  164 INFO Run step 80: PC_REMOTE_VERIFY_ICE_GATHERING
09:27:59     INFO -  165 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) received local trickle ICE candidates
09:27:59     INFO -  166 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) ICE gathering state is not 'new'
09:27:59     INFO -  167 INFO Run step 81: PC_LOCAL_WAIT_FOR_MEDIA_FLOW
09:27:59     INFO -  168 INFO Checking data flow to element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  169 INFO Checking data flow to element: pcLocal_local_{b133a0dc-5654-48b0-b332-8ce7a4021329}
09:27:59     INFO -  170 INFO Checking RTP packet flow for track {6f20246f-4b74-4d72-b1e7-0770749f4d15}
09:27:59     INFO -  171 INFO Checking RTP packet flow for track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96}
09:27:59     INFO -  172 INFO Element pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262} saw 'timeupdate', currentTime=65.03619047619047s, readyState=4
09:27:59     INFO -  173 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Media flowing for element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.


174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
267 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
321 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
323 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
330 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
342 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Number of ICE connections according to stats is not zero - Result logged after SimpleTest.finish()
343 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | stats reports exactly 1 ICE connection - Result logged after SimpleTest.finish()
344 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
345 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
346 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
347 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
PROCESS-CRASH | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | application crashed [@ mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::~Log]
06-13 09:30:13.895 F/MOZ_Assert( 2104): Assertion failure: false (An assert from the graphics logger), at /builds/slave/m-in-and-api-15-d-000000000000/build/src/gfx/2d/Logging.h:519
Status: RESOLVED → REOPENED
Flags: needinfo?(pehrson)
Resolution: FIXED → ---
Flags: needinfo?(pehrson)
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e39947767d4
Add test for "addtrack" on recv side of RTCPeerConnection. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/628f41d0df69
Test that "addtrack" and "removetrack" events don't occur on manual operations. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8ba0b71eb0
Implement MediaStream.onaddtrack. r=jib, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8bcd0ffe5e
Don't block ended tracks on removal from MediaStream. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/9027f0781612
Test MediaStreamTrackEvent like RTCTrackEvent. r=smaug
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: