1103188 - Implement MediaStream.addTrack()/removeTrack()

Status: RESOLVED FIXED

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

Description

[Thomas Auge]

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().

Comment 1

[Iñaki Baz Castillo]

Any ETA or comment about this? It is a very important feature IMHO.

Comment 2

[Andreas Pehrson [:pehrsons]]

I intend to implement addTrack() and removeTrack() as part of this bug. I have some WIP patches already.

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: 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 4

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons

Still quite WIP.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Comment 9

[rajkumaradass]

Could you please let us know which version of nightly build this is targeted for? as this would help plan us accordingly.

thanks.

Comment 10

[Andreas Pehrson [:pehrsons]]

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.

Comment 11

[Andreas Pehrson [:pehrsons]]

See bug 1170958 comment 56 for a test we should write.

Comment 12

[Andreas Pehrson [: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

Comment 13

[Andreas Pehrson [:pehrsons]]

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 14

[Andreas Pehrson [:pehrsons]]

Attachment: 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 15

[Andreas Pehrson [:pehrsons]]

(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.

Comment 16

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

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.

Comment 17

[Andreas Pehrson [: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).

Comment 18

[Andreas Pehrson [: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

Comment 19

[Andreas Pehrson [:pehrsons]]

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 20

[Andreas Pehrson [:pehrsons]]

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

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

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. )

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

https://reviewboard.mozilla.org/r/8991/#review17219

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 24

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

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.

Comment 25

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

Specs don't favor space before paren, but apparently this particular spec uses unusual style.

Comment 26

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

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?

Comment 27

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

(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)

Comment 28

[Andreas Pehrson [:pehrsons]]

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.

Comment 29

[Andreas Pehrson [:pehrsons]]

(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.

Comment 30

[Andreas Pehrson [:pehrsons]]

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 31

[Andreas Pehrson [: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

Comment 32

[Andreas Pehrson [:pehrsons]]

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 33

[Andreas Pehrson [:pehrsons]]

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 34

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

Bug 1103188 - Part 4. Always check tracks on getUserMedia(). r?jib

Comment 35

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib

Carries r=smaug,jib.

Comment 36

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.

Comment 37

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

Carries r=jib,roc.

Comment 38

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

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.

Comment 39

[Andreas Pehrson [:pehrsons]]

(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.

Comment 40

[Andreas Pehrson [:pehrsons]]

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

[Kyrylo Slatin]

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

Comment 42

[Andreas Pehrson [:pehrsons]]

(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.

Comment 43

[Andreas Pehrson [:pehrsons]]

(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

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

(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

Comment 45

[Andreas Pehrson [: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

Comment 46

[Andreas Pehrson [:pehrsons]]

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 47

[Andreas Pehrson [:pehrsons]]

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 48

[Andreas Pehrson [:pehrsons]]

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 49

[Andreas Pehrson [:pehrsons]]

Attachment: 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 50

[Andreas Pehrson [:pehrsons]]

Attachment: 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

Comment 51

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Bug 1103188 - Part 7. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

Comment 52

[Andreas Pehrson [:pehrsons]]

Attachment: 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 53

[Andreas Pehrson [:pehrsons]]

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.

Comment 54

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Carries r=roc

Comment 55

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib

Carries r=smaug,jib

Comment 56

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.

Comment 57

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

Not sure what went wrong here.

Comment 58

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib

Carries r=jib.

Comment 59

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

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?

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

https://reviewboard.mozilla.org/r/20177/#review18111

Comment 62

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

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

Comment 63

[Andreas Pehrson [:pehrsons]]

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

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

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

Comment 65

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

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

Comment 66

[Andreas Pehrson [:pehrsons]]

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.

Comment 67

[Andreas Pehrson [:pehrsons]]

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 68

[Andreas Pehrson [: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

Comment 69

[Andreas Pehrson [:pehrsons]]

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 70

[Andreas Pehrson [:pehrsons]]

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 71

[Andreas Pehrson [:pehrsons]]

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 72

[Andreas Pehrson [:pehrsons]]

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 73

[Andreas Pehrson [:pehrsons]]

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

Comment 74

[Andreas Pehrson [:pehrsons]]

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

Comment 75

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib

Carries r=smaug,jib.

Comment 76

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.

Comment 77

[Andreas Pehrson [:pehrsons]]

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

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

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.

Comment 79

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

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)))]);

? :)

Comment 80

[Andreas Pehrson [:pehrsons]]

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

Comment 81

[Andreas Pehrson [:pehrsons]]

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

Comment 82

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

Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

https://reviewboard.mozilla.org/r/20177/#review18319

Comment 83

[Pulsebot]

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

Comment 84

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

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

Comment 85

[Andreas Pehrson [: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

Comment 86

[Gian-Carlo Pascutto [:gcp]]

Mac specific failures might be fixed by bug 1205164, which avoids race conditions in the Mac shared memory implementation.

Comment 87

[Andreas Pehrson [:pehrsons]]

(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.

Comment 88

[Andreas Pehrson [:pehrsons]]

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.

Comment 89

[Andreas Pehrson [:pehrsons]]

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

Comment 90

[Andreas Pehrson [:pehrsons]]

Attachment: 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

Comment 91

[Andreas Pehrson [:pehrsons]]

Attachment: 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

Comment 92

[Andreas Pehrson [:pehrsons]]

Attachment: 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!)

Comment 93

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

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).

Comment 94

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

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

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

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;

?

Comment 96

[Andreas Pehrson [:pehrsons]]

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 :-)

Comment 97

[Andreas Pehrson [:pehrsons]]

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.

Comment 98

[Andreas Pehrson [:pehrsons]]

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

[Karl Tomlinson (:karlt)]

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.

Comment 100

[Andreas Pehrson [:pehrsons]]

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.

Comment 101

[Andreas Pehrson [:pehrsons]]

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

[Pulsebot]

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

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

(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).

Comment 104

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

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

Comment 105

[Kohei Yoshino [:kohei]]

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
> });

Comment 106

[Andreas Pehrson [:pehrsons]]

(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.

Comment 107

[Kohei Yoshino [:kohei]]

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

[Thijs Triemstra]

(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

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

(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.

Comment 110

[Kohei Yoshino [:kohei]]

I have updated my own app, and it worked fine.

https://github.com/bzdeck/bzdeck/commit/1859459

Comment 111

[Andreas Pehrson [:pehrsons]]

(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 112

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

It works provided you stop the tracks in the right order. See Bug 1192170 comment 10.

Comment 113

[Thijs Triemstra]

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? :-/

Comment 114

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

I can't think of an easy way. Is the workaround in Bug 1192170 comment 10 insufficient?

Comment 115

[Andreas Pehrson [:pehrsons]]

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

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

(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

[Eric Shepherd [:sheppy]]

addTrack() is now documented: https://developer.mozilla.org/en-US/docs/Web/API/MediaStream.addTrack

removeTrack() not done yet.

Comment 118

[pedro.spinsomewebs]

What's the ETA on removeTrack?