Closed
Bug 1208373
Opened 9 years ago
Closed 8 years ago
Implement MediaStreamTrack.readyState/onended
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: pehrsons, Assigned: pehrsons)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(9 files, 1 obsolete file)
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 |
58 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jib
:
review+
smaug
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jesup
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jesup
:
review+
|
Details |
The readyState can be `live` or `ended`. The "ended" event is raised when the readyState goes from `live` to `ended`.
Comment 1•9 years ago
|
||
I'd like to add two more use cases to this to consider when testing it ... 1) if the user unplugs a microphone or camera when it is in active use by GUM, the state needs to go to ended and the onended callback run. 2) if the page the users goes to the browser chrome and revokes permission to share a camera or microphone that is in use, the stream also needs to go to ended.
Updated•9 years ago
|
Rank: 25
Priority: -- → P2
Comment 2•8 years ago
|
||
It's time to prioritize this higher and get it done.
Rank: 25 → 15
Priority: P2 → P1
Comment 3•8 years ago
|
||
Jan-Ivar, Andreas -- One of you would be a great choice as bug owner for this one. Any interest? I'd like to get this in sooner than later. Fx48 is probably too much of a stretch, but how about targeting Fx49?
Flags: needinfo?(pehrsons)
Flags: needinfo?(jib)
Assignee | ||
Comment 4•8 years ago
|
||
I think it's mostly easy, but I'm a bit scared of HTMLMediaElement and the refactoring we'll have to do wrt active streams (media element shall now end when stream gets inactive (= no live tracks) instead of on the stream's own "ended" event - which we need to kill). I hope and think we'll find that MediaStreamTrack.onended can be implemented before that refactor however. I can take it on, but first I'm gonna land bug 1208371.
Flags: needinfo?(pehrsons)
Comment 6•8 years ago
|
||
Thanks, Andreas. I'll assign this to you now, but I realize you won't start work until cloning is landed.
Assignee: nobody → pehrsons
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49999/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49999/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50001/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50001/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50003/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50003/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50005/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50005/
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/1-2/
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/1-2/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50469/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50469/
Attachment #8747702 -
Attachment description: MozReview Request: Bug 1208373 - Test that a peerConnection's tracks end on close(). r?jib → MozReview Request: Bug 1208373 - Test that a peerConnection's received tracks end on close(). r?jib
Attachment #8747703 -
Attachment description: MozReview Request: Bug 1208373 - Add test for MediaStreamTrack "ended" event. r?jib → MozReview Request: Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. r?jib
Attachment #8748675 -
Flags: review?(jib)
Attachment #8748676 -
Flags: review?(rjesup)
Attachment #8748677 -
Flags: review?(jib)
Attachment #8748677 -
Flags: review?(bugs)
Attachment #8748678 -
Flags: review?(jib)
Attachment #8747701 -
Flags: review?(jib)
Attachment #8747702 -
Flags: review?(jib)
Attachment #8747703 -
Flags: review?(jib)
Attachment #8747704 -
Flags: review?(jib)
Attachment #8747704 -
Flags: review?(bugs)
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50471/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50471/
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50473/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50473/
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50475/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50475/
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/1-2/
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/1-2/
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/2-3/
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/2-3/
Assignee | ||
Comment 21•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d028f504cd99
Comment 22•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup https://reviewboard.mozilla.org/r/50471/#review47237
Attachment #8748676 -
Flags: review?(rjesup) → review+
Comment 23•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). https://reviewboard.mozilla.org/r/49999/#review47243 lgtm with one issue fixed. ::: dom/media/tests/mochitest/mediaStreamPlayback.js:58 (Diff revision 2) > - // TODO (bug 910249) Also check that all the tracks are local. > - this.mediaStream.getTracks().forEach(t => t.stop()); > + this.mediaStream.getTracks().filter(t => t.readyState == "live").forEach(t => { > + t.addEventListener("ended", function endedTrackUnexpected() { > + ok(false, "Unexpected ended event for track " + t.id); > + }); > + t.stop(); > + is(t.readyState, "ended", "Stopped track " + t.id + " should be ended"); > + }); This library function should remove the event listeners it adds. Since it's verifying that events are *not* fired, this may require refactoring a bit to give it time to fire before removing the event listeners. Would prefer this happened before the returned promise is resolved. There's also a change here where .stop is now only called on live tracks where before it was called on all tracks. I suppose this is ok.
Attachment #8747701 -
Flags: review?(jib)
Comment 24•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). https://reviewboard.mozilla.org/r/50001/#review47259 ::: dom/media/tests/mochitest/pc.js:136 (Diff revision 2) > + .map(receiver => timerGuard(new Promise(resolve => { > + info("Waiting for track " + receiver.track.id + " (" + > + receiver.track.kind + ") to end."); > + receiver.track.addEventListener("ended", function trackEnded() { > + info("Track " + receiver.track.id + " ended."); > + is(receiver.track.readyState, "ended", > + "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event"); > + resolve(); > + }) > + }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end") I prefer promise constructor executor functions to be as minimal as possible. How about a central helper for this repeating pattern: var haveEvent = (target, name) => new Promise(resolve => target.addEventListener(name, function listener(event) { target.removeEventListener(name, listener); resolve(event); })); then use it here like this: .map(receiver => { info("Waiting for track " + receiver.track.id + " (" + receiver.track.kind + ") to end."); return timerGuard(haveEvent(receiver.track, "ended").then(e => { info("Track " + receiver.track.id + " ended."); is(receiver.track.readyState, "ended", "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event"); }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end"); }); That takes care of removing the listener as well. ::: dom/media/tests/mochitest/pc.js:467 (Diff revision 2) > + .then(() => finished()) > + .catch(e => { > - ok(false, 'Error in test execution: ' + e + > + ok(false, 'Error in test execution: ' + e + > - ((typeof e.stack === 'string') ? > + ((typeof e.stack === 'string') ? > - (' ' + e.stack.split('\n').join(' ... ')) : ''))); > + (' ' + e.stack.split('\n').join(' ... ')) : '')); > + finished(); > + }); This seems like it would run finished() twice if there's ever a coding error in finished. Not sure what problem you experienced here, but I would have two catch statements instead, one before finished and one after.
Attachment #8747702 -
Flags: review?(jib)
Comment 25•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. https://reviewboard.mozilla.org/r/50003/#review47265 ::: dom/media/tests/mochitest/test_getUserMedia_trackEnded.html:25 (Diff revision 3) > + return new Promise((resolve, reject) => > + iframe.contentDocument.gUM({audio: true, video: true}, resolve, reject)) A comment on the 10-foot pole here might be nice to explain the iframe promise-chain problem you ran into. ::: dom/media/tests/mochitest/test_getUserMedia_trackEnded.html:31 (Diff revision 3) > + return new Promise(resolve => t.onended = e => { > + info("onended handler invoked for track " + t.id); > + is(e.target, t, "Target should be correct"); > + resolve(); > + }); I'd prefer: return new Promise(resolve => t.onended = resolve).then(e => { info("onended handler invoked for track " + t.id); is(e.target, t, "Target should be correct"); });
Attachment #8747703 -
Flags: review?(jib) → review+
Comment 26•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. https://reviewboard.mozilla.org/r/50469/#review47273
Attachment #8748675 -
Flags: review?(jib) → review+
Comment 27•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. https://reviewboard.mozilla.org/r/50005/#review47277 lgtm. ::: dom/media/MediaStreamTrack.cpp:354 (Diff revision 3) > + NS_DispatchToMainThread(NewRunnableFrom([self]() { > + MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended"))); I suppose that's cleaner than: NS_DispatchToMainThread(NewRunnableFrom([this, self]() { MOZ_ALWAYS_SUCCEEDS(DispatchTrustedEvent(NS_LITERAL_STRING("ended")));
Attachment #8747704 -
Flags: review?(jib)
Comment 28•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. https://reviewboard.mozilla.org/r/50005/#review47279
Attachment #8747704 -
Flags: review+
Comment 29•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. https://reviewboard.mozilla.org/r/50473/#review47281 ::: dom/media/MediaStreamTrack.h:269 (Diff revision 1) > + /** > + * Forces the ready state to a particular value, for instance when we're > + * cloning an already ended track. > + */ > + void SetReadyState(MediaStreamTrackState aState) { mReadyState = aState; } For encapsulation it seems perhaps a bit unnecessary to expose these setters just for cloning, when CloneInternal (or some central version of it) theoretically could have taken care of this.
Attachment #8748677 -
Flags: review?(jib) → review+
Updated•8 years ago
|
Attachment #8748678 -
Flags: review?(jib) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. https://reviewboard.mozilla.org/r/50475/#review47289 ::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html:79 (Diff revision 1) > // Handle both MediaErrors (with the `code` attribute) and other errors. > .catch(e => ok(false, "Error (" + e + ")" + > (e.code ? " (code=" + e.code + ")" : "") + > " in test for " + media.name + > (e.stack ? ":\n" + e.stack : ""))) > + .then(() => test ? test.close() : null) null seems arbitrary. Why not: .then(() => test && test.close()) ::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html (Diff revision 1) > (e.code ? " (code=" + e.code + ")" : "") + > " in test for " + media.name + > (e.stack ? ":\n" + e.stack : ""))) > + .then(() => test ? test.close() : null) > .then(() => { > - if (test) { test.close(); } Gah, that's what we get when we overload similar-sounding APIs with different semantics (not your fault, but gah).
Comment 31•8 years ago
|
||
Overall lgtm and works. Nice! Probably should've just r+ed the two remaining tests, just figured I'd see them again if you go for the changes I suggested. Let me know.
Comment 32•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). https://reviewboard.mozilla.org/r/50001/#review47295
Attachment #8747702 -
Flags: review+
Comment 33•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). https://reviewboard.mozilla.org/r/49999/#review47297
Attachment #8747701 -
Flags: review+
Assignee | ||
Comment 34•8 years ago
|
||
https://reviewboard.mozilla.org/r/49999/#review47243 > This library function should remove the event listeners it adds. > > Since it's verifying that events are *not* fired, this may require refactoring a bit to give it time to fire before removing the event listeners. Would prefer this happened before the returned promise is resolved. > > There's also a change here where .stop is now only called on live tracks where before it was called on all tracks. I suppose this is ok. I'll call it on all tracks again. stop() should be ignored if a track is ended and we'd like to walk that path. I am now removing the listeners. I'll let you review again, I put in an arbitrary delay of half a second.
Assignee | ||
Comment 35•8 years ago
|
||
https://reviewboard.mozilla.org/r/50001/#review47259 > I prefer promise constructor executor functions to be as minimal as possible. > > How about a central helper for this repeating pattern: > > var haveEvent = (target, name) => new Promise(resolve => > target.addEventListener(name, function listener(event) { > target.removeEventListener(name, listener); > resolve(event); > })); > > then use it here like this: > > .map(receiver => { > info("Waiting for track " + receiver.track.id + " (" + > receiver.track.kind + ") to end."); > return timerGuard(haveEvent(receiver.track, "ended").then(e => { > info("Track " + receiver.track.id + " ended."); > is(receiver.track.readyState, "ended", > "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event"); > }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end"); > }); > > That takes care of removing the listener as well. In case we time out instead of see the event, we don't remove the listener and might log results (the is()) after the test has finished. Thoughts on that?
Assignee | ||
Comment 36•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/2-3/
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/2-3/
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/3-4/
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/1-2/
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/1-2/
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/3-4/
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/1-2/
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8747701 -
Flags: review+ → review?(jib)
Assignee | ||
Updated•8 years ago
|
Attachment #8747702 -
Flags: review+ → review?(jib)
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. https://reviewboard.mozilla.org/r/50005/#review48401 ::: dom/media/MediaStreamTrack.cpp:358 (Diff revision 4) > + RefPtr<MediaStreamTrack> self = this; > + NS_DispatchToMainThread(NewRunnableFrom([self]() { > + MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended"))); > + return NS_OK; > + })); > + (new AsyncEventDispatcher(this, NS_LITERAL_STRING("ended"), false))->PostDOMEvent(); is simpler and less weird since it doesn't rely on lambdas.
Attachment #8747704 -
Flags: review?(bugs) → review+
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. https://reviewboard.mozilla.org/r/50473/#review48433 ::: dom/media/MediaStreamTrack.cpp:361 (Diff revision 2) > NS_DispatchToMainThread(NewRunnableFrom([self]() { > MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended"))); > return NS_OK; > })); > > - mEnded = true; > + mReadyState = MediaStreamTrackState::Ended; this is not what the spec says should happen. readyState should be set 'ended' during the same task in which the "ended" event is fired, in other words, queue a task which first sets readyState to 'ended' and then _synchronously_ dispatch simple event named "ended". This difference between the patch and the spec is minor but observable from scripts.
Attachment #8748677 -
Flags: review?(bugs)
Attachment #8748677 -
Flags: review-
That means that my suggestion to use AsyncEventDispatcher won't quite work. I would just have a method in MediaStreamTrack to set the readyState and then dispatch event synchronously and use plain normal runnable method trigger it, http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#281
Assignee | ||
Comment 47•8 years ago
|
||
https://reviewboard.mozilla.org/r/50473/#review48433 > this is not what the spec says should happen. > readyState should be set 'ended' during the same task in which the "ended" event is fired, in other words, queue a task which first sets readyState to 'ended' and then _synchronously_ dispatch simple event named "ended". > This difference between the patch and the spec is minor but observable from scripts. Aye, good catch. I'll fix.
Assignee | ||
Comment 48•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/3-4/
Attachment #8748677 -
Flags: review- → review?(bugs)
Assignee | ||
Comment 49•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/3-4/
Assignee | ||
Comment 50•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/4-5/
Assignee | ||
Comment 51•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/2-3/
Assignee | ||
Comment 52•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/2-3/
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/4-5/
Assignee | ||
Comment 54•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/2-3/
Assignee | ||
Comment 55•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/2-3/
Comment 56•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #45) > > NS_DispatchToMainThread(NewRunnableFrom([self]() { > > MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended"))); > > return NS_OK; > > })); FWIW I actually thought this is a great use of a lambda. Very readable and flexible (the fact that you can fix which task mReadyState is set from by simply moving a line, seems proof of the latter). I agree NewRunnableMethod() is another good pattern sometimes, but limited to only calling methods without arguments AFAICT, so if you end up defining a method you otherwise don't need just to use it, then it's not a good fit IMHO. [1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=114ca1fc9c51#753
Lambda is rather error prone, which is why we have checks to ensure refcounted objects are handled properly. Though, I guess those checks remove the most obvious security hazards. RunnableMethod can take arguments. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=114ca1fc9c51#794
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. https://reviewboard.mozilla.org/r/50473/#review48461
Attachment #8748677 -
Flags: review?(bugs) → review+
Comment 59•8 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #35) > > How about a central helper for this repeating pattern: > > In case we time out instead of see the event, we don't remove the listener > and might log results (the is()) after the test has finished. Thoughts on > that? Good point. Here's my second attempt: var haveEvent = (target, name, cancelPromise) => { var listener; var p = Promise.race([ (cancelPromise || new Promise()).then(Promise.reject), new Promise(resolve => target.addEventListener(name, listener = resolve)) ]); p.then(() => target.removeEventListener(name, listener)); return p; }; Example usage: haveEvent(receiver.track, "ended", wait(500)) .then(event => info("ended fired"), e => e? Promise.reject(e) : info("ended never fired.")) This is basically a cancel pattern, where I'm passing in a cancel signal in the form of another promise.
Comment 60•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). https://reviewboard.mozilla.org/r/49999/#review48459 ::: dom/media/tests/mochitest/mediaStreamPlayback.js:58 (Diff revisions 2 - 4) > - this.mediaStream.getTracks().filter(t => t.readyState == "live").forEach(t => { > - t.addEventListener("ended", function endedTrackUnexpected() { > - ok(false, "Unexpected ended event for track " + t.id); > - }); > + var noTrackEnded = Promise.all(this.mediaStream.getTracks().map(t => new Promise((resolve, reject) => { > + let endedTrackUnexpected = () => > + (cleanup(), reject(new Error("Unexpected ended event for track " + t.id))); > + let cleanup = () => t.removeEventListener("ended", endedTrackUnexpected); > + > + t.addEventListener("ended", endedTrackUnexpected); > + wait(500) > + .then(() => cleanup()) > + .then(() => ok(true, "Ended event did not occur after stop() for any track")) > + .then(() => resolve()); > + > t.stop(); > is(t.readyState, "ended", "Stopped track " + t.id + " should be ended"); > - }); > + }))); This is still the promise constructor anti-pattern [1] which is hard to read and I think we should avoid. Exhibit A is the lack of error handling if there's a coding error in e.g. wait, ok or cleanup. Can we use what I suggest in comment 59? [1] http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it ::: dom/media/tests/mochitest/mediaStreamPlayback.js:64 (Diff revisions 2 - 4) > - t.addEventListener("ended", function endedTrackUnexpected() { > - ok(false, "Unexpected ended event for track " + t.id); > - }); > + let endedTrackUnexpected = () => > + (cleanup(), reject(new Error("Unexpected ended event for track " + t.id))); > + let cleanup = () => t.removeEventListener("ended", endedTrackUnexpected); > + > + t.addEventListener("ended", endedTrackUnexpected); > + wait(500) wait(0) would probably suffice. I think we just want a crank of the event loop to be sure.
Attachment #8747701 -
Flags: review?(jib)
Comment 61•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). https://reviewboard.mozilla.org/r/50001/#review48469 ::: dom/media/tests/mochitest/pc.js:471 (Diff revisions 2 - 4) > - .then(() => finished()) > + .catch(e => > - .catch(e => { > ok(false, 'Error in test execution: ' + e + > ((typeof e.stack === 'string') ? > - (' ' + e.stack.split('\n').join(' ... ')) : '')); > - finished(); > + (' ' + e.stack.split('\n').join(' ... ')) : ''))) > + .then(() => finished()) or just: .then(finished)
Attachment #8747702 -
Flags: review?(jib) → review+
Assignee | ||
Comment 62•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/4-5/
Attachment #8747701 -
Flags: review?(jib)
Assignee | ||
Comment 63•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/4-5/
Assignee | ||
Comment 64•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/5-6/
Assignee | ||
Comment 65•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/3-4/
Assignee | ||
Comment 66•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/3-4/
Assignee | ||
Comment 67•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/5-6/
Assignee | ||
Comment 68•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/3-4/
Assignee | ||
Comment 69•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/3-4/
Assignee | ||
Comment 70•8 years ago
|
||
(In reply to Cullen Jennings from comment #1) > I'd like to add two more use cases to this to consider when testing it ... > 1) if the user unplugs a microphone or camera when it is in active use by > GUM, the state needs to go to ended and the onended callback run. 2) if the > page the users goes to the browser chrome and revokes permission to share a > camera or microphone that is in use, the stream also needs to go to ended. 1) Per legacy behavior we automatically switch over to the next device. The exact logic is unclear to me. I'll file a bug. 2) Works.
Assignee | ||
Comment 71•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. I changed to haveEvent here as well. Just have a look at the diff (and ignore the rebase leftovers), thanks!
Attachment #8747703 -
Flags: review+ → review?(jib)
Assignee | ||
Comment 72•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Same here, and the rebase leftovers are quite long but nothing I've touched.
Attachment #8747702 -
Flags: review+ → review?(jib)
Updated•8 years ago
|
Attachment #8747701 -
Flags: review?(jib) → review+
Comment 73•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). https://reviewboard.mozilla.org/r/49999/#review49141
Comment 74•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). https://reviewboard.mozilla.org/r/50001/#review49147
Attachment #8747702 -
Flags: review?(jib) → review+
Comment 75•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. https://reviewboard.mozilla.org/r/50003/#review49149
Attachment #8747703 -
Flags: review?(jib) → review+
Assignee | ||
Comment 76•8 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70) > (In reply to Cullen Jennings from comment #1) > > I'd like to add two more use cases to this to consider when testing it ... > > 1) if the user unplugs a microphone or camera when it is in active use by > > GUM, the state needs to go to ended and the onended callback run. 2) if the > > page the users goes to the browser chrome and revokes permission to share a > > camera or microphone that is in use, the stream also needs to go to ended. > > 1) Per legacy behavior we automatically switch over to the next device. The > exact logic is unclear to me. I'll file a bug. I could be wrong here. A Macbook, which I tested, masks a TRRS connected device and the built in microphone as the same I think - since they both never show up at the same time in the gUM prompt.
Assignee | ||
Comment 77•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/5-6/
Assignee | ||
Comment 78•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/5-6/
Assignee | ||
Comment 79•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/6-7/
Assignee | ||
Comment 80•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/4-5/
Assignee | ||
Comment 81•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/4-5/
Assignee | ||
Comment 82•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/6-7/
Assignee | ||
Comment 83•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/4-5/
Assignee | ||
Comment 84•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/4-5/
Comment 85•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6614cb53f0a https://hg.mozilla.org/integration/mozilla-inbound/rev/b09fd2894b02 https://hg.mozilla.org/integration/mozilla-inbound/rev/e24de2c6fdb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/133c1cb39c00 https://hg.mozilla.org/integration/mozilla-inbound/rev/1709366abefd https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3ed1091455 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee525e8953bd https://hg.mozilla.org/integration/mozilla-inbound/rev/64d446651351
Comment 86•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/547b5816e80b https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ba366db4cb https://hg.mozilla.org/integration/mozilla-inbound/rev/27acc73002f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/66b38c73d507 https://hg.mozilla.org/integration/mozilla-inbound/rev/54e742653c70 https://hg.mozilla.org/integration/mozilla-inbound/rev/148472ab356d https://hg.mozilla.org/integration/mozilla-inbound/rev/75157b3d9903 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4882d8c454
Comment 87•8 years ago
|
||
backed this out since this somehow seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=27836453&repo=mozilla-inbound
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 88•8 years ago
|
||
Bug 1019579 broke the "ended" event since it exposes a track that in some cases never gets an underlying track to go live. We rely on the underlying track ending for ending the MediaStreamTrack. I tried to hack it by forcing the track to end when we close the PeerConnection, without the need for an underlying track. The only problem is, if that MediaStreamTrack was cloned, the clone won't end. Byron, you want to try fixing it up so we can guarantee that the underlying track gets created after all? Randell, can we live with this flaw for now (i.e., tell the failing tests to not expect "ended")?
Comment 89•8 years ago
|
||
I can try to move the responsibility for creation of the underlying track away from MediaPipeline. I'll have to give some thought to the best way to do this.
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 90•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 8748677 [details] > MozReview Request: Bug 1208373 - Implement MediaStreamTrack.readyState. > r?smaug,jib > > https://reviewboard.mozilla.org/r/50473/#review48433 > > ::: dom/media/MediaStreamTrack.cpp:361 > (Diff revision 2) > > NS_DispatchToMainThread(NewRunnableFrom([self]() { > > MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended"))); > > return NS_OK; > > })); > > > > - mEnded = true; > > + mReadyState = MediaStreamTrackState::Ended; > > this is not what the spec says should happen. > readyState should be set 'ended' during the same task in which the "ended" > event is fired, in other words, queue a task which first sets readyState to > 'ended' and then _synchronously_ dispatch simple event named "ended". > This difference between the patch and the spec is minor but observable from > scripts. This caused a race condition (bug 1275201). Seeing though that NotifyEnded() is already the only thing called after a previous dispatch from MediaStreamGraph to main thread, per [1]. Are we fine to just set the readyState and raise "ended" synchronously in MediaStreamTrack::NotifyEnded()? [1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/media/DOMMediaStream.cpp#199
Flags: needinfo?(bugs)
yeah, sounds like setting the readyState first and then dispatching event synchronously should be fine, assuming MediaStreamTrack can handle synchronous event dispatching (like not having raw pointers on stack and using them after dispatch and stuff like that.)
Flags: needinfo?(bugs)
Assignee | ||
Comment 92•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/6-7/
Assignee | ||
Comment 93•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/6-7/
Assignee | ||
Comment 94•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/7-8/
Assignee | ||
Comment 95•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/5-6/
Assignee | ||
Comment 96•8 years ago
|
||
Comment on attachment 8748676 [details] MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/5-6/
Assignee | ||
Comment 97•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/7-8/
Assignee | ||
Comment 98•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/5-6/
Assignee | ||
Comment 99•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/5-6/
Assignee | ||
Comment 100•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. The "ended" dispatch is now synchronous. Calls start from DOMMediaStream::OwnedStreamListener::DoNotifyTrackEnded, which is Dispatched from MediaStreamGraph to main thread by DOMMediaStream::OwnedStreamListener::NotifyQueuedTrackChanges. Smaug, could you just check that the interdiff looks fine? Thanks!
Attachment #8747704 -
Flags: review+ → review?(bugs)
Attachment #8747704 -
Flags: review?(bugs) → review+
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. https://reviewboard.mozilla.org/r/50005/#review51542 assuming the interdiff is between 7-8, then looks ok. state is updated first.
Assignee | ||
Comment 102•8 years ago
|
||
This lets us notify about a created TrackUnionStream track (and since it was created, we can notify when it ends), even though it has been blocked from main thread. Review commit: https://reviewboard.mozilla.org/r/58540/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58540/
Attachment #8747701 -
Attachment description: MozReview Request: Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). r?jib → Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().
Attachment #8747702 -
Attachment description: MozReview Request: Bug 1208373 - Test that a peerConnection's received tracks end on close(). r?jib → Bug 1208373 - Test that a peerConnection's received tracks end on close().
Attachment #8747703 -
Attachment description: MozReview Request: Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. r?jib → Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.
Attachment #8748675 -
Attachment description: MozReview Request: Bug 1208373 - Test that ended tracks that are cloned are also ended. r?jib → Bug 1208373 - Test that ended tracks that are cloned are also ended.
Attachment #8747704 -
Attachment description: MozReview Request: Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. r?smaug,jib → Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.
Attachment #8748677 -
Attachment description: MozReview Request: Bug 1208373 - Implement MediaStreamTrack.readyState. r?smaug,jib → Bug 1208373 - Implement MediaStreamTrack.readyState.
Attachment #8748678 -
Attachment description: MozReview Request: Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. r?jib → Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.
Attachment #8761251 -
Flags: review?(rjesup)
Attachment #8761252 -
Flags: review?(rjesup)
Assignee | ||
Comment 103•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58542/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58542/
Assignee | ||
Comment 104•8 years ago
|
||
Comment on attachment 8747701 [details] Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/7-8/
Assignee | ||
Comment 105•8 years ago
|
||
Comment on attachment 8747702 [details] Bug 1208373 - Test that a peerConnection's received tracks end on close(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/7-8/
Assignee | ||
Comment 106•8 years ago
|
||
Comment on attachment 8747703 [details] Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/8-9/
Assignee | ||
Comment 107•8 years ago
|
||
Comment on attachment 8748675 [details] Bug 1208373 - Test that ended tracks that are cloned are also ended. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/6-7/
Assignee | ||
Comment 108•8 years ago
|
||
Comment on attachment 8747704 [details] Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/8-9/
Assignee | ||
Comment 109•8 years ago
|
||
Comment on attachment 8748677 [details] Bug 1208373 - Implement MediaStreamTrack.readyState. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/6-7/
Assignee | ||
Comment 110•8 years ago
|
||
Comment on attachment 8748678 [details] Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/6-7/
Assignee | ||
Updated•8 years ago
|
Attachment #8748676 -
Attachment is obsolete: true
Comment 111•8 years ago
|
||
Comment on attachment 8761251 [details] Bug 1208373 - Introduce a new blocking mode to MediaInputPort. https://reviewboard.mozilla.org/r/58540/#review55414 I also wish there were a simpler way, but I think this will work ::: dom/media/DOMMediaStream.h:308 (Diff revision 1) > - already_AddRefed<media::Pledge<bool, nsresult>> BlockSourceTrackId(TrackID aTrackId); > + already_AddRefed<media::Pledge<bool, nsresult>> > + BlockSourceTrackId(TrackID aTrackId, BlockingMode aBlockingMode); Nit: I'd indent the second line
Attachment #8761251 -
Flags: review?(rjesup) → review+
Comment 112•8 years ago
|
||
Comment on attachment 8761252 [details] Bug 1208373 - Don't remove tracks from StreamTracks. Just their content. https://reviewboard.mozilla.org/r/58542/#review55416
Attachment #8761252 -
Flags: review?(rjesup) → review+
Comment 113•8 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b077be047c0 Check that we don't get "ended" event for tracks after calling stop(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/ea733590b886 Test that a peerConnection's received tracks end on close(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/1a53787bf0f7 Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/816141c9f43f Test that ended tracks that are cloned are also ended. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8e3a0dbff3 Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5684b6362e Implement MediaStreamTrack.readyState. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2842b199f5 Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/5454dcaa31ff Introduce a new blocking mode to MediaInputPort. r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/c864d8ec3ba3 Don't remove tracks from StreamTracks. Just their content. r=jesup
Comment 114•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba996f363fe Backed out changeset c864d8ec3ba3 https://hg.mozilla.org/integration/mozilla-inbound/rev/9177f45a2521 Backed out changeset 5454dcaa31ff https://hg.mozilla.org/integration/mozilla-inbound/rev/32325c1ec786 Backed out changeset 7f2842b199f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf1df26497d Backed out changeset 2b5684b6362e https://hg.mozilla.org/integration/mozilla-inbound/rev/156b087287f5 Backed out changeset 9c8e3a0dbff3 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b15328a193 Backed out changeset 816141c9f43f https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6dd61ac523 Backed out changeset 1a53787bf0f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7602995ca0f Backed out changeset ea733590b886 https://hg.mozilla.org/integration/mozilla-inbound/rev/df403befe9fe Backed out changeset 2b077be047c0 for test failures in test_all_synthetic_events.html on a CLOSED TREE
Comment 115•8 years ago
|
||
backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df403befe9fe76dea67ca4a0fb4386c6cab6bf93 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29886477&repo=mozilla-inbound
Flags: needinfo?(pehrson)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(rjesup)
Flags: needinfo?(pehrson)
Comment 116•8 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4827087a333 Check that we don't get "ended" event for tracks after calling stop(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddb73097cfd Test that a peerConnection's received tracks end on close(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8a0b464701 Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/bc120c9071bb Test that ended tracks that are cloned are also ended. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8c5a9bb9d5 Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/8cffa26910a8 Implement MediaStreamTrack.readyState. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/72cf70065470 Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/38e3e48c8dd0 Introduce a new blocking mode to MediaInputPort. r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/95412432bf10 Don't remove tracks from StreamTracks. Just their content. r=jesup
Comment 117•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c4827087a333 https://hg.mozilla.org/mozilla-central/rev/5ddb73097cfd https://hg.mozilla.org/mozilla-central/rev/fb8a0b464701 https://hg.mozilla.org/mozilla-central/rev/bc120c9071bb https://hg.mozilla.org/mozilla-central/rev/9a8c5a9bb9d5 https://hg.mozilla.org/mozilla-central/rev/8cffa26910a8 https://hg.mozilla.org/mozilla-central/rev/72cf70065470 https://hg.mozilla.org/mozilla-central/rev/38e3e48c8dd0 https://hg.mozilla.org/mozilla-central/rev/95412432bf10
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 118•8 years ago
|
||
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 → ---
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(pehrson)
Comment 119•8 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa149cf4393 Check that we don't get "ended" event for tracks after calling stop(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/528831bae79e Test that a peerConnection's received tracks end on close(). r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3948698940 Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/53dc43787dc3 Test that ended tracks that are cloned are also ended. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/51f07d349d5e Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/48581a9670a1 Implement MediaStreamTrack.readyState. r=smaug,jib https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5a51da1a85 Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/3598ea9d1b25 Introduce a new blocking mode to MediaInputPort. r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/43f38305af17 Don't remove tracks from StreamTracks. Just their content. r=jesup
Comment 120•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0aa149cf4393 https://hg.mozilla.org/mozilla-central/rev/528831bae79e https://hg.mozilla.org/mozilla-central/rev/fe3948698940 https://hg.mozilla.org/mozilla-central/rev/53dc43787dc3 https://hg.mozilla.org/mozilla-central/rev/51f07d349d5e https://hg.mozilla.org/mozilla-central/rev/48581a9670a1 https://hg.mozilla.org/mozilla-central/rev/ed5a51da1a85 https://hg.mozilla.org/mozilla-central/rev/3598ea9d1b25 https://hg.mozilla.org/mozilla-central/rev/43f38305af17
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 122•8 years ago
|
||
New documentation created: https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/onended Documentation updated: https://developer.mozilla.org/en-US/docs/Web/Events/ended (now covers both HTML media and Media Streams) https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/readyState Listed on Firefox 50 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/50#WebRTC Please feel free to correct any mistakes, or let me know if there's something you think should be fixed. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Comment 124•8 years ago
|
||
Hello there, I found that in latest Firefox 50 (macOS), when the window being shared (obtained with getUserMedia / media.getusermedia.screensharing.allowed_domains) is closed, "ended" event is not emitted from its MediaTrack. (in Chrome, "ended" have been emitted correctly) I'm preparing a minimal example to demonstrate this.
Assignee | ||
Comment 125•8 years ago
|
||
(In reply to Wang Guan from comment #124) > Hello there, > > I found that in latest Firefox 50 (macOS), when the window being shared > (obtained with getUserMedia / > media.getusermedia.screensharing.allowed_domains) is closed, "ended" event > is not emitted from its MediaTrack. > > (in Chrome, "ended" have been emitted correctly) > > I'm preparing a minimal example to demonstrate this. Feel free to file a new bug with this minimal example and make it block this one.
You need to log in
before you can comment on or make changes to this bug.
Description
•