Last Comment Bug 1103188 - Implement MediaStream.addTrack()/removeTrack()
: Implement MediaStream.addTrack()/removeTrack()
Status: RESOLVED FIXED
: dev-doc-needed, site-compat
Product: Core
Classification: Components
Component: Audio/Video: MediaStreamGraph (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal with 5 votes (vote)
: mozilla44
Assigned To: Andreas Pehrson [:pehrsons] (Telenor)
:
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
: 1021647 (view as bug list)
Depends on: 1166140 1170112 1170958
Blocks: 910249 985265 1258143 1070216 1208316 1223696
  Show dependency treegraph
 
Reported: 2014-11-21 14:45 PST by Thomas Auge
Modified: 2016-04-05 08:20 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
MozReview Request: bz://1103188/pehrsons (39 bytes, text/x-review-board-request)
2015-05-18 22:43 PDT, Andreas Pehrson [:pehrsons] (Telenor)
no flags Details | Review
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib (39 bytes, text/x-review-board-request)
2015-06-10 08:18 PDT, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Review
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc (39 bytes, text/x-review-board-request)
2015-06-10 08:18 PDT, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Review
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib (40 bytes, text/x-review-board-request)
2015-09-07 01:04 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib (40 bytes, text/x-review-board-request)
2015-09-18 00:06 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc (40 bytes, text/x-review-board-request)
2015-09-23 21:15 PDT, Andreas Pehrson [:pehrsons] (Telenor)
roc: review+
Details | Review
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc (40 bytes, text/x-review-board-request)
2015-09-23 21:15 PDT, Andreas Pehrson [:pehrsons] (Telenor)
roc: review+
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib (40 bytes, text/x-review-board-request)
2015-09-23 21:16 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib (40 bytes, text/x-review-board-request)
2015-09-23 21:16 PDT, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib (40 bytes, text/x-review-board-request)
2015-09-29 09:42 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib (40 bytes, text/x-review-board-request)
2015-09-29 09:42 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib (40 bytes, text/x-review-board-request)
2015-09-29 09:42 PDT, Andreas Pehrson [:pehrsons] (Telenor)
jib: review+
Details | Review

Description User image Thomas Auge 2014-11-21 14:45:18 PST
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 User image Iñaki Baz Castillo 2015-01-12 04:59:06 PST
Any ETA or comment about this? It is a very important feature IMHO.
Comment 2 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-05-18 18:59:04 PDT
I intend to implement addTrack() and removeTrack() as part of this bug. I have some WIP patches already.
Comment 3 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-05-18 22:43:09 PDT
Created attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons

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

Pull down these commits:

hg pull -r 3151f9e32b8ced9b72c850232bc63b74888520b5 https://reviewboard-hg.mozilla.org/gecko/
Comment 4 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-05-18 22:44:25 PDT
Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons

Still quite WIP.
Comment 5 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-06-10 08:18:32 PDT
Comment on attachment 8607342 [details]
MozReview Request: bz://1103188/pehrsons
Comment 6 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-06-10 08:18:32 PDT
Created attachment 8618701 [details]
MozReview Request: Bug 1103188 - Part 1. MediaStream WebIDL update with addTrack/removeTrack. r?smaug,jib
Comment 7 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-06-10 08:18:32 PDT
Created attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc
Comment 8 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-06-25 07:33:30 PDT
*** Bug 1021647 has been marked as a duplicate of this bug. ***
Comment 9 User image rajkumaradass 2015-08-21 02:15:49 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-08-21 02:25:46 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-02 21:31:49 PDT
See bug 1170958 comment 56 for a test we should write.
Comment 12 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-07 01:04:43 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-07 01:04:45 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-07 01:04:47 PDT
Created attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

Bug 1103188 - Part 3. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Comment 15 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-07 01:08:53 PDT
(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 User image Jan-Ivar Bruaroey [:jib] 2015-09-14 09:05:28 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-14 20:21:10 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-15 00:03:47 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-15 00:03:50 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-15 00:03:53 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-09-15 03:25:02 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-09-15 04:15:31 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-09-15 04:16:56 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-15 06:58:52 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-09-15 07:07:46 PDT
Specs don't favor space before paren, but apparently this particular spec uses unusual style.
Comment 26 User image Jan-Ivar Bruaroey [:jib] 2015-09-15 08:41:09 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-15 09:08:49 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-17 00:24:01 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-17 00:24:39 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-17 01:23:01 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:06:14 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:06:17 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:06:19 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:06:22 PDT
Created attachment 8662784 [details]
MozReview Request: Bug 1103188 - Part 4. Deprecate DOMMediaStream::Stop(). r?jib

Bug 1103188 - Part 4. Always check tracks on getUserMedia(). r?jib
Comment 35 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:06:57 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:08:02 PDT
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.
Comment 37 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-18 00:08:52 PDT
Comment on attachment 8657704 [details]
MozReview Request: Bug 1103188 - Part 3. Remove identical override nsDOMUserMediaStream::Stop(). r?jib

Carries r=jib,roc.
Comment 38 User image Jan-Ivar Bruaroey [:jib] 2015-09-18 09:03:34 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-21 23:14:24 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-21 23:17:16 PDT
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 User image Kyrylo Slatin 2015-09-21 23:26:30 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-22 00:43:19 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-22 00:54:54 PDT
(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 User image Jan-Ivar Bruaroey [:jib] 2015-09-22 07:51:03 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:49 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:51 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:53 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:55 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:56 PDT
Created attachment 8665255 [details]
MozReview Request: Bug 1103188 - Part 5. Break out MediaTrackListListener to an interface. r?roc

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

The logic for adding MediaTracks to the MediaTrackList when
MediaStreamTracks are added or removed from a DOMMediaStream is moved to
HTMLMediaElement as this fits the model better - HTMLMediaElement is the
owner of the MediaTrackLists.
Comment 50 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:15:59 PDT
Created attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

Bug 1103188 - Part 6. End playback in HTMLMediaElement when a stream we are playing no longer has any tracks. r?roc
Comment 51 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:16:01 PDT
Created attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Bug 1103188 - Part 7. MediaStream::AddTrack/RemoveTrack tests. r?jib,roc
Comment 52 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:16:03 PDT
Created attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib

Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib
Comment 53 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:17:57 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:18:33 PDT
Comment on attachment 8665257 [details]
MozReview Request: Bug 1103188 - Always check tracks on getUserMedia(). r?jib

Carries r=roc
Comment 55 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:19:12 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:19:39 PDT
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.
Comment 57 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:20:21 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 21:21:26 PDT
Comment on attachment 8665258 [details]
MozReview Request: Bug 1103188 - Part 8. Always check tracks on getUserMedia(). r?jib

Carries r=jib.
Comment 59 User image Jan-Ivar Bruaroey [:jib] 2015-09-23 21:49:56 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-09-23 22:02:40 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-09-23 22:04:27 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-23 22:41:50 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 22:43:27 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-23 22:44:05 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-23 22:48:10 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-23 23:34:17 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 02:45:24 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:08 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:11 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:13 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:15 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:16 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:19 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:50:21 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:51:37 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:52:55 PDT
Comment on attachment 8618702 [details]
MozReview Request: Bug 1103188 - Part 2. MediaStream::AddTrack/RemoveTrack implementation. r?roc

Carries r=roc.
Comment 77 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 03:58:56 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-24 05:30:27 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-24 05:35:01 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 20:58:03 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-24 20:58:05 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-25 06:51:15 PDT
Comment on attachment 8665256 [details]
MozReview Request: Bug 1103188 - MediaStream::AddTrack/RemoveTrack tests. r?jib,roc

https://reviewboard.mozilla.org/r/20177/#review18319
Comment 84 User image Wes Kocher (:KWierso) 2015-09-25 13:09:37 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-28 04:10:11 PDT
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 User image Gian-Carlo Pascutto [:gcp] 2015-09-28 06:25:44 PDT
Mac specific failures might be fixed by bug 1205164, which avoids race conditions in the Mac shared memory implementation.
Comment 87 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-28 23:12:30 PDT
(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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-28 23:25:35 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 01:37:59 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 09:42:22 PDT
Created attachment 8667366 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib

Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Comment 91 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 09:42:25 PDT
Created attachment 8667367 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib

Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Comment 92 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 09:42:28 PDT
Created attachment 8667368 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib

Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
This is needed to avoid something like:
* [old stream] stop track 1 -> deallocate MediaDevice for track 1
* [new stream] gUM() -> allocate MediaDevice for track 1
* [old stream] stop stream -> deallocate MediaDevice for track 1
* [new stream] gUM() -> start MediaDevice for track 1 (oops, MediaDevice was no more!)
Comment 93 User image Jan-Ivar Bruaroey [:jib] 2015-09-29 13:29:41 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-29 14:06:32 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-09-29 16:21:25 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 21:45:18 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 21:45:37 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-29 22:31:50 PDT
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 User image Karl Tomlinson (:karlt) 2015-09-29 22:33:37 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-30 00:03:12 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-30 00:04:25 PDT
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 103 User image Jan-Ivar Bruaroey [:jib] 2015-09-30 10:33:17 PDT
(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 105 User image Kohei Yoshino [:kohei] 2015-09-30 16:20:56 PDT
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 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-09-30 19:36:48 PDT
(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 User image Kohei Yoshino [:kohei] 2015-10-01 14:02:44 PDT
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 User image Thijs Triemstra 2015-10-17 07:30:41 PDT
(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 User image Randell Jesup [:jesup] 2015-10-18 10:41:27 PDT
(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 User image Kohei Yoshino [:kohei] 2015-10-18 15:03:25 PDT
I have updated my own app, and it worked fine.

https://github.com/bzdeck/bzdeck/commit/1859459
Comment 111 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-10-18 20:55:03 PDT
(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 User image Jan-Ivar Bruaroey [:jib] 2015-10-19 08:38:03 PDT Comment hidden (obsolete)
Comment 113 User image Thijs Triemstra 2015-10-19 11:41:57 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-10-19 13:48:35 PDT Comment hidden (obsolete)
Comment 115 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-10-20 01:35:58 PDT
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 User image Jan-Ivar Bruaroey [:jib] 2015-10-20 07:29:45 PDT
(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 User image Eric Shepherd [:sheppy] 2015-11-17 12:36:07 PST
addTrack() is now documented: https://developer.mozilla.org/en-US/docs/Web/API/MediaStream.addTrack

removeTrack() not done yet.
Comment 118 User image pedro.spinsomewebs 2016-04-05 08:20:24 PDT
What's the ETA on removeTrack?

Note You need to log in before you can comment on or make changes to this bug.