Andreas Pehrson [:pehrsons] Assignee
	Description • 9 years ago

The readyState can be `live` or `ended`. The "ended" event is raised when the readyState goes from `live` to `ended`.

	Cullen Jennings
	Comment 1 • 9 years ago

I'd like to add two more use cases to this to consider when testing it ... 1) if the user unplugs a microphone or camera when it is in active use by GUM, the state needs to go to ended and the onended callback run. 2) if the page the users goes to the browser chrome and revokes permission to share a camera or microphone that is in use, the stream also needs to go to ended.

	Maire Reavy [:mreavy]
	Comment 2 • 8 years ago

It's time to prioritize this higher and get it done.

	Maire Reavy [:mreavy]
	Comment 3 • 8 years ago

Jan-Ivar, Andreas -- One of you would be a great choice as bug owner for this one.  Any interest?  I'd like to get this in sooner than later.  Fx48 is probably too much of a stretch, but how about targeting Fx49?

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

I think it's mostly easy, but I'm a bit scared of HTMLMediaElement and the refactoring we'll have to do wrt active streams (media element shall now end when stream gets inactive (= no live tracks) instead of on the stream's own "ended" event - which we need to kill).

I hope and think we'll find that MediaStreamTrack.onended can be implemented before that refactor however.

I can take it on, but first I'm gonna land bug 1208371.

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

wfm

	Maire Reavy [:mreavy]
	Comment 6 • 8 years ago

Thanks, Andreas.  I'll assign this to you now, but I realize you won't start work until cloning is landed.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 7 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/49999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49999/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/50001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50001/

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

Review commit: https://reviewboard.mozilla.org/r/50003/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50003/

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

Review commit: https://reviewboard.mozilla.org/r/50005/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50005/

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/1-2/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/1-2/

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

Review commit: https://reviewboard.mozilla.org/r/50469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50469/

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

Review commit: https://reviewboard.mozilla.org/r/50471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50471/

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

Review commit: https://reviewboard.mozilla.org/r/50473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50473/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 16 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/50475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50475/

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/1-2/

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/1-2/

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/2-3/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/2-3/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 21 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d028f504cd99

	Randell Jesup [:jesup] (needinfo me)
	Comment 22 • 8 years ago

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

https://reviewboard.mozilla.org/r/50471/#review47237

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

https://reviewboard.mozilla.org/r/49999/#review47243

lgtm with one issue fixed.

::: dom/media/tests/mochitest/mediaStreamPlayback.js:58
(Diff revision 2)
> -    // TODO (bug 910249) Also check that all the tracks are local.
> -    this.mediaStream.getTracks().forEach(t => t.stop());
> +    this.mediaStream.getTracks().filter(t => t.readyState == "live").forEach(t => {
> +      t.addEventListener("ended", function endedTrackUnexpected() {
> +        ok(false, "Unexpected ended event for track " + t.id);
> +      });
> +      t.stop();
> +      is(t.readyState, "ended", "Stopped track " + t.id + " should be ended");
> +    });

This library function should remove the event listeners it adds. 

Since it's verifying that events are *not* fired, this may require refactoring a bit to give it time to fire before removing the event listeners. Would prefer this happened before the returned promise is resolved.

There's also a change here where .stop is now only called on live tracks where before it was called on all tracks. I suppose this is ok.

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

https://reviewboard.mozilla.org/r/50001/#review47259

::: dom/media/tests/mochitest/pc.js:136
(Diff revision 2)
> +        .map(receiver => timerGuard(new Promise(resolve => {
> +          info("Waiting for track " + receiver.track.id + " (" +
> +               receiver.track.kind + ") to end.");
> +          receiver.track.addEventListener("ended", function trackEnded() {
> +            info("Track " + receiver.track.id + " ended.");
> +            is(receiver.track.readyState, "ended",
> +               "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event");
> +            resolve();
> +          })
> +        }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end")

I prefer promise constructor executor functions to be as minimal as possible.

How about a central helper for this repeating pattern:

    var haveEvent = (target, name) => new Promise(resolve =>
      target.addEventListener(name, function listener(event) {
        target.removeEventListener(name, listener);
        resolve(event);
      }));

then use it here like this:

        .map(receiver => {
          info("Waiting for track " + receiver.track.id + " (" +
               receiver.track.kind + ") to end.");
          return timerGuard(haveEvent(receiver.track, "ended").then(e => {
            info("Track " + receiver.track.id + " ended.");
            is(receiver.track.readyState, "ended",
               "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event");
          }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end");
        });

That takes care of removing the listener as well.

::: dom/media/tests/mochitest/pc.js:467
(Diff revision 2)
> +    .then(() => finished())
> +    .catch(e => {
> -           ok(false, 'Error in test execution: ' + e +
> +      ok(false, 'Error in test execution: ' + e +
> -              ((typeof e.stack === 'string') ?
> +         ((typeof e.stack === 'string') ?
> -               (' ' + e.stack.split('\n').join(' ... ')) : '')));
> +          (' ' + e.stack.split('\n').join(' ... ')) : ''));
> +      finished();
> +    });

This seems like it would run finished() twice if there's ever a coding error in finished.

Not sure what problem you experienced here, but I would have two catch statements instead, one before finished and one after.

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

https://reviewboard.mozilla.org/r/50003/#review47265

::: dom/media/tests/mochitest/test_getUserMedia_trackEnded.html:25
(Diff revision 3)
> +    return new Promise((resolve, reject) =>
> +        iframe.contentDocument.gUM({audio: true, video: true}, resolve, reject))

A comment on the 10-foot pole here might be nice to explain the iframe promise-chain problem you ran into.

::: dom/media/tests/mochitest/test_getUserMedia_trackEnded.html:31
(Diff revision 3)
> +          return new Promise(resolve => t.onended = e => {
> +            info("onended handler invoked for track " + t.id);
> +            is(e.target, t, "Target should be correct");
> +            resolve();
> +          });

I'd prefer:

    return new Promise(resolve => t.onended = resolve).then(e => {
      info("onended handler invoked for track " + t.id);
      is(e.target, t, "Target should be correct");
    });

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

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

https://reviewboard.mozilla.org/r/50469/#review47273

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

https://reviewboard.mozilla.org/r/50005/#review47277

lgtm.

::: dom/media/MediaStreamTrack.cpp:354
(Diff revision 3)
> +  NS_DispatchToMainThread(NewRunnableFrom([self]() {
> +    MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended")));

I suppose that's cleaner than:

  NS_DispatchToMainThread(NewRunnableFrom([this, self]() {
    MOZ_ALWAYS_SUCCEEDS(DispatchTrustedEvent(NS_LITERAL_STRING("ended")));

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

https://reviewboard.mozilla.org/r/50005/#review47279

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

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

https://reviewboard.mozilla.org/r/50473/#review47281

::: dom/media/MediaStreamTrack.h:269
(Diff revision 1)
> +  /**
> +   * Forces the ready state to a particular value, for instance when we're
> +   * cloning an already ended track.
> +   */
> +  void SetReadyState(MediaStreamTrackState aState) { mReadyState = aState; }

For encapsulation it seems perhaps a bit unnecessary to expose these setters just for cloning, when CloneInternal (or some central version of it) theoretically could have taken care of this.

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

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

https://reviewboard.mozilla.org/r/50475/#review47289

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html:79
(Diff revision 1)
>    // Handle both MediaErrors (with the `code` attribute) and other errors.
>    .catch(e => ok(false, "Error (" + e + ")" +
>                          (e.code ? " (code=" + e.code + ")" : "") +
>                          " in test for " + media.name +
>                          (e.stack ? ":\n" + e.stack : "")))
> +  .then(() => test ? test.close() : null)

null seems arbitrary. Why not:

    .then(() => test && test.close())

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html
(Diff revision 1)
>                          (e.code ? " (code=" + e.code + ")" : "") +
>                          " in test for " + media.name +
>                          (e.stack ? ":\n" + e.stack : "")))
> +  .then(() => test ? test.close() : null)
>    .then(() => {
> -    if (test) { test.close(); }

Gah, that's what we get when we overload similar-sounding APIs with different semantics (not your fault, but gah).

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

Overall lgtm and works. Nice! Probably should've just r+ed the two remaining tests, just figured I'd see them again if you go for the changes I suggested. Let me know.

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

https://reviewboard.mozilla.org/r/50001/#review47295

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

https://reviewboard.mozilla.org/r/49999/#review47297

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

https://reviewboard.mozilla.org/r/49999/#review47243

> This library function should remove the event listeners it adds. 
> 
> Since it's verifying that events are *not* fired, this may require refactoring a bit to give it time to fire before removing the event listeners. Would prefer this happened before the returned promise is resolved.
> 
> There's also a change here where .stop is now only called on live tracks where before it was called on all tracks. I suppose this is ok.

I'll call it on all tracks again. stop() should be ignored if a track is ended and we'd like to walk that path.

I am now removing the listeners. I'll let you review again, I put in an arbitrary delay of half a second.

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

https://reviewboard.mozilla.org/r/50001/#review47259

> I prefer promise constructor executor functions to be as minimal as possible.
> 
> How about a central helper for this repeating pattern:
> 
>     var haveEvent = (target, name) => new Promise(resolve =>
>       target.addEventListener(name, function listener(event) {
>         target.removeEventListener(name, listener);
>         resolve(event);
>       }));
> 
> then use it here like this:
> 
>         .map(receiver => {
>           info("Waiting for track " + receiver.track.id + " (" +
>                receiver.track.kind + ") to end.");
>           return timerGuard(haveEvent(receiver.track, "ended").then(e => {
>             info("Track " + receiver.track.id + " ended.");
>             is(receiver.track.readyState, "ended",
>                "Track " + receiver.track.id + " should have readyState 'ended' after 'ended' event");
>           }), 50000, "MediaStreamTrack " + receiver.track.id + " didn't end");
>         });
> 
> That takes care of removing the listener as well.

In case we time out instead of see the event, we don't remove the listener and might log results (the is()) after the test has finished. Thoughts on that?

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/2-3/

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/2-3/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 38 • 8 years ago

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/3-4/

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

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/1-2/

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

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/1-2/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 41 • 8 years ago

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/3-4/

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

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/1-2/

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

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/1-2/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 44 • 8 years ago

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

https://reviewboard.mozilla.org/r/50005/#review48401

::: dom/media/MediaStreamTrack.cpp:358
(Diff revision 4)
> +  RefPtr<MediaStreamTrack> self = this;
> +  NS_DispatchToMainThread(NewRunnableFrom([self]() {
> +    MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended")));
> +    return NS_OK;
> +  }));
> +

(new AsyncEventDispatcher(this, NS_LITERAL_STRING("ended"), false))->PostDOMEvent();
is simpler and less weird since it doesn't rely on lambdas.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 45 • 8 years ago

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

https://reviewboard.mozilla.org/r/50473/#review48433

::: dom/media/MediaStreamTrack.cpp:361
(Diff revision 2)
>    NS_DispatchToMainThread(NewRunnableFrom([self]() {
>      MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended")));
>      return NS_OK;
>    }));
>  
> -  mEnded = true;
> +  mReadyState = MediaStreamTrackState::Ended;

this is not what the spec says should happen.
readyState should be set 'ended' during the same task in which the "ended" event is fired, in other words, queue a task which first sets readyState to 'ended' and then _synchronously_ dispatch simple event named "ended".
This difference between the patch and the spec is minor but observable from scripts.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 46 • 8 years ago

That means that my suggestion to use AsyncEventDispatcher won't quite work.
I would just have a method in MediaStreamTrack to set the readyState and then dispatch event synchronously and use plain normal runnable method trigger it, 
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#281

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

https://reviewboard.mozilla.org/r/50473/#review48433

> this is not what the spec says should happen.
> readyState should be set 'ended' during the same task in which the "ended" event is fired, in other words, queue a task which first sets readyState to 'ended' and then _synchronously_ dispatch simple event named "ended".
> This difference between the patch and the spec is minor but observable from scripts.

Aye, good catch. I'll fix.

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/3-4/

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/3-4/

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/4-5/

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

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/2-3/

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

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/2-3/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/4-5/

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

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/2-3/

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

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/2-3/

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

(In reply to Olli Pettay [:smaug] from comment #45)
> >    NS_DispatchToMainThread(NewRunnableFrom([self]() {
> >      MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended")));
> >      return NS_OK;
> >    }));

FWIW I actually thought this is a great use of a lambda. Very readable and flexible (the fact that you can fix which task mReadyState is set from by simply moving a line, seems proof of the latter).

I agree NewRunnableMethod() is another good pattern sometimes, but limited to only calling methods without arguments AFAICT, so if you end up defining a method you otherwise don't need just to use it, then it's not a good fit IMHO.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=114ca1fc9c51#753

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 57 • 8 years ago

Lambda is rather error prone, which is why we have checks to ensure refcounted objects are handled properly. Though, I guess those checks remove the most obvious security hazards.

RunnableMethod can take arguments.
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=114ca1fc9c51#794

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 58 • 8 years ago

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

https://reviewboard.mozilla.org/r/50473/#review48461

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

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #35)
> > How about a central helper for this repeating pattern:
> 
> In case we time out instead of see the event, we don't remove the listener
> and might log results (the is()) after the test has finished. Thoughts on
> that?

Good point. Here's my second attempt:

    var haveEvent = (target, name, cancelPromise) => {
      var listener;
      var p = Promise.race([
        (cancelPromise || new Promise()).then(Promise.reject),
        new Promise(resolve => target.addEventListener(name, listener = resolve))
      ]);
      p.then(() => target.removeEventListener(name, listener));
      return p;
    };

Example usage:

    haveEvent(receiver.track, "ended", wait(500))
    .then(event => info("ended fired"), e => e? Promise.reject(e) : info("ended never fired."))

This is basically a cancel pattern, where I'm passing in a cancel signal in the form of another promise.

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

https://reviewboard.mozilla.org/r/49999/#review48459

::: dom/media/tests/mochitest/mediaStreamPlayback.js:58
(Diff revisions 2 - 4)
> -    this.mediaStream.getTracks().filter(t => t.readyState == "live").forEach(t => {
> -      t.addEventListener("ended", function endedTrackUnexpected() {
> -        ok(false, "Unexpected ended event for track " + t.id);
> -      });
> +    var noTrackEnded = Promise.all(this.mediaStream.getTracks().map(t => new Promise((resolve, reject) => {
> +      let endedTrackUnexpected = () =>
> +        (cleanup(), reject(new Error("Unexpected ended event for track " + t.id)));
> +      let cleanup = () => t.removeEventListener("ended", endedTrackUnexpected);
> +
> +      t.addEventListener("ended", endedTrackUnexpected);
> +      wait(500)
> +        .then(() => cleanup())
> +        .then(() => ok(true, "Ended event did not occur after stop() for any track"))
> +        .then(() => resolve());
> +
>        t.stop();
>        is(t.readyState, "ended", "Stopped track " + t.id + " should be ended");
> -    });
> +    })));

This is still the promise constructor anti-pattern [1] which is hard to read and I think we should avoid. Exhibit A is the lack of error handling if there's a coding error in e.g. wait, ok or cleanup.

Can we use what I suggest in comment 59?

[1] http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it

::: dom/media/tests/mochitest/mediaStreamPlayback.js:64
(Diff revisions 2 - 4)
> -      t.addEventListener("ended", function endedTrackUnexpected() {
> -        ok(false, "Unexpected ended event for track " + t.id);
> -      });
> +      let endedTrackUnexpected = () =>
> +        (cleanup(), reject(new Error("Unexpected ended event for track " + t.id)));
> +      let cleanup = () => t.removeEventListener("ended", endedTrackUnexpected);
> +
> +      t.addEventListener("ended", endedTrackUnexpected);
> +      wait(500)

wait(0) would probably suffice. I think we just want a crank of the event loop to be sure.

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

https://reviewboard.mozilla.org/r/50001/#review48469

::: dom/media/tests/mochitest/pc.js:471
(Diff revisions 2 - 4)
> -    .then(() => finished())
> +    .catch(e =>
> -    .catch(e => {
>        ok(false, 'Error in test execution: ' + e +
>           ((typeof e.stack === 'string') ?
> -          (' ' + e.stack.split('\n').join(' ... ')) : ''));
> -      finished();
> +          (' ' + e.stack.split('\n').join(' ... ')) : '')))
> +    .then(() => finished())

or just: .then(finished)

	Andreas Pehrson [:pehrsons] Assignee
	Comment 62 • 8 years ago

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/4-5/

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/4-5/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 64 • 8 years ago

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/5-6/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 65 • 8 years ago

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/3-4/

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

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/3-4/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/5-6/

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

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/3-4/

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

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/3-4/

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

(In reply to Cullen Jennings from comment #1)
> I'd like to add two more use cases to this to consider when testing it ...
> 1) if the user unplugs a microphone or camera when it is in active use by
> GUM, the state needs to go to ended and the onended callback run. 2) if the
> page the users goes to the browser chrome and revokes permission to share a
> camera or microphone that is in use, the stream also needs to go to ended.

1) Per legacy behavior we automatically switch over to the next device. The exact logic is unclear to me. I'll file a bug.
2) Works.

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

I changed to haveEvent here as well. Just have a look at the diff (and ignore the rebase leftovers), thanks!

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Same here, and the rebase leftovers are quite long but nothing I've touched.

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

https://reviewboard.mozilla.org/r/49999/#review49141

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

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

https://reviewboard.mozilla.org/r/50001/#review49147

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

https://reviewboard.mozilla.org/r/50003/#review49149

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

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> (In reply to Cullen Jennings from comment #1)
> > I'd like to add two more use cases to this to consider when testing it ...
> > 1) if the user unplugs a microphone or camera when it is in active use by
> > GUM, the state needs to go to ended and the onended callback run. 2) if the
> > page the users goes to the browser chrome and revokes permission to share a
> > camera or microphone that is in use, the stream also needs to go to ended.
> 
> 1) Per legacy behavior we automatically switch over to the next device. The
> exact logic is unclear to me. I'll file a bug.

I could be wrong here. A Macbook, which I tested, masks a TRRS connected device and the built in microphone as the same I think - since they both never show up at the same time in the gUM prompt.

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/5-6/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 78 • 8 years ago

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/5-6/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 79 • 8 years ago

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/6-7/

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

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/4-5/

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

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/4-5/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 82 • 8 years ago

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/6-7/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 83 • 8 years ago

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/4-5/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 84 • 8 years ago

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/4-5/

	Pulsebot
	Comment 85 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6614cb53f0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09fd2894b02
https://hg.mozilla.org/integration/mozilla-inbound/rev/e24de2c6fdb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/133c1cb39c00
https://hg.mozilla.org/integration/mozilla-inbound/rev/1709366abefd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3ed1091455
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee525e8953bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d446651351

	Pulsebot
	Comment 86 • 8 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/547b5816e80b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ba366db4cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/27acc73002f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b38c73d507
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e742653c70
https://hg.mozilla.org/integration/mozilla-inbound/rev/148472ab356d
https://hg.mozilla.org/integration/mozilla-inbound/rev/75157b3d9903
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4882d8c454

	Carsten Book [:Tomcat]
	Comment 87 • 8 years ago

backed this out since this somehow seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=27836453&repo=mozilla-inbound

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

Bug 1019579 broke the "ended" event since it exposes a track that in some cases never gets an underlying track to go live. We rely on the underlying track ending for ending the MediaStreamTrack.

I tried to hack it by forcing the track to end when we close the PeerConnection, without the need for an underlying track.

The only problem is, if that MediaStreamTrack was cloned, the clone won't end.

Byron, you want to try fixing it up so we can guarantee that the underlying track gets created after all?

Randell, can we live with this flaw for now (i.e., tell the failing tests to not expect "ended")?

	Byron Campen [:bwc]
	Comment 89 • 8 years ago

I can try to move the responsibility for creation of the underlying track away from MediaPipeline. I'll have to give some thought to the best way to do this.

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

(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 8748677 [details]
> MozReview Request: Bug 1208373 - Implement MediaStreamTrack.readyState.
> r?smaug,jib
> 
> https://reviewboard.mozilla.org/r/50473/#review48433
> 
> ::: dom/media/MediaStreamTrack.cpp:361
> (Diff revision 2)
> >    NS_DispatchToMainThread(NewRunnableFrom([self]() {
> >      MOZ_ALWAYS_SUCCEEDS(self->DispatchTrustedEvent(NS_LITERAL_STRING("ended")));
> >      return NS_OK;
> >    }));
> >  
> > -  mEnded = true;
> > +  mReadyState = MediaStreamTrackState::Ended;
> 
> this is not what the spec says should happen.
> readyState should be set 'ended' during the same task in which the "ended"
> event is fired, in other words, queue a task which first sets readyState to
> 'ended' and then _synchronously_ dispatch simple event named "ended".
> This difference between the patch and the spec is minor but observable from
> scripts.

This caused a race condition (bug 1275201). Seeing though that NotifyEnded() is already the only thing called after a previous dispatch from MediaStreamGraph to main thread, per [1]. Are we fine to just set the readyState and raise "ended" synchronously in MediaStreamTrack::NotifyEnded()?


[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/media/DOMMediaStream.cpp#199

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 91 • 8 years ago

yeah, sounds like setting the readyState first and then dispatching event synchronously should be fine, assuming MediaStreamTrack can handle synchronous event dispatching (like not having raw pointers on stack and using them after dispatch and stuff like that.)

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

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/6-7/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 93 • 8 years ago

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/6-7/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 94 • 8 years ago

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/7-8/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 95 • 8 years ago

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/5-6/

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

Comment on attachment 8748676 [details]
MozReview Request: Bug 1208373 - End received audio tracks on closing of PeerConnection. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50471/diff/5-6/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/7-8/

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

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/5-6/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 99 • 8 years ago

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/5-6/

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

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

The "ended" dispatch is now synchronous.

Calls start from DOMMediaStream::OwnedStreamListener::DoNotifyTrackEnded, which is Dispatched from MediaStreamGraph to main thread by DOMMediaStream::OwnedStreamListener::NotifyQueuedTrackChanges.

Smaug, could you just check that the interdiff looks fine? Thanks!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 101 • 8 years ago

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

https://reviewboard.mozilla.org/r/50005/#review51542

assuming the interdiff is between 7-8, then looks ok. state is updated first.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 102 • 8 years ago

This lets us notify about a created TrackUnionStream track (and since it was
created, we can notify when it ends), even though it has been blocked from main
thread.

Review commit: https://reviewboard.mozilla.org/r/58540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58540/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 103 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58542/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 104 • 8 years ago

Comment on attachment 8747701 [details]
Bug 1208373 - Check that we don't get "ended" event for tracks after calling stop().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49999/diff/7-8/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 105 • 8 years ago

Comment on attachment 8747702 [details]
Bug 1208373 - Test that a peerConnection's received tracks end on close().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50001/diff/7-8/

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

Comment on attachment 8747703 [details]
Bug 1208373 - Add test for MediaStreamTrack "ended" event and "readyState" attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50003/diff/8-9/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 107 • 8 years ago

Comment on attachment 8748675 [details]
Bug 1208373 - Test that ended tracks that are cloned are also ended.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50469/diff/6-7/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 108 • 8 years ago

Comment on attachment 8747704 [details]
Bug 1208373 - Implement MediaStreamTrack's "ended" event and onended EventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50005/diff/8-9/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 109 • 8 years ago

Comment on attachment 8748677 [details]
Bug 1208373 - Implement MediaStreamTrack.readyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50473/diff/6-7/

	Andreas Pehrson [:pehrsons] Assignee
	Comment 110 • 8 years ago

Comment on attachment 8748678 [details]
Bug 1208373 - Fix test_peerConnection_capturedVideo.html to wait for close() promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50475/diff/6-7/

	Randell Jesup [:jesup] (needinfo me)
	Comment 111 • 8 years ago

Comment on attachment 8761251 [details]
Bug 1208373 - Introduce a new blocking mode to MediaInputPort.

https://reviewboard.mozilla.org/r/58540/#review55414

I also wish there were a simpler way, but I think this will work

::: dom/media/DOMMediaStream.h:308
(Diff revision 1)
> -    already_AddRefed<media::Pledge<bool, nsresult>> BlockSourceTrackId(TrackID aTrackId);
> +    already_AddRefed<media::Pledge<bool, nsresult>>
> +    BlockSourceTrackId(TrackID aTrackId, BlockingMode aBlockingMode);

Nit: I'd indent the second line

	Randell Jesup [:jesup] (needinfo me)
	Comment 112 • 8 years ago

Comment on attachment 8761252 [details]
Bug 1208373 - Don't remove tracks from StreamTracks. Just their content.

https://reviewboard.mozilla.org/r/58542/#review55416

	Pulsebot
	Comment 113 • 8 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b077be047c0
Check that we don't get "ended" event for tracks after calling stop(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea733590b886
Test that a peerConnection's received tracks end on close(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a53787bf0f7
Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/816141c9f43f
Test that ended tracks that are cloned are also ended. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8e3a0dbff3
Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5684b6362e
Implement MediaStreamTrack.readyState. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2842b199f5
Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/5454dcaa31ff
Introduce a new blocking mode to MediaInputPort. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/c864d8ec3ba3
Don't remove tracks from StreamTracks. Just their content. r=jesup

	Pulsebot
	Comment 114 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba996f363fe
Backed out changeset c864d8ec3ba3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9177f45a2521
Backed out changeset 5454dcaa31ff 
https://hg.mozilla.org/integration/mozilla-inbound/rev/32325c1ec786
Backed out changeset 7f2842b199f5 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf1df26497d
Backed out changeset 2b5684b6362e 
https://hg.mozilla.org/integration/mozilla-inbound/rev/156b087287f5
Backed out changeset 9c8e3a0dbff3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b15328a193
Backed out changeset 816141c9f43f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6dd61ac523
Backed out changeset 1a53787bf0f7 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7602995ca0f
Backed out changeset ea733590b886 
https://hg.mozilla.org/integration/mozilla-inbound/rev/df403befe9fe
Backed out changeset 2b077be047c0 for test failures in test_all_synthetic_events.html on a CLOSED TREE

	Carsten Book [:Tomcat]
	Comment 115 • 8 years ago

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df403befe9fe76dea67ca4a0fb4386c6cab6bf93 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29886477&repo=mozilla-inbound

	Pulsebot
	Comment 116 • 8 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4827087a333
Check that we don't get "ended" event for tracks after calling stop(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddb73097cfd
Test that a peerConnection's received tracks end on close(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8a0b464701
Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc120c9071bb
Test that ended tracks that are cloned are also ended. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8c5a9bb9d5
Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cffa26910a8
Implement MediaStreamTrack.readyState. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cf70065470
Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e3e48c8dd0
Introduce a new blocking mode to MediaInputPort. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/95412432bf10
Don't remove tracks from StreamTracks. Just their content. r=jesup

	Carsten Book [:Tomcat]
	Comment 117 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c4827087a333
https://hg.mozilla.org/mozilla-central/rev/5ddb73097cfd
https://hg.mozilla.org/mozilla-central/rev/fb8a0b464701
https://hg.mozilla.org/mozilla-central/rev/bc120c9071bb
https://hg.mozilla.org/mozilla-central/rev/9a8c5a9bb9d5
https://hg.mozilla.org/mozilla-central/rev/8cffa26910a8
https://hg.mozilla.org/mozilla-central/rev/72cf70065470
https://hg.mozilla.org/mozilla-central/rev/38e3e48c8dd0
https://hg.mozilla.org/mozilla-central/rev/95412432bf10

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 118 • 8 years ago

Backed out this patch stack for frequent failures in test_peerConnection_addtrack_removetrack_events.html on Android 4.3 debug:

bug 1208373: https://hg.mozilla.org/integration/mozilla-inbound/rev/d433bc994ae2885a7269df4ef0969af89888c1ee
bug 1274221: https://hg.mozilla.org/integration/mozilla-inbound/rev/57453dbd063c0711348d6631cd7fc330346ef5f0
bug 1208328: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8017df8752dc1bb7862dbdfe2fc0c99bfa2c146

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30078129&repo=mozilla-inbound
09:27:59     INFO -  164 INFO Run step 80: PC_REMOTE_VERIFY_ICE_GATHERING
09:27:59     INFO -  165 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) received local trickle ICE candidates
09:27:59     INFO -  166 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) ICE gathering state is not 'new'
09:27:59     INFO -  167 INFO Run step 81: PC_LOCAL_WAIT_FOR_MEDIA_FLOW
09:27:59     INFO -  168 INFO Checking data flow to element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  169 INFO Checking data flow to element: pcLocal_local_{b133a0dc-5654-48b0-b332-8ce7a4021329}
09:27:59     INFO -  170 INFO Checking RTP packet flow for track {6f20246f-4b74-4d72-b1e7-0770749f4d15}
09:27:59     INFO -  171 INFO Checking RTP packet flow for track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96}
09:27:59     INFO -  172 INFO Element pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262} saw 'timeupdate', currentTime=65.03619047619047s, readyState=4
09:27:59     INFO -  173 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Media flowing for element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.


174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
267 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
321 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
323 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
330 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
342 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Number of ICE connections according to stats is not zero - Result logged after SimpleTest.finish()
343 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | stats reports exactly 1 ICE connection - Result logged after SimpleTest.finish()
344 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
345 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
346 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
347 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
PROCESS-CRASH | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | application crashed [@ mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::~Log]
06-13 09:30:13.895 F/MOZ_Assert( 2104): Assertion failure: false (An assert from the graphics logger), at /builds/slave/m-in-and-api-15-d-000000000000/build/src/gfx/2d/Logging.h:519

	Pulsebot
	Comment 119 • 8 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa149cf4393
Check that we don't get "ended" event for tracks after calling stop(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/528831bae79e
Test that a peerConnection's received tracks end on close(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3948698940
Add test for MediaStreamTrack "ended" event and "readyState" attribute. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/53dc43787dc3
Test that ended tracks that are cloned are also ended. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f07d349d5e
Implement MediaStreamTrack's "ended" event and onended EventHandler. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/48581a9670a1
Implement MediaStreamTrack.readyState. r=smaug,jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5a51da1a85
Fix test_peerConnection_capturedVideo.html to wait for close() promise. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/3598ea9d1b25
Introduce a new blocking mode to MediaInputPort. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f38305af17
Don't remove tracks from StreamTracks. Just their content. r=jesup

	Carsten Book [:Tomcat]
	Comment 120 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0aa149cf4393
https://hg.mozilla.org/mozilla-central/rev/528831bae79e
https://hg.mozilla.org/mozilla-central/rev/fe3948698940
https://hg.mozilla.org/mozilla-central/rev/53dc43787dc3
https://hg.mozilla.org/mozilla-central/rev/51f07d349d5e
https://hg.mozilla.org/mozilla-central/rev/48581a9670a1
https://hg.mozilla.org/mozilla-central/rev/ed5a51da1a85
https://hg.mozilla.org/mozilla-central/rev/3598ea9d1b25
https://hg.mozilla.org/mozilla-central/rev/43f38305af17

	Eric Shepherd [:sheppy]
	Comment 122 • 8 years ago

New documentation created:

https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/onended

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/Events/ended (now covers both HTML media and Media Streams)

https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/readyState

Listed on Firefox 50 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/50#WebRTC

Please feel free to correct any mistakes, or let me know if there's something you think should be fixed. Thanks!

	Wang Guan
	Comment 124 • 8 years ago

Hello there,

I found that in latest Firefox 50 (macOS), when the window being shared (obtained with getUserMedia / media.getusermedia.screensharing.allowed_domains) is closed, "ended" event is not emitted from its MediaTrack.

(in Chrome, "ended" have been emitted correctly)

I'm preparing a minimal example to demonstrate this.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 125 • 8 years ago

(In reply to Wang Guan from comment #124)
> Hello there,
> 
> I found that in latest Firefox 50 (macOS), when the window being shared
> (obtained with getUserMedia /
> media.getusermedia.screensharing.allowed_domains) is closed, "ended" event
> is not emitted from its MediaTrack.
> 
> (in Chrome, "ended" have been emitted correctly)
> 
> I'm preparing a minimal example to demonstrate this.

Feel free to file a new bug with this minimal example and make it block this one.