Closed
Bug 1103188
Opened 10 years ago
Closed 10 years ago
Implement MediaStream.addTrack()/removeTrack()
Categories
(Core :: Audio/Video: MediaStreamGraph, defect)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: thomas, Assigned: pehrsons)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-needed, site-compat)
Attachments
(10 files, 2 obsolete files)
39 bytes,
text/x-review-board-request
|
pehrsons
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
pehrsons
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
roc
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
roc
:
review+
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36
Steps to reproduce:
MediaStream in Firefox does not support the addTrack() method.
Actual results:
This prevents the use of video together with WebAudio processed streams in WebRTC, as the two have to be combined later into one MediaStream.
Expected results:
It should be possible to add a video track to a MediaStream from createMediaStreamDestination().
Updated•10 years ago
|
Component: Untriaged → WebRTC
Product: Firefox → Core
Comment 1•10 years ago
|
||
Any ETA or comment about this? It is a very important feature IMHO.
Assignee | ||
Comment 2•10 years ago
|
||
I intend to implement addTrack() and removeTrack() as part of this bug. I have some WIP patches already.
Assignee: nobody → pehrsons
Status: UNCONFIRMED → ASSIGNED
Component: WebRTC → Video/Audio
Ever confirmed: true
OS: Windows 7 → Unspecified
Hardware: x86_64 → Unspecified
Summary: Implement MediaStream.addTrack() → Implement MediaStream.addTrack()/removeTrack()
Version: 36 Branch → unspecified
Assignee | ||
Comment 3•10 years ago
|
||
/r/8989 - Bug 1103188 - Part 1. Implement DOMMediaStream::AddTrack/RemoveTrack. r=
/r/8991 - Bug 1103188 - Part 2. Add MediaStream::AddTrack/RemoveTrack mochitests. r=
Pull down these commits:
hg pull -r 3151f9e32b8ced9b72c850232bc63b74888520b5 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons
Still quite WIP.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8607342 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Comment 9•10 years ago
|
||
Could you please let us know which version of nightly build this is targeted for? as this would help plan us accordingly.
thanks.
Assignee | ||
Comment 10•10 years ago
|
||
I want to land it as soon as I can but it depends on how long it takes for blockers to get resolved and how long reviews need.
Let's say 44. That shouldn't be an overestimation.
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 11•10 years ago
|
||
See bug 1170958 comment 56 for a test we should write.
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Attachment #8618701 -
Attachment description: MozReview Request: Bug 1103188 - Part 1. Implement DOMMediaStream::AddTrack/RemoveTrack. r= → MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Attachment #8618702 -
Attachment description: MozReview Request: Bug 1103188 - Part 2. Add MediaStream::AddTrack/RemoveTrack mochitests. r= → MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Assignee | ||
Comment 14•10 years ago
|
||
Bug 1103188 - Part 3. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #11)
> See bug 1170958 comment 56 for a test we should write.
We can't write this test now because we don't yet remove tracks as they end (e.g., when captured from a media element).
When we have implemented MediaStreamTrack.ended and MediaStream's "removetrack" event, this seems like a very good thing to test.
Flags: needinfo?(pehrsons)
Comment 16•10 years ago
|
||
Did you mean to request review here? None of the review flags are set in either bugzilla or mozReview, so I never got any email above my bugzilla noise filter, but from your commit messages it looks like you're seeking review.
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> Did you mean to request review here? None of the review flags are set in
> either bugzilla or mozReview, so I never got any email above my bugzilla
> noise filter, but from your commit messages it looks like you're seeking
> review.
I meant to use this for safe storage (and perhaps show some progress) until bug 1170958 got stable enough :-)
I think we're there now, so I'll soon update the patches and request review for real (tm).
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Attachment #8618701 -
Flags: review?(jib)
Attachment #8618701 -
Flags: review?(bugs)
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Attachment #8618702 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8657704 -
Flags: review?(roc)
Attachment #8657704 -
Flags: review?(jib)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Bug 1103188 - Part 3. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Comment 21•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Per spec add/removeTrack() never throw exceptions.
So, remove [Throws] from .webidl
(If we think the methods should throw exceptions in some cases,
then it is ok to keep [Throws] but a spec bug must be filed. )
Attachment #8618701 -
Flags: review?(bugs) → review+
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
https://reviewboard.mozilla.org/r/8991/#review17219
Attachment #8618702 -
Flags: review?(roc) → review+
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
https://reviewboard.mozilla.org/r/18417/#review17221
Great!!!
Attachment #8657704 -
Flags: review?(roc) → review+
Comment 24•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
https://reviewboard.mozilla.org/r/8989/#review17231
::: dom/webidl/MediaStream.webidl:32
(Diff revision 3)
> +// Constructor (sequence<MediaStreamTrack> tracks)]
And Exposed=Window, if we care about commented-out code.
::: dom/webidl/MediaStream.webidl:34
(Diff revision 3)
> - readonly attribute DOMString id;
> - sequence<AudioStreamTrack> getAudioTracks();
> - sequence<VideoStreamTrack> getVideoTracks();
> - sequence<MediaStreamTrack> getTracks();
> + readonly attribute DOMString id;
> + sequence<AudioStreamTrack> getAudioTracks ();
> + sequence<VideoStreamTrack> getVideoTracks ();
> + sequence<MediaStreamTrack> getTracks ();
These are whitespace-only changes. Are they necessary? (I wish I knew why specs seem to favor space-before-paren, so I admit bias here). I'll defer to smaug.
Attachment #8618701 -
Flags: review?(jib) → review+
Comment 25•10 years ago
|
||
Specs don't favor space before paren, but apparently this particular spec uses unusual style.
Comment 26•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
https://reviewboard.mozilla.org/r/18417/#review17235
Very nice audio track tests!
::: dom/media/tests/mochitest/head.js:330
(Diff revision 2)
> + tracks.forEach(t => ok(mediaStream.getTracks().includes(t),
Is .includes finally riding the trains? If so, yay! (if not, nay).
::: dom/media/tests/mochitest/head.js:330
(Diff revision 2)
> + tracks.forEach(t => ok(mediaStream.getTracks().includes(t),
> + (message ? message + ": " : "") +
> + "MediaStream " + mediaStream.id +
> + " contains track " + t.id));
> + is(mediaStream.getTracks().length, tracks.length,
> + (message ? message + ": " : "") +
> + "MediaStream " + mediaStream.id + " contains no extra tracks");
Style-nit: maybe easier to read if we first do:
message += message ? ": " : "";
?
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:18
(Diff revision 2)
> + var audioStream, videoStream, audioVideoStream;
> + var audioTrack1, audioTrack2;
> + var videoTrack1, videoTrack2;
style-idea: These are effectively global already. If we moved these out, we'd save one indent on the whole thing (plus {} brackets and return).
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:23
(Diff revision 2)
> + .then(() => getUserMedia({audio: true, video: false})).then(s => audioStream = s)
> + .then(() => getUserMedia({audio: false, video: true})).then(s => videoStream = s)
> + .then(() => getUserMedia({audio: true, video: true})).then(s => audioVideoStream = s)
> + .then(() => {
> + info("Test media stream tracks");
> + checkMediaStreamTracks({audio: true}, audioStream, "Initial audio-only gUM");
> + checkMediaStreamTracks({video: true}, videoStream, "Initial video-only gUM");
> + checkMediaStreamTracks({audio: true, video: true}, audioVideoStream, "Initial audio-video gUM");
Better to keep constraints args identical. Also, the three gums and three tests here could be rolled into a sub-function that reuses the same constraints argument for both functions. That would reduce the possibility of maintenance mistakes by removing the need to visually inspect that these match up.
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:48
(Diff revision 2)
> + var playback = new LocalMediaStreamPlayback(testElem, stream);
> + return playback.playMedia(false);
Does this stop track afterwards? If not, we should probably `track.stop()` explicitly here if we're not going to use it again (I know they're fake, but it still seems wrong to leave them active until gc).
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:75
(Diff revision 2)
> + stream.removeTrack(videoTrack1);
> + checkMediaStreamContains(stream, [track], "Removed added video");
> +
> + stream.removeTrack(videoTrack1);
> + checkMediaStreamContains(stream, [track], "Re-removed added video");
> +
> + stream.removeTrack(track);
> + checkMediaStreamContains(stream, [], "Removed original video");
Great, just checking: would removing the original track before the added one exercise any different code paths or state?
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:87
(Diff revision 2)
> + elem.mozSrcObject = stream;
elem.srcObject = stream;
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:89
(Diff revision 2)
> + return wait(500).then(() => {
> + ok(!loadeddata, "Stream without tracks shall not raise 'loadeddata' on media element");
Is 500 ms sufficient on emulators? I guess there's no intermittent risk here, so who cares I suppose.
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:92
(Diff revision 2)
> + elem.mozSrcObject = null;
ditto
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:96
(Diff revision 2)
> + checkMediaStreamContains(stream, [track], "Re-added added then removed track");
Maybe commas in that sentence?
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:104
(Diff revision 2)
> + var audioStreamTrack = audioStream.getTracks()[0];
> + var videoStreamTrack = videoStream.getTracks()[0];
Maybe just audioTrack and videoTrack?
::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:120
(Diff revision 2)
> + function createOscillator(ac, frequency) {
createOscillatorStream?
Attachment #8657704 -
Flags: review?(jib) → review+
Comment 27•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #25)
> Specs don't favor space before paren, but apparently this particular spec
> uses unusual style.
Fwiw, the style seems enforced by whatever tool the w3c uses for class="idl", e.g. "getAudioTracks()" in the source becomes "getAudioTracks ()" on output. It seems to be their preferred style from a quick polling:
http://w3c.github.io/screen-orientation/ (space)
http://w3c.github.io/push-api/ (space)
http://w3c.github.io/webrtc-pc/ (space)
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/ (no space)
Updated•10 years ago
|
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Assignee | ||
Comment 28•10 years ago
|
||
https://reviewboard.mozilla.org/r/8989/#review17231
> These are whitespace-only changes. Are they necessary? (I wish I knew why specs seem to favor space-before-paren, so I admit bias here). I'll defer to smaug.
Not strictly necessary, but they make us follow the spec to the letter.
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8618701 [details]
> MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with
> addTrack/removeTrack. r?smaug,jib
>
> Per spec add/removeTrack() never throw exceptions.
> So, remove [Throws] from .webidl
Done.
Assignee | ||
Comment 30•10 years ago
|
||
https://reviewboard.mozilla.org/r/18417/#review17235
Thanks to padenot, mostly :)
> Is .includes finally riding the trains? If so, yay! (if not, nay).
Riding the 43 train per bug 1070767, yay!
> Style-nit: maybe easier to read if we first do:
>
> message += message ? ": " : "";
>
> ?
Just tried this. With =+ on `undefined` you'll get "undefinedsomething". I'll do `message = message ? (message + ": ") : "";` instead.
> style-idea: These are effectively global already. If we moved these out, we'd save one indent on the whole thing (plus {} brackets and return).
Nice, done.
> Better to keep constraints args identical. Also, the three gums and three tests here could be rolled into a sub-function that reuses the same constraints argument for both functions. That would reduce the possibility of maintenance mistakes by removing the need to visually inspect that these match up.
This `getUserMedia` actually comes from head.js. I think we should always check the tracks when calling gUM so I'm adding it there instead. I'll make that a new patch for you to review though.
> Does this stop track afterwards? If not, we should probably `track.stop()` explicitly here if we're not going to use it again (I know they're fake, but it still seems wrong to leave them active until gc).
It does not. Fixed.
> Great, just checking: would removing the original track before the added one exercise any different code paths or state?
It shouldn't really, but I'm testing that case in the webaudio tests below. I reckon that is sufficient.
> elem.srcObject = stream;
Done.
> Is 500 ms sufficient on emulators? I guess there's no intermittent risk here, so who cares I suppose.
Probably not but as you say I don't think it's an issue.
> Maybe commas in that sentence?
I turned it into "Re-added added-then-removed track" to avoid ambiguity.
> Maybe just audioTrack and videoTrack?
Done.
> createOscillatorStream?
Done.
Assignee | ||
Comment 31•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Attachment #8618701 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8618702 -
Flags: review+
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Assignee | ||
Comment 33•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Bug 1103188 - Part 3. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8657704 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
Bug 1103188 - Part 4. Always check tracks on getUserMedia(). r?jib
Attachment #8662784 -
Flags: review?(jib)
Assignee | ||
Comment 35•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Carries r=smaug,jib.
Attachment #8618701 -
Flags: review+
Assignee | ||
Comment 36•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Carries r=roc.
Attachment #8618702 -
Flags: review+
Assignee | ||
Comment 37•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Carries r=jib,roc.
Attachment #8657704 -
Flags: review+
Updated•10 years ago
|
Attachment #8662784 -
Flags: review?(jib) → review+
Comment 38•10 years ago
|
||
Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
https://reviewboard.mozilla.org/r/19571/#review17709
lgtm
::: dom/media/tests/mochitest/head.js:218
(Diff revision 1)
> - return navigator.mediaDevices.getUserMedia(constraints);
> + return navigator.mediaDevices.getUserMedia(constraints)
> + .then(stream => checkMediaStreamTracks(constraints, stream) || stream);
Maybe
.then(stream => (checkMediaStreamTracks(constraints, stream), stream)) ?
Then as a reader I don't have to look up what checkMediaStreamTracks returns.
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #26)
> Does this stop track afterwards? If not, we should probably `track.stop()`
> explicitly here if we're not going to use it again (I know they're fake, but
> it still seems wrong to leave them active until gc).
This revealed a problem. I changed to using `playWithMediaStreamStop()` in the test and that automatically calls `stream.stop()` after finishing.
Now, MediaStream::stop() is no longer in the spec but we still have it from older versions of said spec. The way our MediaStreams treat stream stops is by ending all tracks they own (so not stopping tracks added by `addTrack()`).
How do you think we should treat `stop()` to not break backwards compatibility too much?
I see a couple of options:
* Remove stop() and break some sites
* Stop everything owned and removeTrack() what else is in the stream
* Stop everything owned and also stop() other tracks in the stream
* Remove stream.stop() from our addTrackRemoveTrack test (use track.stop() instead) and let behavior be undefined when stopping a stream with addTrack()ed tracks.
Flags: needinfo?(jib)
Assignee | ||
Comment 40•10 years ago
|
||
The symptoms of stream.stop() when the stream has addTrack()ed tracks right now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c7c020307c
Stream does not end because the video track is still live. Audio stops though, per my local testrun.
Comment 41•10 years ago
|
||
IMHO, MediaStream class for end-users (web developers) is just a means for playing actual tracks. So tracks appended by addTrack should be treated as owned and thus stopped altogether with other tracks of the MediaStream
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #39)
> I see a couple of options:
>
> * Remove stop() and break some sites
> * Stop everything owned and removeTrack() what else is in the stream
> * Stop everything owned and also stop() other tracks in the stream
> * Remove stream.stop() from our addTrackRemoveTrack test (use track.stop()
> instead) and let behavior be undefined when stopping a stream with
> addTrack()ed tracks.
..* Stop all tracks currently in the stream (all tracks returned by `getTracks()`)
Yeah, I think this makes the most sense. Much like Kyrylo says in comment 41. This will allow us to retain current behavior for non-addTrack/removeTrack uses while still making some sort of sense for the addTrack/removeTrack cases. I'll add a patch and you can give your feedback jib.
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #42)
> ..* Stop all tracks currently in the stream (all tracks returned by
> `getTracks()`)
>
> Yeah, I think this makes the most sense. Much like Kyrylo says in comment
> 41. This will allow us to retain current behavior for
> non-addTrack/removeTrack uses while still making some sort of sense for the
> addTrack/removeTrack cases. I'll add a patch and you can give your feedback
> jib.
Though `stop()` is only available for local tracks (i.e., gUM, not remote side of peerconnection). If tracks in a stream are a mix of local and remote, this gets tricky. I'm leaning towards undefined behavior now (plus a warning in the console).
Comment 44•10 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #39)
> * Remove stop() and break some sites
No, since we want to stay backwards compatible I think.
> * Remove stream.stop() from our addTrackRemoveTrack test (use track.stop()
> instead)
Definitely this, regardless of whatever else we do on this.
> and let behavior be undefined when stopping a stream with addTrack()ed tracks.
This. While it's a tricky question, I'm fairly hard-line [1] on not adding new functionality that is non-spec, and stream.stop() is gone. To me, backwards compatible means: support code already written, but not support new code written the old way. If you touch old code then that's when you should fix it the right way.
I feel this way because getting rid of old APIs is harder than adding new ones, and touching old code is an opportune moment to push for convergence (that doesn't break old code).
> I'm leaning towards undefined behavior now (plus a warning in the console).
Yes, definitely a console warning, even of type JS error, since the desired behavior wont be met.
[1] https://github.com/webrtc/adapter/issues/58
Flags: needinfo?(jib)
Assignee | ||
Comment 45•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Attachment #8618701 -
Flags: review+ → review?(jib)
Assignee | ||
Updated•10 years ago
|
Attachment #8618702 -
Flags: review+ → review?(roc)
Assignee | ||
Comment 46•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Assignee | ||
Updated•10 years ago
|
Attachment #8657704 -
Attachment description: MozReview Request: Bug 1103188 - Part 3. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc → MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Attachment #8657704 -
Flags: review?(roc)
Attachment #8657704 -
Flags: review?(jib)
Attachment #8657704 -
Flags: review+
Assignee | ||
Comment 47•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Assignee | ||
Updated•10 years ago
|
Attachment #8662784 -
Attachment description: MozReview Request: Bug 1103188 - Part 4. Always check tracks on getUserMedia(). r?jib → MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Assignee | ||
Comment 48•10 years ago
|
||
Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Assignee | ||
Comment 49•10 years ago
|
||
Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
Other modules than MediaTrackLists may want to receive updates on a
DOMMediaStream's track set. This moves the MediaTrackListListener out of
the MediaTrackList class into DOMMediaStream as a general interface.
The logic for adding MediaTracks to the MediaTrackList when
MediaStreamTracks are added or removed from a DOMMediaStream is moved to
HTMLMediaElement as this fits the model better - HTMLMediaElement is the
owner of the MediaTrackLists.
Attachment #8665255 -
Flags: review?(roc)
Assignee | ||
Comment 50•10 years ago
|
||
Bug 1103188 - Part 6. End playback in HTMLMediaElement when a stream we are playing no longer has any tracks. r?roc
Attachment #8665256 -
Flags: review?(roc)
Assignee | ||
Comment 51•10 years ago
|
||
Bug 1103188 - Part 7. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665257 -
Flags: review?(jib)
Assignee | ||
Comment 52•10 years ago
|
||
Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib
Assignee | ||
Comment 53•10 years ago
|
||
Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Changing order of commits in mozreview might not be such a good idea.
Attachment #8662784 -
Flags: review+ → review?(jib)
Assignee | ||
Comment 54•10 years ago
|
||
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib
Carries r=roc
Attachment #8665257 -
Flags: review+
Assignee | ||
Comment 55•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Carries r=smaug,jib
Attachment #8618701 -
Flags: review?(jib) → review+
Assignee | ||
Comment 56•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Carries r=roc.
Attachment #8618702 -
Flags: review?(roc) → review+
Assignee | ||
Comment 57•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Not sure what went wrong here.
Attachment #8657704 -
Flags: review?(roc)
Assignee | ||
Comment 58•10 years ago
|
||
Comment on attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib
Carries r=jib.
Attachment #8665258 -
Flags: review+
Comment 59•10 years ago
|
||
Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
https://reviewboard.mozilla.org/r/19571/#review18105
lgtm with nits
::: dom/locales/en-US/chrome/dom/dom.properties:110
(Diff revision 2)
> +# LOCALIZATION NODE: Do not translate "MediaStream", "stop()" and "MediaStreamTrack"
NOTE
::: dom/media/DOMMediaStream.cpp:951
(Diff revision 2)
> + nsIDocument* document = nullptr;
> + if (pWindow) {
> + document = pWindow->GetExtantDoc();
> + }
Don't like ? : ?
::: dom/media/DOMMediaStream.cpp:955
(Diff revision 2)
> + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> + NS_LITERAL_CSTRING("Media"),
> + document,
Is ReportToConsole ok to call with a nullptr document?
Attachment #8662784 -
Flags: review?(jib) → review+
Comment on attachment 8665255 [details]
MozReview Request: Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
https://reviewboard.mozilla.org/r/20175/#review18109
Attachment #8665255 -
Flags: review?(roc) → review+
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
https://reviewboard.mozilla.org/r/20177/#review18111
Attachment #8665256 -
Flags: review?(roc) → review+
Comment 62•10 years ago
|
||
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib
https://reviewboard.mozilla.org/r/20179/#review18115
lgtm!
::: dom/media/tests/mochitest/mediaStreamPlayback.js:188
(Diff revision 1)
> + stopTracksForStreamInMediaPlayback: {
> + value: function () {
> + return new Promise((resolve, reject) => {
Long Promise constructor executor function. How about:
stopTracksForStreamInMediaPlayback: {
value: function () {
var stop = () => new Promise(resolve => {
this.mediaElement.addEventListener('ended', function stopped() {
this.mediaElement.removeEventListener('ended', stopped);
resolve();
});
// TODO Also check that all the tracks are local.
this.mediaStream.getTracks().forEach(t => t.stop());
});
var timeout = (ms, msg) => wait(ms).then(() => Promise.reject(new Error(msg)));
return Promise.race([stop(),
timeout(ENDED_TIMEOUT_LENGTH, "ended event never fired")])
.then(() => ok(true, "ended event successfully fired"));
},
? hmm, almost more code, but more self explanatory maybe? dunno
Attachment #8665257 -
Flags: review?(jib) → review+
Assignee | ||
Comment 63•10 years ago
|
||
https://reviewboard.mozilla.org/r/19571/#review18105
> NOTE
Wups.
> Don't like ? : ?
Yes I do :)
Fixed.
> Is ReportToConsole ok to call with a nullptr document?
Yes. I didn't investigate how nullptr is handled but it's for instance done here: https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/dom/json/nsJSON.cpp#57
Comment 64•10 years ago
|
||
Comment on attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib
https://reviewboard.mozilla.org/r/20181/#review18119
Attachment #8665258 -
Flags: review+
Comment 65•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
https://reviewboard.mozilla.org/r/18417/#review18121
Attachment #8657704 -
Flags: review?(jib) → review+
Assignee | ||
Comment 66•10 years ago
|
||
https://reviewboard.mozilla.org/r/20179/#review18115
> Long Promise constructor executor function. How about:
>
> stopTracksForStreamInMediaPlayback: {
> value: function () {
> var stop = () => new Promise(resolve => {
> this.mediaElement.addEventListener('ended', function stopped() {
> this.mediaElement.removeEventListener('ended', stopped);
> resolve();
> });
> // TODO Also check that all the tracks are local.
> this.mediaStream.getTracks().forEach(t => t.stop());
> });
> var timeout = (ms, msg) => wait(ms).then(() => Promise.reject(new Error(msg)));
>
> return Promise.race([stop(),
> timeout(ENDED_TIMEOUT_LENGTH, "ended event never fired")])
> .then(() => ok(true, "ended event successfully fired"));
> },
>
> ? hmm, almost more code, but more self explanatory maybe? dunno
I made another alternative where I moved the timeout logic to head.js. Up for you to review soon.
Assignee | ||
Comment 67•10 years ago
|
||
https://reviewboard.mozilla.org/r/20177/#review18111
OK, I have to revise this slightly. When ending media elements after tracks are gone I ran into a lot of other issues because this behavior is not expected by tests (or code, like mediaElement.captureStream was originally not made with this in mind).
I think we can live with a slight hack in the addTrackRemoveTrack test for now, and I'll revisit this after we have fixed mediaElement.captureStream streams to not modify their track set on seek, and after we have implemented MediaStream's active/inactive state.
Assignee | ||
Comment 68•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Attachment #8618701 -
Flags: review+ → review?(jib)
Assignee | ||
Updated•10 years ago
|
Attachment #8618702 -
Flags: review+ → review?(roc)
Assignee | ||
Comment 69•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Assignee | ||
Updated•10 years ago
|
Attachment #8657704 -
Flags: review?(roc)
Assignee | ||
Comment 70•10 years ago
|
||
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib
Assignee | ||
Comment 71•10 years ago
|
||
Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib
Assignee | ||
Comment 72•10 years ago
|
||
Comment on attachment 8665255 [details]
MozReview Request: Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
Other modules than MediaTrackLists may want to receive updates on a
DOMMediaStream's track set. This moves the MediaTrackListListener out of
the MediaTrackList class into DOMMediaStream as a general interface.
The logic for adding MediaTracks to the MediaTrackList when
MediaStreamTracks are added or removed from a DOMMediaStream is moved to
HTMLMediaElement as this fits the model better - HTMLMediaElement is the
owner of the MediaTrackLists.
Assignee | ||
Comment 73•10 years ago
|
||
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Bug 1103188 - Part 6. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665256 -
Attachment description: MozReview Request: Bug 1103188 - Part 6. End playback in HTMLMediaElement when a stream we are playing no longer has any tracks. r?roc → MozReview Request: Bug 1103188 - Part 6. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665256 -
Flags: review?(jib)
Assignee | ||
Comment 74•10 years ago
|
||
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib
Bug 1103188 - Part 7. Always check tracks on getUserMedia(). r?jib
Attachment #8665257 -
Attachment description: MozReview Request: Bug 1103188 - Part 7. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc → MozReview Request: Bug 1103188 - Part 7. Always check tracks on getUserMedia(). r?jib
Attachment #8665257 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8665258 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8657704 -
Flags: review?(roc)
Assignee | ||
Comment 75•10 years ago
|
||
Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Carries r=smaug,jib.
Attachment #8618701 -
Flags: review?(jib) → review+
Assignee | ||
Comment 76•10 years ago
|
||
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Carries r=roc.
Attachment #8618702 -
Flags: review?(roc) → review+
Assignee | ||
Comment 77•10 years ago
|
||
https://reviewboard.mozilla.org/r/20177/#review18185
The interdiff is not working because of the reordered commits, so I'll just point out what got updated.
::: dom/media/tests/mochitest/head.js:360
(Diff revision 2)
> + , wait(time).then(() => Promise.reject(new Error(timeoutMsg)))
I moved the timeout utility function to head.js per my previous comment.
::: dom/media/tests/mochitest/mediaStreamPlayback.js:203
(Diff revision 2)
> + this.mediaStream.stop();
There's a dummy mediaStream.stop() that doesn't belong here. It's just there for now so the media element will end playback. When we get active state for MediaStreams and we fix media element captureStream to spec we can remove this.
Comment 78•10 years ago
|
||
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
https://reviewboard.mozilla.org/r/20177/#review18189
::: dom/media/tests/mochitest/mediaStreamPlayback.js:198
(Diff revision 2)
> + // TODO Also check that all the tracks are local.
> + this.mediaStream.getTracks().forEach(t => t.stop());
> +
> + // XXX When we implement MediaStream.active, do not stop the stream.
> + // We just do it now so the media element will raise 'ended'.
Maybe add bug numbers.
Attachment #8665256 -
Flags: review?(jib)
Comment 79•10 years ago
|
||
https://reviewboard.mozilla.org/r/20177/#review18191
::: dom/media/tests/mochitest/head.js:357
(Diff revision 2)
> +function timeout(promise, time, timeoutMsg) {
> + return Promise.race(
> + [ promise
> + , wait(time).then(() => Promise.reject(new Error(timeoutMsg)))
> + ]);
> +}
How bout
var timeout = (p, ms, msg) => Promise.race([p, wait(ms).then(() => Promise.reject(new Error(timeoutMsg)))]);
? :)
Assignee | ||
Comment 80•10 years ago
|
||
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665256 -
Attachment description: MozReview Request: Bug 1103188 - Part 6. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc → MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665256 -
Flags: review?(jib)
Assignee | ||
Comment 81•10 years ago
|
||
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib
Bug 1103188 - Always check tracks on getUserMedia(). r?jib
Attachment #8665257 -
Attachment description: MozReview Request: Bug 1103188 - Part 7. Always check tracks on getUserMedia(). r?jib → MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib
Comment 82•10 years ago
|
||
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
https://reviewboard.mozilla.org/r/20177/#review18319
Attachment #8665256 -
Flags: review?(jib) → review+
Comment 83•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da8f4905060
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8cc967f8eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca97d52bf144
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc6b5f5ba444
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb656022a1a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe0ebdebad5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f33a8ef14c
https://treeherder.mozilla.org/logviewer.html#?job_id=14723851&repo=mozilla-inbound started happening frequently after this and bug 1170958 landed.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab984256b2d
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 85•10 years ago
|
||
I was able to reproduce on Mac as well if I use the real camera stream intead of the fake one, and run the test a couple of times per:
> ./mach mochitest dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html --e10s --run-until-failure
I seem to be triggering an existing bug because I do gumStream.stop() and getUserMedia() repeatedly and without delays in between.
CamerasParent is failing when calling AddRenderer at [1]. In the logs it looked like some commands from CamerasChild came in the wrong order when it failed, but I'm still trying to verify exactly what's going on.
[1] https://dxr.mozilla.org/mozilla-central/rev/6256ec9113c115141aab089c45ee69438884b680/dom/media/systemservices/CamerasParent.cpp#682
Flags: needinfo?(pehrsons)
Comment 86•10 years ago
|
||
Mac specific failures might be fixed by bug 1205164, which avoids race conditions in the Mac shared memory implementation.
Assignee | ||
Comment 87•10 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #86)
> Mac specific failures might be fixed by bug 1205164, which avoids race
> conditions in the Mac shared memory implementation.
Still happens. I'll dig deeper.
Assignee | ||
Comment 88•10 years ago
|
||
A normal case looks like:
> 544964608[113281e80]: int mozilla::camera::CamerasChild::AllocateCaptureDevice(mozilla::camera::CaptureEngine, const char *, const unsigned int, int &)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyAllocateCaptureDevice(const int &)
> 544964608[113281e80]: Capture Device allocated: 4097
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StartCapture(mozilla::camera::CaptureEngine, const int, webrtc::CaptureCapability &, webrtc::ExternalRenderer *)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvFrameSizeChange(const int &, const int &, const int &, const int &)
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StopCapture(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
But when it fails it looks like:
> 544964608[113281e80]: int mozilla::camera::CamerasChild::AllocateCaptureDevice(mozilla::camera::CaptureEngine, const char *, const unsigned int, int &)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyAllocateCaptureDevice(const int &)
> 544964608[113281e80]: Capture Device allocated: 4097
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StartCapture(mozilla::camera::CaptureEngine, const int, webrtc::CaptureCapability &, webrtc::ExternalRenderer *)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyFailure()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StopCapture(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
Now we for some reason get:
> Allocate()
> Release() <- wut?
> Start()
> Stop()
> Release()
This looks wrong. Trying to see what's causing this.
Assignee | ||
Comment 89•10 years ago
|
||
We have something like this going on in MediaManager:
gUM() -> dispatches GetUserMediaTask
MSG stream finished -> dispatches MediaOperationTask with MEDIA_STOP
GetUserMediaTask runs -> calls Allocate() -> dispatches MediaOperationTask with MEDIA_START
MediaOperationTask with MEDIA_STOP runs -> calls Release() <-- this is from an old gUM request
MediaOperationTask with MEDIA_START runs -> calls Start() -> fails because camera was deallocated -> calls stop to clean up
Assignee | ||
Comment 90•10 years ago
|
||
Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Attachment #8667366 -
Flags: review?(jib)
Assignee | ||
Comment 91•10 years ago
|
||
Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Attachment #8667367 -
Flags: review?(jib)
Assignee | ||
Comment 92•10 years ago
|
||
Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
This is needed to avoid something like:
* [old stream] stop track 1 -> deallocate MediaDevice for track 1
* [new stream] gUM() -> allocate MediaDevice for track 1
* [old stream] stop stream -> deallocate MediaDevice for track 1
* [new stream] gUM() -> start MediaDevice for track 1 (oops, MediaDevice was no more!)
Attachment #8667368 -
Flags: review?(jib)
Comment 93•10 years ago
|
||
Comment on attachment 8667366 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
https://reviewboard.mozilla.org/r/20733/#review18583
Nice cleanup.
::: dom/media/MediaManager.h:240
(Diff revision 1)
> NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
Maybe change to MOZ_ASSERT here too while you're at it?
::: dom/media/MediaManager.h:275
(Diff revision 1)
> - NotifyFinished(aGraph);
> + NS_DispatchToMainThread(
> + NS_NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyFinished));
I suppose the invariant that guarantees that 'this' is alive when the runnable executes is that the runnable is the one removing it?
Can a listener be removed any other way?
I see the pattern repeated in e.g. GetUserMediaCallbackMediaStreamListener::Invalidate, so hopefully it's ok there too.
::: dom/media/MediaManager.h:280
(Diff revision 1)
> + NS_NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyRemoved));
How about using lambdas here? :) See media::NewRunnableFrom if you're interested.
::: dom/media/MediaManager.cpp:2349
(Diff revision 1)
> + MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");
Perhaps subjective, but I find the second argument to MOZ_ASSERT redundant in most cases (like here).
Attachment #8667366 -
Flags: review?(jib) → review+
Updated•10 years ago
|
Attachment #8667367 -
Flags: review?(jib) → review+
Comment 94•10 years ago
|
||
Comment on attachment 8667367 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
https://reviewboard.mozilla.org/r/20735/#review18591
lgtm.
::: dom/media/MediaManager.cpp:3028
(Diff revision 1)
> // XXX refactor to combine with Invalidate()?
remove?
::: dom/media/MediaManager.cpp:3032
(Diff revision 1)
> - NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> + MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");
Again I vote to remove the second argument.
::: dom/media/MediaManager.cpp:3038
(Diff revision 1)
> + if (!mAudioDevice) {
> + Invalidate();
> + } else {
Your change here seems neutral, so this isn't a comment on your patch. Just curious:
Can mAudioDevice->GetMediaSource() == dom::MediaSourceEnum::AudioCapture here, and if so, is it ok that the code further down that does test for this and does something I don't fully understand, doesn't run in this case? I can't tell.
Comment 95•10 years ago
|
||
Comment on attachment 8667368 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
https://reviewboard.mozilla.org/r/20737/#review18605
Ship it with fix.
::: dom/media/MediaManager.cpp:3014
(Diff revision 1)
> mStopped = true;
>
> + AudioDevice* audioDevice = nullptr;
> + VideoDevice* videoDevice = nullptr;
> + if (!mAudioStopped) {
> + audioDevice = mAudioDevice.get();
> + }
> + if (!mVideoStopped) {
> + videoDevice = mVideoDevice.get();
> + }
> +
> // We can't take a chance on blocking here, so proxy this to another
> // thread.
> // Pass a ref to us (which is threadsafe) so it can query us for the
> // source stream info.
> MediaManager::PostTask(FROM_HERE,
> new MediaOperationTask(MEDIA_STOP,
> this, nullptr, nullptr,
> - mAudioDevice, mVideoDevice,
> + audioDevice, videoDevice,
> mFinished, mWindowID, nullptr));
> + mAudioStopped = !!audioDevice;
> + mVideoStopped = !!videoDevice;
Isn't this just:
MediaManager::PostTask(FROM_HERE,
new MediaOperationTask(MEDIA_STOP,
this, nullptr, nullptr,
(!mAudioStopped? audioDevice.get() : nullptr),
(!mVideoStopped? videoDevice.get() : nullptr),
mFinished, mWindowID, nullptr));
mStopped = mAudioStopped = mVideoStopped = true;
?
::: dom/media/MediaManager.cpp:3177
(Diff revision 1)
> + AudioDevice* audioDevice = aIsAudio && !mAudioStopped ? mAudioDevice.get() : nullptr;
> + VideoDevice* videoDevice = !aIsAudio && !mVideoStopped ? mVideoDevice.get() : nullptr;
> MediaManager::PostTask(FROM_HERE,
> new MediaOperationTask(MEDIA_STOP_TRACK,
> this, nullptr, nullptr,
> - aIsAudio ? mAudioDevice.get() : nullptr,
> + audioDevice, videoDevice,
> - !aIsAudio ? mVideoDevice.get() : nullptr,
> mFinished, mWindowID, nullptr));
> + mAudioStopped = !!audioDevice;
> + mVideoStopped = !!videoDevice;
If mAudioStopped or mVideoStopped are already set, this code clears them, which seems wrong. How about:
bool stopAudio = aIsAudio && !mAudioStopped;
bool stopVideo = !aIsAudio && !mVideoStopped;
MediaManager::PostTask(FROM_HERE,
new MediaOperationTask(MEDIA_STOP_TRACK,
this, nullptr, nullptr,
(stopAudio? mAudioDevice.get() : nullptr)
(stopVideo? mVideoDevice.get() : nullptr,
mFinished, mWindowID, nullptr));
mAudioStopped |= stopAudio;
mVideoStopped |= stopVideo;
?
Attachment #8667368 -
Flags: review?(jib) → review+
Assignee | ||
Comment 96•10 years ago
|
||
https://reviewboard.mozilla.org/r/20733/#review18583
> I suppose the invariant that guarantees that 'this' is alive when the runnable executes is that the runnable is the one removing it?
>
> Can a listener be removed any other way?
>
> I see the pattern repeated in e.g. GetUserMediaCallbackMediaStreamListener::Invalidate, so hopefully it's ok there too.
Safer to pass a refptr to a lambda I guess. `this` has threadsafe refcounting so we should be good.
> How about using lambdas here? :) See media::NewRunnableFrom if you're interested.
Oh, yes! Looks much nicer.
> Perhaps subjective, but I find the second argument to MOZ_ASSERT redundant in most cases (like here).
Agree. But then I'll change all the other ones too :-)
Assignee | ||
Comment 97•10 years ago
|
||
https://reviewboard.mozilla.org/r/20735/#review18591
> remove?
Done.
> Your change here seems neutral, so this isn't a comment on your patch. Just curious:
>
> Can mAudioDevice->GetMediaSource() == dom::MediaSourceEnum::AudioCapture here, and if so, is it ok that the code further down that does test for this and does something I don't fully understand, doesn't run in this case? I can't tell.
The checks below run if mAudioDevice is set, and `mAudioDevice` is only set to an instance on `Activate()`, never to nullptr. Could be a problem if you have both video and audio screen capture I suppose? Well, audio capture is experimental and behind a flag anyways.
Assignee | ||
Comment 98•10 years ago
|
||
https://reviewboard.mozilla.org/r/20737/#review18605
> Isn't this just:
>
> MediaManager::PostTask(FROM_HERE,
> new MediaOperationTask(MEDIA_STOP,
> this, nullptr, nullptr,
> (!mAudioStopped? audioDevice.get() : nullptr),
> (!mVideoStopped? videoDevice.get() : nullptr),
> mFinished, mWindowID, nullptr));
> mStopped = mAudioStopped = mVideoStopped = true;
>
> ?
You're right.
> If mAudioStopped or mVideoStopped are already set, this code clears them, which seems wrong. How about:
>
> bool stopAudio = aIsAudio && !mAudioStopped;
> bool stopVideo = !aIsAudio && !mVideoStopped;
>
> MediaManager::PostTask(FROM_HERE,
> new MediaOperationTask(MEDIA_STOP_TRACK,
> this, nullptr, nullptr,
> (stopAudio? mAudioDevice.get() : nullptr)
> (stopVideo? mVideoDevice.get() : nullptr,
> mFinished, mWindowID, nullptr));
> mAudioStopped |= stopAudio;
> mVideoStopped |= stopVideo;
>
> ?
You're right again! I was a bit stressed when I wrote this. Thanks :)
Comment 99•10 years ago
|
||
https://reviewboard.mozilla.org/r/20733/#review18583
> Safer to pass a refptr to a lambda I guess. `this` has threadsafe refcounting so we should be good.
NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.
https://hg.mozilla.org/mozilla-central/annotate/acdb22976ff86539dc10413c5f366e1fb429a680/xpcom/glue/nsThreadUtils.h#l306
> Oh, yes! Looks much nicer.
Lambdas can look nicer but AFAIK we're missing support for move construction of lambda closures. When the closures contain smart pointers, copy construction would produce unnecessary ref-count manipulation, which is a bit more expensive for atomic ref-counts.
Assignee | ||
Comment 100•10 years ago
|
||
https://reviewboard.mozilla.org/r/20733/#review18583
> NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.
> https://hg.mozilla.org/mozilla-central/annotate/acdb22976ff86539dc10413c5f366e1fb429a680/xpcom/glue/nsThreadUtils.h#l306
Ah, great info. Then I'm switching back to NS_NewRunnableMethod() for its conciseness.
Assignee | ||
Comment 101•10 years ago
|
||
Try push for m-e10s-2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b9e5d0773d
Try push (builds only) for comment fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=429346341fe5
Comment 102•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db0763589cc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd114d99c70
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fee52040539
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c662547624
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e11e0cb6416
https://hg.mozilla.org/integration/mozilla-inbound/rev/29309e60a4bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cd802ae577
https://hg.mozilla.org/integration/mozilla-inbound/rev/56468656b77f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7580a7086ff1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad5cb2c028d
Comment 103•10 years ago
|
||
(In reply to Karl Tomlinson (ni?:karlt) from comment #99)
> NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.
Makes sense now that I read the docs and see that it's specifically written for ref-counted objects, thanks!
> Lambdas can look nicer but AFAIK we're missing support for move construction
> of lambda closures. When the closures contain smart pointers, copy
> construction would produce unnecessary ref-count manipulation, which is a
> bit more expensive for atomic ref-counts.
Good point in this case where we don't have an nsRefPtr to start, and create one solely for passing (not always an issue in general, but I'm all for not disturbing atomic ref-counts unnecessarily).
https://hg.mozilla.org/mozilla-central/rev/db0763589cc3
https://hg.mozilla.org/mozilla-central/rev/8fd114d99c70
https://hg.mozilla.org/mozilla-central/rev/1fee52040539
https://hg.mozilla.org/mozilla-central/rev/46c662547624
https://hg.mozilla.org/mozilla-central/rev/7e11e0cb6416
https://hg.mozilla.org/mozilla-central/rev/29309e60a4bf
https://hg.mozilla.org/mozilla-central/rev/b4cd802ae577
https://hg.mozilla.org/mozilla-central/rev/56468656b77f
https://hg.mozilla.org/mozilla-central/rev/7580a7086ff1
https://hg.mozilla.org/mozilla-central/rev/8ad5cb2c028d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 105•10 years ago
|
||
I'm writing the site compatibility doc. This is my understanding but correct?
> navigator.mediaDevices.getUserMedia({ audio: false, video: true }).then(input => {
> // input.stop(); // Deprecated
> input.getVideoTracks()[0].stop(); // Recommended
> });
Flags: needinfo?(pehrsons)
Keywords: site-compat
Assignee | ||
Comment 106•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #105)
> I'm writing the site compatibility doc. This is my understanding but correct?
>
> > navigator.mediaDevices.getUserMedia({ audio: false, video: true }).then(input => {
> > // input.stop(); // Deprecated
> > input.getVideoTracks()[0].stop(); // Recommended
> > });
Correct!
There are still some quirks here. For instance HTMLMediaElement should raise "ended" when the stream (`input` in your example) becomes inactive (no longer has live tracks).
For now we don't have the either active/inactive state on streams or "ended" on no tracks ("ended" needs `input.stop()` to happen, or that the source ends the stream, which will happen for getUserMedia). I'm hoping to get most of this fixed in 44 but there's a dependency on bug 1172394 that might take some time to resolve.
Flags: needinfo?(pehrsons)
Comment 107•10 years ago
|
||
Thanks Andreas! I just posted the site compat doc only describing MediaStream.stop for now:
https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-deprecated/
Comment 108•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #107)
> Thanks Andreas! I just posted the site compat doc only describing
> MediaStream.stop for now:
>
> https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-
> deprecated/
After reading that article I started to use MediaStreamTrack.stop() in Chrome (that also deprecated MediaStream.stop() in M45) but this does not seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop() doesn't disable the camera(light) at all. Should I file a new bug for this? (its driving me insane)
Comment 109•10 years ago
|
||
(In reply to info from comment #108)
> (In reply to Kohei Yoshino [:kohei] from comment #107)
> > Thanks Andreas! I just posted the site compat doc only describing
> > MediaStream.stop for now:
> >
> > https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-
> > deprecated/
>
> After reading that article I started to use MediaStreamTrack.stop() in
> Chrome (that also deprecated MediaStream.stop() in M45) but this does not
> seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop()
> doesn't disable the camera(light) at all. Should I file a new bug for this?
> (its driving me insane)
It should work (see the snippet in comment 105 for an example). Please verify you're seeing the set of tracks, perhaps in the debugger. If you're pretty sure you have it right, please file a new bug, preferably with a simple testcase.
Flags: needinfo?(kohei.yoshino)
Comment 110•10 years ago
|
||
I have updated my own app, and it worked fine.
https://github.com/bzdeck/bzdeck/commit/1859459
Flags: needinfo?(kohei.yoshino) → needinfo?(info)
Assignee | ||
Comment 111•10 years ago
|
||
(In reply to info from comment #108)
> After reading that article I started to use MediaStreamTrack.stop() in
> Chrome (that also deprecated MediaStream.stop() in M45) but this does not
> seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop()
> doesn't disable the camera(light) at all. Should I file a new bug for this?
> (its driving me insane)
The deprecation happened in 44 so that's really from when support for track.stop() can be expected to be fully supported.
We had bug 1192170 land in 44 as well, so you need at least that version for your code to work as expected.
You can try it with Nightly (https://nightly.mozilla.org/), and please do report a bug if it doesn't work there!
Comment hidden (obsolete) |
Comment 113•10 years ago
|
||
Thanks everyone, tested it in nightly 44 and the light is off now. Any suggestions on how to check for this functionality (<44) without checking for browser/version nr? :-/
Flags: needinfo?(info)
Comment hidden (obsolete) |
Assignee | ||
Comment 115•10 years ago
|
||
If you have to support stopping the stream anyway because of old Firefox installations, checking for the existence of MediaStream.stop would be a way to stay compatible. Not sure when it will actually be removed though.
Another way would be to check for MediaStream.addTrack, but it's more of "This implementation is sufficiently up to date so stopping the tracks should be fine". At least track.stop and stream.addTrack goes hand in hand since a major reason why you'd want track.stop over stream.stop is that you can mix tracks from different streams.
Comment 116•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #114)
> Is the workaround in Bug 1192170 comment 10 insufficient?
Turned out I was wrong there. Also, in the end, that bug was about the Firefox camera indicator, not the light. If you see a problem with the camera light then please file a new bug on it.
Comment 117•9 years ago
|
||
addTrack() is now documented: https://developer.mozilla.org/en-US/docs/Web/API/MediaStream.addTrack
removeTrack() not done yet.
Comment 118•9 years ago
|
||
What's the ETA on removeTrack?
You need to log in
before you can comment on or make changes to this bug.
Description
•