Thomas Auge Reporter
	Description • 10 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().

	Iñaki Baz Castillo
	Comment 1 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 2 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 3 • 9 years ago

/r/8989 - Bug 1103188 - Part 1. Implement DOMMediaStream::AddTrack/RemoveTrack. r=
/r/8991 - Bug 1103188 - Part 2. Add MediaStream::AddTrack/RemoveTrack mochitests. r=

Pull down these commits:

hg pull -r 3151f9e32b8ced9b72c850232bc63b74888520b5 https://reviewboard-hg.mozilla.org/gecko/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 4 • 9 years ago

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

Still quite WIP.

	rajkumaradass
	Comment 9 • 9 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.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 10 • 9 years ago

I want to land it as soon as I can but it depends on how long it takes for blockers to get resolved and how long reviews need.

Let's say 44. That shouldn't be an overestimation.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 11 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 12 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 13 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 14 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 15 • 9 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #11)
> See bug 1170958 comment 56 for a test we should write.

We can't write this test now because we don't yet remove tracks as they end (e.g., when captured from a media element).

When we have implemented MediaStreamTrack.ended and MediaStream's "removetrack" event, this seems like a very good thing to test.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 16 • 9 years ago

Did you mean to request review here? None of the review flags are set in either bugzilla or mozReview, so I never got any email above my bugzilla noise filter, but from your commit messages it looks like you're seeking review.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 17 • 9 years ago

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> Did you mean to request review here? None of the review flags are set in
> either bugzilla or mozReview, so I never got any email above my bugzilla
> noise filter, but from your commit messages it looks like you're seeking
> review.

I meant to use this for safe storage (and perhaps show some progress) until bug 1170958 got stable enough :-)
I think we're there now, so I'll soon update the patches and request review for real (tm).

	Andreas Pehrson [:pehrsons] Assignee
	Comment 18 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 19 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 20 • 9 years ago

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

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 9 years ago

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

Per spec add/removeTrack() never throw exceptions.
So, remove [Throws] from .webidl

(If we think the methods should throw exceptions in some cases,
then it is ok to keep [Throws] but a spec bug must be filed. )

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 9 years ago

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

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 23 • 9 years ago

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

https://reviewboard.mozilla.org/r/18417/#review17221

Great!!!

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 24 • 9 years ago

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

https://reviewboard.mozilla.org/r/8989/#review17231

::: dom/webidl/MediaStream.webidl:32
(Diff revision 3)
> +//  Constructor (sequence<MediaStreamTrack> tracks)]

And Exposed=Window, if we care about commented-out code.

::: dom/webidl/MediaStream.webidl:34
(Diff revision 3)
> -    readonly attribute DOMString    id;
> -    sequence<AudioStreamTrack> getAudioTracks();
> -    sequence<VideoStreamTrack> getVideoTracks();
> -    sequence<MediaStreamTrack> getTracks();
> +    readonly    attribute DOMString    id;
> +    sequence<AudioStreamTrack> getAudioTracks ();
> +    sequence<VideoStreamTrack> getVideoTracks ();
> +    sequence<MediaStreamTrack> getTracks ();

These are whitespace-only changes. Are they necessary? (I wish I knew why specs seem to favor space-before-paren, so I admit bias here). I'll defer to smaug.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 26 • 9 years ago

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

https://reviewboard.mozilla.org/r/18417/#review17235

Very nice audio track tests!

::: dom/media/tests/mochitest/head.js:330
(Diff revision 2)
> +  tracks.forEach(t => ok(mediaStream.getTracks().includes(t),

Is .includes finally riding the trains? If so, yay! (if not, nay).

::: dom/media/tests/mochitest/head.js:330
(Diff revision 2)
> +  tracks.forEach(t => ok(mediaStream.getTracks().includes(t),
> +                         (message ? message + ": " : "") +
> +                         "MediaStream " + mediaStream.id +
> +                         " contains track " + t.id));
> +  is(mediaStream.getTracks().length, tracks.length,
> +     (message ? message + ": " : "") +
> +     "MediaStream " + mediaStream.id + " contains no extra tracks");

Style-nit: maybe easier to read if we first do:

 message += message ? ": " : "";
 
 ?

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:18
(Diff revision 2)
> +    var audioStream, videoStream, audioVideoStream;
> +    var audioTrack1, audioTrack2;
> +    var videoTrack1, videoTrack2;

style-idea: These are effectively global already. If we moved these out, we'd save one indent on the whole thing (plus {} brackets and return).

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:23
(Diff revision 2)
> +      .then(() => getUserMedia({audio: true, video: false})).then(s => audioStream = s)
> +      .then(() => getUserMedia({audio: false, video: true})).then(s => videoStream = s)
> +      .then(() => getUserMedia({audio: true, video: true})).then(s => audioVideoStream = s)
> +      .then(() => {
> +        info("Test media stream tracks");
> +        checkMediaStreamTracks({audio: true}, audioStream, "Initial audio-only gUM");
> +        checkMediaStreamTracks({video: true}, videoStream, "Initial video-only gUM");
> +        checkMediaStreamTracks({audio: true, video: true}, audioVideoStream, "Initial audio-video gUM");

Better to keep constraints args identical. Also, the three gums and three tests here could be rolled into a sub-function that reuses the same constraints argument for both functions. That would reduce the possibility of maintenance mistakes by removing the need to visually inspect that these match up.

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:48
(Diff revision 2)
> +        var playback = new LocalMediaStreamPlayback(testElem, stream);
> +        return playback.playMedia(false);

Does this stop track afterwards? If not, we should probably `track.stop()` explicitly here if we're not going to use it again (I know they're fake, but it still seems wrong to leave them active until gc).

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:75
(Diff revision 2)
> +        stream.removeTrack(videoTrack1);
> +        checkMediaStreamContains(stream, [track], "Removed added video");
> +
> +        stream.removeTrack(videoTrack1);
> +        checkMediaStreamContains(stream, [track], "Re-removed added video");
> +
> +        stream.removeTrack(track);
> +        checkMediaStreamContains(stream, [], "Removed original video");

Great, just checking: would removing the original track before the added one exercise any different code paths or state?

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:87
(Diff revision 2)
> +        elem.mozSrcObject = stream;

elem.srcObject = stream;

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:89
(Diff revision 2)
> +        return wait(500).then(() => {
> +          ok(!loadeddata, "Stream without tracks shall not raise 'loadeddata' on media element");

Is 500 ms sufficient on emulators? I guess there's no intermittent risk here, so who cares I suppose.

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:92
(Diff revision 2)
> +          elem.mozSrcObject = null;

ditto

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:96
(Diff revision 2)
> +          checkMediaStreamContains(stream, [track], "Re-added added then removed track");

Maybe commas in that sentence?

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:104
(Diff revision 2)
> +          var audioStreamTrack = audioStream.getTracks()[0];
> +          var videoStreamTrack = videoStream.getTracks()[0];

Maybe just audioTrack and videoTrack?

::: dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html:120
(Diff revision 2)
> +        function createOscillator(ac, frequency) {

createOscillatorStream?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 27 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #25)
> Specs don't favor space before paren, but apparently this particular spec
> uses unusual style.

Fwiw, the style seems enforced by whatever tool the w3c uses for class="idl", e.g. "getAudioTracks()" in the source becomes "getAudioTracks ()" on output. It seems to be their preferred style from a quick polling:

http://w3c.github.io/screen-orientation/ (space)
http://w3c.github.io/push-api/ (space)
http://w3c.github.io/webrtc-pc/ (space)
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/ (no space)

	Andreas Pehrson [:pehrsons] Assignee
	Comment 28 • 9 years ago

https://reviewboard.mozilla.org/r/8989/#review17231

> These are whitespace-only changes. Are they necessary? (I wish I knew why specs seem to favor space-before-paren, so I admit bias here). I'll defer to smaug.

Not strictly necessary, but they make us follow the spec to the letter.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 29 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8618701 [details]
> MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with
> addTrack/removeTrack. r?smaug,jib
> 
> Per spec add/removeTrack() never throw exceptions.
> So, remove [Throws] from .webidl

Done.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 30 • 9 years ago

https://reviewboard.mozilla.org/r/18417/#review17235

Thanks to padenot, mostly :)

> Is .includes finally riding the trains? If so, yay! (if not, nay).

Riding the 43 train per bug 1070767, yay!

> Style-nit: maybe easier to read if we first do:
> 
>  message += message ? ": " : "";
>  
>  ?

Just tried this. With =+ on `undefined` you'll get "undefinedsomething". I'll do `message = message ? (message + ": ") : "";` instead.

> style-idea: These are effectively global already. If we moved these out, we'd save one indent on the whole thing (plus {} brackets and return).

Nice, done.

> Better to keep constraints args identical. Also, the three gums and three tests here could be rolled into a sub-function that reuses the same constraints argument for both functions. That would reduce the possibility of maintenance mistakes by removing the need to visually inspect that these match up.

This `getUserMedia` actually comes from head.js. I think we should always check the tracks when calling gUM so I'm adding it there instead. I'll make that a new patch for you to review though.

> Does this stop track afterwards? If not, we should probably `track.stop()` explicitly here if we're not going to use it again (I know they're fake, but it still seems wrong to leave them active until gc).

It does not. Fixed.

> Great, just checking: would removing the original track before the added one exercise any different code paths or state?

It shouldn't really, but I'm testing that case in the webaudio tests below. I reckon that is sufficient.

> elem.srcObject = stream;

Done.

> Is 500 ms sufficient on emulators? I guess there's no intermittent risk here, so who cares I suppose.

Probably not but as you say I don't think it's an issue.

> Maybe commas in that sentence?

I turned it into "Re-added added-then-removed track" to avoid ambiguity.

> Maybe just audioTrack and videoTrack?

Done.

> createOscillatorStream?

Done.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 31 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 32 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 33 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 34 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 35 • 9 years ago

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

Carries r=smaug,jib.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 36 • 9 years ago

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

Carries r=roc.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 37 • 9 years ago

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

Carries r=jib,roc.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 38 • 9 years ago

Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

https://reviewboard.mozilla.org/r/19571/#review17709

lgtm

::: dom/media/tests/mochitest/head.js:218
(Diff revision 1)
> -  return navigator.mediaDevices.getUserMedia(constraints);
> +  return navigator.mediaDevices.getUserMedia(constraints)
> +    .then(stream => checkMediaStreamTracks(constraints, stream) || stream);

Maybe

    .then(stream => (checkMediaStreamTracks(constraints, stream), stream)) ?

Then as a reader I don't have to look up what checkMediaStreamTracks returns.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 39 • 9 years ago

(In reply to Jan-Ivar Bruaroey [:jib] from comment #26)
> Does this stop track afterwards? If not, we should probably `track.stop()`
> explicitly here if we're not going to use it again (I know they're fake, but
> it still seems wrong to leave them active until gc).

This revealed a problem. I changed to using `playWithMediaStreamStop()` in the test and that automatically calls `stream.stop()` after finishing.

Now, MediaStream::stop() is no longer in the spec but we still have it from older versions of said spec. The way our MediaStreams treat stream stops is by ending all tracks they own (so not stopping tracks added by `addTrack()`).

How do you think we should treat `stop()` to not break backwards compatibility too much?

I see a couple of options:

* Remove stop() and break some sites
* Stop everything owned and removeTrack() what else is in the stream
* Stop everything owned and also stop() other tracks in the stream
* Remove stream.stop() from our addTrackRemoveTrack test (use track.stop() instead) and let behavior be undefined when stopping a stream with addTrack()ed tracks.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 40 • 9 years ago

The symptoms of stream.stop() when the stream has addTrack()ed tracks right now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c7c020307c

Stream does not end because the video track is still live. Audio stops though, per my local testrun.

	Kyrylo Slatin
	Comment 41 • 9 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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 42 • 9 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #39)
> I see a couple of options:
> 
> * Remove stop() and break some sites
> * Stop everything owned and removeTrack() what else is in the stream
> * Stop everything owned and also stop() other tracks in the stream
> * Remove stream.stop() from our addTrackRemoveTrack test (use track.stop()
> instead) and let behavior be undefined when stopping a stream with
> addTrack()ed tracks.
..* Stop all tracks currently in the stream (all tracks returned by `getTracks()`)

Yeah, I think this makes the most sense. Much like Kyrylo says in comment 41. This will allow us to retain current behavior for non-addTrack/removeTrack uses while still making some sort of sense for the addTrack/removeTrack cases. I'll add a patch and you can give your feedback jib.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 43 • 9 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #42)
> ..* Stop all tracks currently in the stream (all tracks returned by
> `getTracks()`)
> 
> Yeah, I think this makes the most sense. Much like Kyrylo says in comment
> 41. This will allow us to retain current behavior for
> non-addTrack/removeTrack uses while still making some sort of sense for the
> addTrack/removeTrack cases. I'll add a patch and you can give your feedback
> jib.

Though `stop()` is only available for local tracks (i.e., gUM, not remote side of peerconnection). If tracks in a stream are a mix of local and remote, this gets tricky. I'm leaning towards undefined behavior now (plus a warning in the console).

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 44 • 9 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #39)
> * Remove stop() and break some sites

No, since we want to stay backwards compatible I think.

> * Remove stream.stop() from our addTrackRemoveTrack test (use track.stop()
> instead)

Definitely this, regardless of whatever else we do on this.

> and let behavior be undefined when stopping a stream with addTrack()ed tracks.

This. While it's a tricky question, I'm fairly hard-line [1] on not adding new functionality that is non-spec, and stream.stop() is gone. To me, backwards compatible means: support code already written, but not support new code written the old way. If you touch old code then that's when you should fix it the right way.

I feel this way because getting rid of old APIs is harder than adding new ones, and touching old code is an opportune moment to push for convergence (that doesn't break old code).

> I'm leaning towards undefined behavior now (plus a warning in the console).

Yes, definitely a console warning, even of type JS error, since the desired behavior wont be met.

[1] https://github.com/webrtc/adapter/issues/58

	Andreas Pehrson [:pehrsons] Assignee
	Comment 45 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 46 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 47 • 9 years ago

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

Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

	Andreas Pehrson [:pehrsons] Assignee
	Comment 48 • 9 years ago

Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 49 • 9 years ago

Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
Other modules than MediaTrackLists may want to receive updates on a
DOMMediaStream's track set. This moves the MediaTrackListListener out of
the MediaTrackList class into DOMMediaStream as a general interface.

The logic for adding MediaTracks to the MediaTrackList when
MediaStreamTracks are added or removed from a DOMMediaStream is moved to
HTMLMediaElement as this fits the model better - HTMLMediaElement is the
owner of the MediaTrackLists.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 50 • 9 years ago

Bug 1103188 - Part 6. End playback in HTMLMediaElement when a stream we are playing no longer has any tracks. r?roc

	Andreas Pehrson [:pehrsons] Assignee
	Comment 51 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 52 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 53 • 9 years ago

Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

Changing order of commits in mozreview might not be such a good idea.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 54 • 9 years ago

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

Carries r=roc

	Andreas Pehrson [:pehrsons] Assignee
	Comment 55 • 9 years ago

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

Carries r=smaug,jib

	Andreas Pehrson [:pehrsons] Assignee
	Comment 56 • 9 years ago

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

Carries r=roc.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 57 • 9 years ago

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

Not sure what went wrong here.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 58 • 9 years ago

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

Carries r=jib.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 59 • 9 years ago

Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

https://reviewboard.mozilla.org/r/19571/#review18105

lgtm with nits

::: dom/locales/en-US/chrome/dom/dom.properties:110
(Diff revision 2)
> +# LOCALIZATION NODE: Do not translate "MediaStream", "stop()" and "MediaStreamTrack"

NOTE

::: dom/media/DOMMediaStream.cpp:951
(Diff revision 2)
> +  nsIDocument* document = nullptr;
> +  if (pWindow) {
> +    document = pWindow->GetExtantDoc();
> +  }

Don't like ? : ?

::: dom/media/DOMMediaStream.cpp:955
(Diff revision 2)
> +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> +                                  NS_LITERAL_CSTRING("Media"),
> +                                  document,

Is ReportToConsole ok to call with a nullptr document?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 60 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 61 • 9 years ago

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

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 62 • 9 years ago

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

https://reviewboard.mozilla.org/r/20179/#review18115

lgtm!

::: dom/media/tests/mochitest/mediaStreamPlayback.js:188
(Diff revision 1)
> +  stopTracksForStreamInMediaPlayback: {
> +    value: function () {
> +      return new Promise((resolve, reject) => {

Long Promise constructor executor function. How about:

  stopTracksForStreamInMediaPlayback: {
    value: function () {
      var stop = () => new Promise(resolve => {
        this.mediaElement.addEventListener('ended', function stopped() {
          this.mediaElement.removeEventListener('ended', stopped);
          resolve();
        });
        // TODO Also check that all the tracks are local.
        this.mediaStream.getTracks().forEach(t => t.stop());
      });
      var timeout = (ms, msg) => wait(ms).then(() => Promise.reject(new Error(msg)));

      return Promise.race([stop(),
                           timeout(ENDED_TIMEOUT_LENGTH, "ended event never fired")])
        .then(() => ok(true, "ended event successfully fired"));
    },

? hmm, almost more code, but more self explanatory maybe? dunno

	Andreas Pehrson [:pehrsons] Assignee
	Comment 63 • 9 years ago

https://reviewboard.mozilla.org/r/19571/#review18105

> NOTE

Wups.

> Don't like ? : ?

Yes I do :)
Fixed.

> Is ReportToConsole ok to call with a nullptr document?

Yes. I didn't investigate how nullptr is handled but it's for instance done here: https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/dom/json/nsJSON.cpp#57

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 64 • 9 years ago

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

https://reviewboard.mozilla.org/r/20181/#review18119

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 65 • 9 years ago

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

https://reviewboard.mozilla.org/r/18417/#review18121

	Andreas Pehrson [:pehrsons] Assignee
	Comment 66 • 9 years ago

https://reviewboard.mozilla.org/r/20179/#review18115

> Long Promise constructor executor function. How about:
> 
>   stopTracksForStreamInMediaPlayback: {
>     value: function () {
>       var stop = () => new Promise(resolve => {
>         this.mediaElement.addEventListener('ended', function stopped() {
>           this.mediaElement.removeEventListener('ended', stopped);
>           resolve();
>         });
>         // TODO Also check that all the tracks are local.
>         this.mediaStream.getTracks().forEach(t => t.stop());
>       });
>       var timeout = (ms, msg) => wait(ms).then(() => Promise.reject(new Error(msg)));
> 
>       return Promise.race([stop(),
>                            timeout(ENDED_TIMEOUT_LENGTH, "ended event never fired")])
>         .then(() => ok(true, "ended event successfully fired"));
>     },
> 
> ? hmm, almost more code, but more self explanatory maybe? dunno

I made another alternative where I moved the timeout logic to head.js. Up for you to review soon.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 67 • 9 years ago

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

OK, I have to revise this slightly. When ending media elements after tracks are gone I ran into a lot of other issues because this behavior is not expected by tests (or code, like mediaElement.captureStream was originally not made with this in mind).

I think we can live with a slight hack in the addTrackRemoveTrack test for now, and I'll revisit this after we have fixed mediaElement.captureStream streams to not modify their track set on seek, and after we have implemented MediaStream's active/inactive state.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 68 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 69 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 70 • 9 years ago

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

Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

	Andreas Pehrson [:pehrsons] Assignee
	Comment 71 • 9 years ago

Comment on attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 72 • 9 years ago

Comment on attachment 8665255 [details]
MozReview Request: Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc

Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc
Other modules than MediaTrackLists may want to receive updates on a
DOMMediaStream's track set. This moves the MediaTrackListListener out of
the MediaTrackList class into DOMMediaStream as a general interface.

The logic for adding MediaTracks to the MediaTrackList when
MediaStreamTracks are added or removed from a DOMMediaStream is moved to
HTMLMediaElement as this fits the model better - HTMLMediaElement is the
owner of the MediaTrackLists.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 73 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 74 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 75 • 9 years ago

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

Carries r=smaug,jib.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 76 • 9 years ago

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

Carries r=roc.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 77 • 9 years ago

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

The interdiff is not working because of the reordered commits, so I'll just point out what got updated.

::: dom/media/tests/mochitest/head.js:360
(Diff revision 2)
> +    , wait(time).then(() => Promise.reject(new Error(timeoutMsg)))

I moved the timeout utility function to head.js per my previous comment.

::: dom/media/tests/mochitest/mediaStreamPlayback.js:203
(Diff revision 2)
> +      this.mediaStream.stop();

There's a dummy mediaStream.stop() that doesn't belong here. It's just there for now so the media element will end playback. When we get active state for MediaStreams and we fix media element captureStream to spec we can remove this.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 78 • 9 years ago

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

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

::: dom/media/tests/mochitest/mediaStreamPlayback.js:198
(Diff revision 2)
> +      // TODO Also check that all the tracks are local.
> +      this.mediaStream.getTracks().forEach(t => t.stop());
> +
> +      // XXX When we implement MediaStream.active, do not stop the stream.
> +      // We just do it now so the media element will raise 'ended'.

Maybe add bug numbers.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 79 • 9 years ago

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

::: dom/media/tests/mochitest/head.js:357
(Diff revision 2)
> +function timeout(promise, time, timeoutMsg) {
> +  return Promise.race(
> +    [ promise
> +    , wait(time).then(() => Promise.reject(new Error(timeoutMsg)))
> +    ]);
> +}

How bout

var timeout = (p, ms, msg) => Promise.race([p, wait(ms).then(() => Promise.reject(new Error(timeoutMsg)))]);

? :)

	Andreas Pehrson [:pehrsons] Assignee
	Comment 80 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 81 • 9 years ago

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

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 82 • 9 years ago

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

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

	Pulsebot
	Comment 83 • 9 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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 84 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 85 • 9 years ago

I was able to reproduce on Mac as well if I use the real camera stream intead of the fake one, and run the test a couple of times per:
> ./mach mochitest dom/media/tests/mochitest/test_getUserMedia_addTrackRemoveTrack.html --e10s --run-until-failure

I seem to be triggering an existing bug because I do gumStream.stop() and getUserMedia() repeatedly and without delays in between.

CamerasParent is failing when calling AddRenderer at [1]. In the logs it looked like some commands from CamerasChild came in the wrong order when it failed, but I'm still trying to verify exactly what's going on.

[1] https://dxr.mozilla.org/mozilla-central/rev/6256ec9113c115141aab089c45ee69438884b680/dom/media/systemservices/CamerasParent.cpp#682

	Gian-Carlo Pascutto [:gcp]
	Comment 86 • 9 years ago

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 87 • 9 years ago

(In reply to Gian-Carlo Pascutto [:gcp] from comment #86)
> Mac specific failures might be fixed by bug 1205164, which avoids race
> conditions in the Mac shared memory implementation.

Still happens. I'll dig deeper.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 88 • 9 years ago

A normal case looks like:

> 544964608[113281e80]: int mozilla::camera::CamerasChild::AllocateCaptureDevice(mozilla::camera::CaptureEngine, const char *, const unsigned int, int &)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyAllocateCaptureDevice(const int &)
> 544964608[113281e80]: Capture Device allocated: 4097
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StartCapture(mozilla::camera::CaptureEngine, const int, webrtc::CaptureCapability &, webrtc::ExternalRenderer *)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvFrameSizeChange(const int &, const int &, const int &, const int &)
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StopCapture(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()


But when it fails it looks like:

> 544964608[113281e80]: int mozilla::camera::CamerasChild::AllocateCaptureDevice(mozilla::camera::CaptureEngine, const char *, const unsigned int, int &)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyAllocateCaptureDevice(const int &)
> 544964608[113281e80]: Capture Device allocated: 4097
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StartCapture(mozilla::camera::CaptureEngine, const int, webrtc::CaptureCapability &, webrtc::ExternalRenderer *)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplyFailure()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::StopCapture(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()
> 544964608[113281e80]: int mozilla::camera::CamerasChild::ReleaseCaptureDevice(mozilla::camera::CaptureEngine, const int)
> 551325696[113550680]: virtual bool mozilla::camera::CamerasChild::RecvReplySuccess()


Now we for some reason get:
> Allocate()
> Release() <- wut?
> Start()
> Stop()
> Release()

This looks wrong. Trying to see what's causing this.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 89 • 9 years ago

We have something like this going on in MediaManager:

gUM() -> dispatches GetUserMediaTask
MSG stream finished -> dispatches MediaOperationTask with MEDIA_STOP
GetUserMediaTask runs -> calls Allocate() -> dispatches MediaOperationTask with MEDIA_START
MediaOperationTask with MEDIA_STOP runs -> calls Release() <-- this is from an old gUM request
MediaOperationTask with MEDIA_START runs -> calls Start() -> fails because camera was deallocated -> calls stop to clean up

	Andreas Pehrson [:pehrsons] Assignee
	Comment 90 • 9 years ago

Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib

	Andreas Pehrson [:pehrsons] Assignee
	Comment 91 • 9 years ago

Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib

	Andreas Pehrson [:pehrsons] Assignee
	Comment 92 • 9 years ago

Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
This is needed to avoid something like:
* [old stream] stop track 1 -> deallocate MediaDevice for track 1
* [new stream] gUM() -> allocate MediaDevice for track 1
* [old stream] stop stream -> deallocate MediaDevice for track 1
* [new stream] gUM() -> start MediaDevice for track 1 (oops, MediaDevice was no more!)

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 93 • 9 years ago

Comment on attachment 8667366 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib

https://reviewboard.mozilla.org/r/20733/#review18583

Nice cleanup.

::: dom/media/MediaManager.h:240
(Diff revision 1)
>      NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");

Maybe change to MOZ_ASSERT here too while you're at it?

::: dom/media/MediaManager.h:275
(Diff revision 1)
> -        NotifyFinished(aGraph);
> +        NS_DispatchToMainThread(
> +          NS_NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyFinished));

I suppose the invariant that guarantees that 'this' is alive when the runnable executes is that the runnable is the one removing it?

Can a listener be removed any other way?

I see the pattern repeated in e.g. GetUserMediaCallbackMediaStreamListener::Invalidate, so hopefully it's ok there too.

::: dom/media/MediaManager.h:280
(Diff revision 1)
> +          NS_NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyRemoved));

How about using lambdas here? :) See media::NewRunnableFrom if you're interested.

::: dom/media/MediaManager.cpp:2349
(Diff revision 1)
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");

Perhaps subjective, but I find the second argument to MOZ_ASSERT redundant in most cases (like here).

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 94 • 9 years ago

Comment on attachment 8667367 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib

https://reviewboard.mozilla.org/r/20735/#review18591

lgtm.

::: dom/media/MediaManager.cpp:3028
(Diff revision 1)
>  // XXX refactor to combine with Invalidate()?

remove?

::: dom/media/MediaManager.cpp:3032
(Diff revision 1)
> -  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");

Again I vote to remove the second argument.

::: dom/media/MediaManager.cpp:3038
(Diff revision 1)
> +    if (!mAudioDevice) {
> +      Invalidate();
> +    } else {

Your change here seems neutral, so this isn't a comment on your patch. Just curious:

Can mAudioDevice->GetMediaSource() == dom::MediaSourceEnum::AudioCapture here, and if so, is it ok that the code further down that does test for this and does something I don't fully understand, doesn't run in this case? I can't tell.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 95 • 9 years ago

Comment on attachment 8667368 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib

https://reviewboard.mozilla.org/r/20737/#review18605

Ship it with fix.

::: dom/media/MediaManager.cpp:3014
(Diff revision 1)
>    mStopped = true;
>  
> +  AudioDevice* audioDevice = nullptr;
> +  VideoDevice* videoDevice = nullptr;
> +  if (!mAudioStopped) {
> +    audioDevice = mAudioDevice.get();
> +  }
> +  if (!mVideoStopped) {
> +    videoDevice = mVideoDevice.get();
> +  }
> +
>    // We can't take a chance on blocking here, so proxy this to another
>    // thread.
>    // Pass a ref to us (which is threadsafe) so it can query us for the
>    // source stream info.
>    MediaManager::PostTask(FROM_HERE,
>      new MediaOperationTask(MEDIA_STOP,
>                             this, nullptr, nullptr,
> -                           mAudioDevice, mVideoDevice,
> +                           audioDevice, videoDevice,
>                             mFinished, mWindowID, nullptr));
> +  mAudioStopped = !!audioDevice;
> +  mVideoStopped = !!videoDevice;

Isn't this just:

    MediaManager::PostTask(FROM_HERE,
      new MediaOperationTask(MEDIA_STOP,
                             this, nullptr, nullptr,
                             (!mAudioStopped? audioDevice.get() : nullptr),
                             (!mVideoStopped? videoDevice.get() : nullptr),
                             mFinished, mWindowID, nullptr));
    mStopped = mAudioStopped = mVideoStopped = true;

?

::: dom/media/MediaManager.cpp:3177
(Diff revision 1)
> +    AudioDevice* audioDevice = aIsAudio  && !mAudioStopped ? mAudioDevice.get() : nullptr;
> +    VideoDevice* videoDevice = !aIsAudio && !mVideoStopped ? mVideoDevice.get() : nullptr;
>      MediaManager::PostTask(FROM_HERE,
>        new MediaOperationTask(MEDIA_STOP_TRACK,
>                               this, nullptr, nullptr,
> -                             aIsAudio  ? mAudioDevice.get() : nullptr,
> +                             audioDevice, videoDevice,
> -                             !aIsAudio ? mVideoDevice.get() : nullptr,
>                               mFinished, mWindowID, nullptr));
> +    mAudioStopped = !!audioDevice;
> +    mVideoStopped = !!videoDevice;

If mAudioStopped or mVideoStopped are already set, this code clears them, which seems wrong. How about:

    bool stopAudio = aIsAudio && !mAudioStopped;
    bool stopVideo = !aIsAudio && !mVideoStopped;

    MediaManager::PostTask(FROM_HERE,
      new MediaOperationTask(MEDIA_STOP_TRACK,
                             this, nullptr, nullptr,
                             (stopAudio? mAudioDevice.get() : nullptr)
                             (stopVideo? mVideoDevice.get() : nullptr,
                             mFinished, mWindowID, nullptr));
    mAudioStopped |= stopAudio;
    mVideoStopped |= stopVideo;

?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 96 • 9 years ago

https://reviewboard.mozilla.org/r/20733/#review18583

> I suppose the invariant that guarantees that 'this' is alive when the runnable executes is that the runnable is the one removing it?
> 
> Can a listener be removed any other way?
> 
> I see the pattern repeated in e.g. GetUserMediaCallbackMediaStreamListener::Invalidate, so hopefully it's ok there too.

Safer to pass a refptr to a lambda I guess. `this` has threadsafe refcounting so we should be good.

> How about using lambdas here? :) See media::NewRunnableFrom if you're interested.

Oh, yes! Looks much nicer.

> Perhaps subjective, but I find the second argument to MOZ_ASSERT redundant in most cases (like here).

Agree. But then I'll change all the other ones too :-)

	Andreas Pehrson [:pehrsons] Assignee
	Comment 97 • 9 years ago

https://reviewboard.mozilla.org/r/20735/#review18591

> remove?

Done.

> Your change here seems neutral, so this isn't a comment on your patch. Just curious:
> 
> Can mAudioDevice->GetMediaSource() == dom::MediaSourceEnum::AudioCapture here, and if so, is it ok that the code further down that does test for this and does something I don't fully understand, doesn't run in this case? I can't tell.

The checks below run if mAudioDevice is set, and `mAudioDevice` is only set to an instance on `Activate()`, never to nullptr. Could be a problem if you have both video and audio screen capture I suppose? Well, audio capture is experimental and behind a flag anyways.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 98 • 9 years ago

https://reviewboard.mozilla.org/r/20737/#review18605

> Isn't this just:
> 
>     MediaManager::PostTask(FROM_HERE,
>       new MediaOperationTask(MEDIA_STOP,
>                              this, nullptr, nullptr,
>                              (!mAudioStopped? audioDevice.get() : nullptr),
>                              (!mVideoStopped? videoDevice.get() : nullptr),
>                              mFinished, mWindowID, nullptr));
>     mStopped = mAudioStopped = mVideoStopped = true;
> 
> ?

You're right.

> If mAudioStopped or mVideoStopped are already set, this code clears them, which seems wrong. How about:
> 
>     bool stopAudio = aIsAudio && !mAudioStopped;
>     bool stopVideo = !aIsAudio && !mVideoStopped;
> 
>     MediaManager::PostTask(FROM_HERE,
>       new MediaOperationTask(MEDIA_STOP_TRACK,
>                              this, nullptr, nullptr,
>                              (stopAudio? mAudioDevice.get() : nullptr)
>                              (stopVideo? mVideoDevice.get() : nullptr,
>                              mFinished, mWindowID, nullptr));
>     mAudioStopped |= stopAudio;
>     mVideoStopped |= stopVideo;
> 
> ?

You're right again! I was a bit stressed when I wrote this. Thanks :)

	Karl Tomlinson (:karlt)
	Comment 99 • 9 years ago

https://reviewboard.mozilla.org/r/20733/#review18583

> Safer to pass a refptr to a lambda I guess. `this` has threadsafe refcounting so we should be good.

NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.
https://hg.mozilla.org/mozilla-central/annotate/acdb22976ff86539dc10413c5f366e1fb429a680/xpcom/glue/nsThreadUtils.h#l306

> Oh, yes! Looks much nicer.

Lambdas can look nicer but AFAIK we're missing support for move construction of lambda closures.  When the closures contain smart pointers, copy construction would produce unnecessary ref-count manipulation, which is a bit more expensive for atomic ref-counts.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 100 • 9 years ago

https://reviewboard.mozilla.org/r/20733/#review18583

> NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.
> https://hg.mozilla.org/mozilla-central/annotate/acdb22976ff86539dc10413c5f366e1fb429a680/xpcom/glue/nsThreadUtils.h#l306

Ah, great info. Then I'm switching back to NS_NewRunnableMethod() for its conciseness.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 101 • 9 years ago

Try push for m-e10s-2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b9e5d0773d
Try push (builds only) for comment fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=429346341fe5

	Pulsebot
	Comment 102 • 9 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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 103 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #99)
> NS_NewRunnableMethod() will ensure a reference is held to its aPtr argument.

Makes sense now that I read the docs and see that it's specifically written for ref-counted objects, thanks!

> Lambdas can look nicer but AFAIK we're missing support for move construction
> of lambda closures.  When the closures contain smart pointers, copy
> construction would produce unnecessary ref-count manipulation, which is a
> bit more expensive for atomic ref-counts.

Good point in this case where we don't have an nsRefPtr to start, and create one solely for passing (not always an issue in general, but I'm all for not disturbing atomic ref-counts unnecessarily).

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 104 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 105 • 9 years ago

I'm writing the site compatibility doc. This is my understanding but correct?

> navigator.mediaDevices.getUserMedia({ audio: false, video: true }).then(input => {
>   // input.stop(); // Deprecated
>   input.getVideoTracks()[0].stop(); // Recommended
> });

	Andreas Pehrson [:pehrsons] Assignee
	Comment 106 • 9 years ago

(In reply to Kohei Yoshino [:kohei] from comment #105)
> I'm writing the site compatibility doc. This is my understanding but correct?
> 
> > navigator.mediaDevices.getUserMedia({ audio: false, video: true }).then(input => {
> >   // input.stop(); // Deprecated
> >   input.getVideoTracks()[0].stop(); // Recommended
> > });

Correct!

There are still some quirks here. For instance HTMLMediaElement should raise "ended" when the stream (`input` in your example) becomes inactive (no longer has live tracks).

For now we don't have the either active/inactive state on streams or "ended" on no tracks ("ended" needs `input.stop()` to happen, or that the source ends the stream, which will happen for getUserMedia). I'm hoping to get most of this fixed in 44 but there's a dependency on bug 1172394 that might take some time to resolve.

	Kohei Yoshino [:kohei]
	Comment 107 • 9 years ago

Thanks Andreas! I just posted the site compat doc only describing MediaStream.stop for now:

https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-deprecated/

	Thijs Triemstra
	Comment 108 • 9 years ago

(In reply to Kohei Yoshino [:kohei] from comment #107)
> Thanks Andreas! I just posted the site compat doc only describing
> MediaStream.stop for now:
> 
> https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-
> deprecated/

After reading that article I started to use MediaStreamTrack.stop() in Chrome (that also deprecated MediaStream.stop() in M45) but this does not seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop() doesn't disable the camera(light) at all. Should I file a new bug for this? (its driving me insane)

	Randell Jesup [:jesup] (needinfo me)
	Comment 109 • 9 years ago

(In reply to info from comment #108)
> (In reply to Kohei Yoshino [:kohei] from comment #107)
> > Thanks Andreas! I just posted the site compat doc only describing
> > MediaStream.stop for now:
> > 
> > https://www.fxsitecompat.com/en-US/docs/2015/mediastream-stop-has-been-
> > deprecated/
> 
> After reading that article I started to use MediaStreamTrack.stop() in
> Chrome (that also deprecated MediaStream.stop() in M45) but this does not
> seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop()
> doesn't disable the camera(light) at all. Should I file a new bug for this?
> (its driving me insane)

It should work (see the snippet in comment 105 for an example).  Please verify you're seeing the set of tracks, perhaps in the debugger.  If you're pretty sure you have it right, please file a new bug, preferably with a simple testcase.

	Kohei Yoshino [:kohei]
	Comment 110 • 9 years ago

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

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

	Andreas Pehrson [:pehrsons] Assignee
	Comment 111 • 9 years ago

(In reply to info from comment #108)
> After reading that article I started to use MediaStreamTrack.stop() in
> Chrome (that also deprecated MediaStream.stop() in M45) but this does not
> seem to work in Firefox (41.0.2 on OSX 10.10), MediaStreamTrack.stop()
> doesn't disable the camera(light) at all. Should I file a new bug for this?
> (its driving me insane)

The deprecation happened in 44 so that's really from when support for track.stop() can be expected to be fully supported.

We had bug 1192170 land in 44 as well, so you need at least that version for your code to work as expected.

You can try it with Nightly (https://nightly.mozilla.org/), and please do report a bug if it doesn't work there!

	Thijs Triemstra
	Comment 113 • 9 years ago

Thanks everyone, tested it in nightly 44 and the light is off now. Any suggestions on how to check for this functionality (<44) without checking for browser/version nr? :-/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 115 • 9 years ago

If you have to support stopping the stream anyway because of old Firefox installations, checking for the existence of MediaStream.stop would be a way to stay compatible. Not sure when it will actually be removed though.

Another way would be to check for MediaStream.addTrack, but it's more of "This implementation is sufficiently up to date so stopping the tracks should be fine". At least track.stop and stream.addTrack goes hand in hand since a major reason why you'd want track.stop over stream.stop is that you can mix tracks from different streams.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 116 • 9 years ago

(In reply to Jan-Ivar Bruaroey [:jib] from comment #114)
> Is the workaround in Bug 1192170 comment 10 insufficient?

Turned out I was wrong there. Also, in the end, that bug was about the Firefox camera indicator, not the light. If you see a problem with the camera light then please file a new bug on it.

	Eric Shepherd [:sheppy]
	Comment 117 • 9 years ago

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

removeTrack() not done yet.

	pedro.spinsomewebs
	Comment 118 • 8 years ago

What's the ETA on removeTrack?