The default bug view has changed. See this FAQ.

Implement MediaStream.addTrack()/removeTrack()

RESOLVED FIXED in Firefox 44

Status

()

Core
Audio/Video: MediaStreamGraph
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Thomas Auge, Assigned: pehrsons)

Tracking

(Blocks: 3 bugs, {dev-doc-needed, site-compat})

unspecified
mozilla44
dev-doc-needed, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments, 2 obsolete attachments)

39 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
39 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
Details | Review
40 bytes, text/x-review-board-request
roc
: review+
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
40 bytes, text/x-review-board-request
jib
: review+
Details | Review
(Reporter)

Description

2 years ago
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

2 years ago
Component: Untriaged → WebRTC
Product: Firefox → Core

Comment 1

2 years ago
Any ETA or comment about this? It is a very important feature IMHO.
See Also: → bug 1137255
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)

Updated

2 years ago
Depends on: 1166140
Created attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons

/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/
Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons

Still quite WIP.
(Assignee)

Updated

2 years ago
Depends on: 1170112
(Assignee)

Updated

2 years ago
Depends on: 1170958
Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons
Attachment #8607342 - Attachment is obsolete: true
Created attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Created attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1021647

Comment 9

2 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.
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.
Keywords: dev-doc-needed
See bug 1170958 comment 56 for a test we should write.
Flags: needinfo?(pehrsons)
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
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
Created 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
(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)
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)
(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)
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)
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

2 years ago
Attachment #8657704 - Flags: review?(roc)
Attachment #8657704 - Flags: review?(jib)
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 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 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+
Specs don't favor space before paren, but apparently this particular spec uses unusual style.
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+
(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)
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
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.
(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.
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.
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

2 years ago
Attachment #8618702 - Flags: review+
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
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+
Created attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

Bug 1103188 - Part 4. Always check tracks on getUserMedia(). r?jib
Attachment #8662784 - Flags: review?(jib)
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+
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.
Attachment #8618702 - Flags: review+
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+
Attachment #8662784 - Flags: review?(jib) → review+
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.
(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)
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

2 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
(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.
(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).
(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)
See Also: → bug 1192170
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

2 years ago
Attachment #8618702 - Flags: review+ → review?(roc)
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

2 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+
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

2 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
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
Created 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.
Attachment #8665255 - Flags: review?(roc)
Created attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

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)
Created attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Bug 1103188 - Part 7. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Attachment #8665257 - Flags: review?(jib)
Created attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib

Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib
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)
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Carries r=roc
Attachment #8665257 - Flags: review+
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+
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+
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)
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 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 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+
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 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 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+
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.
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.
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

2 years ago
Attachment #8618702 - Flags: review+ → review?(roc)
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

2 years ago
Attachment #8657704 - Flags: review?(roc)
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
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
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.
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)
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

2 years ago
Attachment #8665258 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8657704 - Flags: review?(roc)
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+
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+
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 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)
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)

Updated

2 years ago
Blocks: 1208316
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)
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 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

2 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)
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)
Mac specific failures might be fixed by bug 1205164, which avoids race conditions in the Mac shared memory implementation.
(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.
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.
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
Created attachment 8667366 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib

Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Attachment #8667366 - Flags: review?(jib)
Created attachment 8667367 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib

Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Attachment #8667367 - Flags: review?(jib)
Created attachment 8667368 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib

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 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+
Attachment #8667367 - Flags: review?(jib) → review+
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 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+
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 :-)
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.
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 :)
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.
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.
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

2 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
(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
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
(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)
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

a year 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)
(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)
I have updated my own app, and it worked fine.

https://github.com/bzdeck/bzdeck/commit/1859459
Flags: needinfo?(kohei.yoshino) → needinfo?(info)
(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

a year 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)
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.
(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.
Blocks: 1223696
addTrack() is now documented: https://developer.mozilla.org/en-US/docs/Web/API/MediaStream.addTrack

removeTrack() not done yet.
Blocks: 1258143
What's the ETA on removeTrack?
You need to log in before you can comment on or make changes to this bug.