1208316 - Implement active/inactive state for MediaStreams

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

This includes:
`active` attribute on MediaStream
"active" and "inactive" events on MediaStream
HTMLMediaElement should raise "ended" event when playing a MediaStream and it goes inactive

There are a couple of special cases (hacks if you will) put in place in bug 1103188 to account for the fact that we don't have a proper active state yet. These should be removed as part of this bug.

Comment 2

[Andreas Pehrson [:pehrsons]]

The "active" and "inactive" events on MediaStream were just removed in the latest editor's draft [1].

The `active` attribute is still there, but users should listen to "addtrack" and "removetrack" events when monitoring its state (plus keep track of their own calls to `addTrack()` and `removeTrack()`).

Media elements shall still go to their "ended" state when the stream goes inactive.


[1] http://w3c.github.io/mediacapture-main/archives/20151223/getusermedia.html

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Implement MediaStream.active.

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - End a media element when its MediaStream source goes inactive.

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

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

Better semantics for what I want to do with NotifyEnded in later patches in the bug.

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

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

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

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Test media flow per track instead of per stream.

The existing media elements may have ended since we now end when all tracks
have ended. If that is the case it doesn't make sense to test them for media
flow. Instead we test all tracks in getSenders() and getReceivers()
respectively. They should all be live.

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

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

No need to do this since a media element playing the stream will now end when
all tracks have ended.

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Add dummy audio track in pure video track test.

This will keep the stream active and avoid the HTMLMediaElement ending.
We could do it by repeatedly play()ing or autoplay, but then we wouldn't be
sure the media element stopped rendering video because it went to the ended
state or because there were no more frames in the track.

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

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Test active state in MediaStream constructors and clone tests.

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

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

https://reviewboard.mozilla.org/r/77882/#review76432

::: dom/media/tests/mochitest/head.js:290
(Diff revision 1)
> +  if (element) {
> +    ok(false, "Element with id " + id + " should not be in the tree");
> +  }

Nit: I think you can shorten this to something like ok(!element, "...")

The other question is if you should return in case the element exists already, because I would expect one of the calls further below to throw an exception (the setAttribute('id',...) or the appendChild()).

::: dom/media/tests/mochitest/pc.js:1402
(Diff revision 1)
> -   * Check that media flow is present on the given media element by waiting for
> -   * it to reach ready state HAVE_ENOUGH_DATA and progress time further than
> -   * the start of the check.
> +   * Check that media flow is present for the given MediaStreamTrack by playing
> +   * it in a media element and waiting for it to reach ready state
> +   * HAVE_ENOUGH_DATA and progress time further than the start of the check.
>     *
>     * This ensures, that the stream being played is producing
> -   * data and that at least one video frame has been displayed.
> +   * data and, in case of a video track, that at least one video frame has been

"in case of a video track" suggests to me that the function differentiates somehow between audio and video. Which does not appear to be the case to me.

I guess the right description is that the function ensures that the underlying implementation emits expected events in the expected order :-)

::: dom/media/tests/mochitest/pc.js:1414
(Diff revision 1)
> -      info("Checking data flow to element: " + element.id);
> -      if (element.ended && element.readyState >= element.HAVE_CURRENT_DATA) {
> -        resolve();
> +    if (track.ended) {
> +      info("Track " + track.id + " had ended.");
> +      return Promise.resolve();

Shouldn't this return resolve() or reject() depending on whether media actually once flowed through here?

I'm concerned that a track might have ended before we even got around to check it. Or is there a legit use case for a super short lived track?

::: dom/media/tests/mochitest/pc.js:1419
(Diff revision 1)
> -      element.addEventListener("canplay", oncanplay);
> -      element.addEventListener("timeupdate", ontimeupdate);
> -    });
> +    let element = createMediaElementForTrack(track, this.label + '_' + direction);
> +    element.srcObject = new MediaStream([track]);
> +    element.play();

I'm guessing that the normal case here is that we already have video elements which render/play the audio and video stream. And this is "only" needed to be able to track the progress of each track individually.
Wouldn't it then make more sense to use individual track elements from the get go and just re-use these here?

::: dom/media/tests/mochitest/pc.js:1424
(Diff revision 1)
> +      haveEvent(element, "canplay", wait(60000, new Error("Timeout")))
> +        .then(_ => info("Track " + track.id + " saw 'canplay'.")),
> +      haveEvent(element, "timeupdate", wait(60000, new Error("Timeout")))
> +        .then(_ => isnot(element.currentTime, 0,
> +                         "Track " + track.id + " should progress currentTime."))

As it looks like the Error("Timeout") are suppose to ending up rejecting the Promises and bubble up can we make them more specific like Error("Timeout while waiting for 'canplay' even from track " + track.id)?

Comment 14

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

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

https://reviewboard.mozilla.org/r/77872/#review76496

r+ to the .webidl

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

https://reviewboard.mozilla.org/r/77882/#review76432

> Nit: I think you can shorten this to something like ok(!element, "...")
> 
> The other question is if you should return in case the element exists already, because I would expect one of the calls further below to throw an exception (the setAttribute('id',...) or the appendChild()).

Yes, I meant to return here -- that makes sense with the ok(false) :)

> "in case of a video track" suggests to me that the function differentiates somehow between audio and video. Which does not appear to be the case to me.
> 
> I guess the right description is that the function ensures that the underlying implementation emits expected events in the expected order :-)

The description is leaking info about how HTMLMediaElement works. For video we have to wait for a video frame before emitting those events, for audio don't wait for any real audio bits.

I think it's useful info, but I'll remove it if you want.

> Shouldn't this return resolve() or reject() depending on whether media actually once flowed through here?
> 
> I'm concerned that a track might have ended before we even got around to check it. Or is there a legit use case for a super short lived track?

IMO we should have coverage on tracks not ending before they're meant to end. Perhaps we don't?

This is meant to catch when a test explicitly stopped a track for other reasons but the sender or receiver is still around. Since we now test media element flow with a *new* media element the readyState will always be HAVE_NOTHING here.

> I'm guessing that the normal case here is that we already have video elements which render/play the audio and video stream. And this is "only" needed to be able to track the progress of each track individually.
> Wouldn't it then make more sense to use individual track elements from the get go and just re-use these here?

I guess that works. And solves the previous issue.

> As it looks like the Error("Timeout") are suppose to ending up rejecting the Promises and bubble up can we make them more specific like Error("Timeout while waiting for 'canplay' even from track " + track.id)?

The idea is that fitting the Error on one line weighs heavier than the detailed description since the stack trace will tell you exactly where the Timeout came from. In my experience you end up looking at the source when something like this fails anyway.

Comment 16

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

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

https://reviewboard.mozilla.org/r/77872/#review76840

Lgtm with minor nit and question.

::: dom/media/DOMMediaStream.cpp:1196
(Diff revision 1)
>  void
> +DOMMediaStream::NotifyActive()
> +{
> +  LOG(LogLevel::Info, ("DOMMediaStream %p NotifyActive(). ", this));
> +
> +  MOZ_ASSERT(Active());

Nit: I find MOZ_ASSERT(mActive) cleaner, as there's no possibility of action-at-a-distance like there is with sibling member calls.

::: dom/media/DOMMediaStream.cpp:1304
(Diff revision 1)
> -  for (int32_t i = mTrackListeners.Length() - 1; i >= 0; --i) {
> -    mTrackListeners[i]->NotifyTrackRemoved(aTrack);
> +  for (auto listener : mTrackListeners) {
> +    listener->NotifyTrackRemoved(aTrack);

Don't they need to be done in reverse order anymore?

Comment 17

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

Comment on attachment 8789805 [details]
Bug 1208316 - End a media element when its MediaStream source goes inactive.

https://reviewboard.mozilla.org/r/77874/#review76848

::: dom/html/HTMLMediaElement.cpp:4038
(Diff revision 1)
> +  {
> +    LOG(LogLevel::Debug, ("%p, mSrcStream %p became inactive",
> +                          mElement, mElement->mSrcStream.get()));
> +    MOZ_ASSERT(!mElement->mSrcStream->Active());
> +    if (mElement->mMediaStreamListener) {
> +      mElement->mMediaStreamListener->Forget();

I hope we rename that function someday ('forget' makes me think of leaks...)

Comment 18

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

Comment on attachment 8789806 [details]
Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

https://reviewboard.mozilla.org/r/77876/#review76850

Comment 19

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

Comment on attachment 8789807 [details]
Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

https://reviewboard.mozilla.org/r/77878/#review76854

lgtm.

::: dom/media/DOMMediaStream.cpp:378
(Diff revision 1)
>  DOMMediaStream::DOMMediaStream(nsPIDOMWindowInner* aWindow,
>                                 MediaStreamTrackSourceGetter* aTrackSourceGetter)
>    : mLogicalStreamStartTime(0), mWindow(aWindow),
>      mInputStream(nullptr), mOwnedStream(nullptr), mPlaybackStream(nullptr),
>      mTracksPendingRemoval(0), mTrackSourceGetter(aTrackSourceGetter),
> +    mPlaybackTrackListener(MakeAndAddRef<PlaybackTrackListener>(this)),

Nit: What's wrong with:

    mPlaybackTrackListener(new PlaybackTrackListener(this)),

?

::: dom/media/MediaStreamTrack.h:244
(Diff revision 1)
> +   * Called when the track's readyState transitions to "ended".
> +     As opposed to the "ended" event exposed to script this is called for any

missing * and ,

s/As opposed to/Unlike/ ?

::: dom/media/MediaStreamTrack.cpp:375
(Diff revision 1)
>  }
>  
> +void
> +MediaStreamTrack::NotifyEnded()
> +{
> +  MOZ_ASSERT(Ended());

mEnded?

Comment 20

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

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

https://reviewboard.mozilla.org/r/77880/#review76858

lgtm.

::: dom/media/DOMMediaStream.h:748
(Diff revision 1)
>    // True if this stream has live tracks.
>    bool mActive;
>  
> +  // True if this stream only sets mActive to false when its playback stream
> +  // finishes. This is a hack to maintain legacy functionality for playing a
> +  // HTMLMediaElement::MozCaptureStream(). See bug XXX.

Bug number?

::: dom/media/DOMMediaStream.cpp:281
(Diff revision 1)
> +      nsCOMPtr<nsIRunnable> event =
> +        NewRunnableMethod(this, &PlaybackStreamListener::DoNotifyFinished);
> +      aGraph->DispatchToMainThreadAfterStreamStateUpdate(event.forget());

or
    auto& event = NewRunnableMethod(this, &PlaybackStreamListener::DoNotifyFinished);
    aGraph->DispatchToMainThreadAfterStreamStateUpdate(event);
or
    aGraph->DispatchToMainThreadAfterStreamStateUpdate(
        NewRunnableMethod(this, &PlaybackStreamListener::DoNotifyFinished))

Comment 21

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

Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

https://reviewboard.mozilla.org/r/77884/#review76860

Comment 22

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

Comment on attachment 8789811 [details]
Bug 1208316 - Add dummy audio track in pure video track test.

https://reviewboard.mozilla.org/r/77886/#review76862

::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_video.html:75
(Diff revision 1)
> +    // We add a dummy audio track to the captureStream output stream so we can
> +    // avoid the consuming media element ending when we remove tracks and/or
> +    // sources on the input stream.

I found the rationale in the commit message helpful. Could we incorporate some of that in this comment as well?

Comment 23

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

Comment on attachment 8789812 [details]
Bug 1208316 - Test active state in MediaStream constructors and clone tests.

https://reviewboard.mozilla.org/r/77888/#review76864

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamConstructors.html:119
(Diff revision 1)
>        var osc1k = createOscillatorStream(audioContext, 1000);
> +      ok(osc1k.active, "WebAudio stream should be active");
>        var audioTrack1k = osc1k.getTracks()[0];
>  
>        var osc5k = createOscillatorStream(audioContext, 5000);
> +      ok(osc5k.active, "WebAudio stream should be active");

A couple of these seem redundant.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamConstructors.html:129
(Diff revision 1)
>        var audioTrack10k = osc10k.getTracks()[0];
>  
>        return Promise.resolve().then(() => {
>          info("Analysing audio output with empty default constructed stream");
>          var stream = new MediaStream();
> +        ok(!stream.active, "New constructed MediaStream should be inactive");

already tested.

Comment 24

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

Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

https://reviewboard.mozilla.org/r/77890/#review76866

.

    _O/\O_

Comment 25

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Notify watchers on StreamListener::Forget().

This makes sense because the result of StreamListener::NextFrameStatus depends
on mElement.

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

Comment 26

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

https://reviewboard.mozilla.org/r/77872/#review76840

> Don't they need to be done in reverse order anymore?

Ups, this change was not really intended. I went through users of NotifyTrackRemoved and we'll need the reverse index iteration for MediaRecorder.

We should probably use it for all the Notify methods just in case one of them decides to remove their TrackListener inside the Notify method.

Comment 27

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789807 [details]
Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

https://reviewboard.mozilla.org/r/77878/#review76854

> Nit: What's wrong with:
> 
>     mPlaybackTrackListener(new PlaybackTrackListener(this)),
> 
> ?

The war on raw pointers? :)

Comment 28

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

Comment on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

https://reviewboard.mozilla.org/r/78344/#review76934

lgtm.

Comment 29

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

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

Comment 30

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

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

Comment 31

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

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

Comment 32

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

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

Comment 33

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789805 [details]
Bug 1208316 - End a media element when its MediaStream source goes inactive.

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

Comment 34

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789806 [details]
Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

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

Comment 35

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789807 [details]
Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

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

Comment 36

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

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

Comment 37

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

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

Comment 38

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

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

Comment 39

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789811 [details]
Bug 1208316 - Add dummy audio track in pure video track test.

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

Comment 40

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789812 [details]
Bug 1208316 - Test active state in MediaStream constructors and clone tests.

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

Comment 41

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

Comment 42

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

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

Comment 43

[Andreas Pehrson [:pehrsons]]

Latest try [1] shows two problems:

1. Assertions in MediaEngineRemoteVideoSource. There's a patch for that on bug 1295352 so I'm making it block.
2. The other is a MOZ_ASSERT failing on Android. It's either because MSG notifies us multiple times about EVENT_FINISHED, or because of external MediaStreamTracks mixed into a non-MediaStream sourced media element's mozCaptureStream. Will need to do some thinking on that case.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e55d00b91c2

Comment 44

[Andreas Pehrson [:pehrsons]]

Also removing the dependency on bug 1172394 since I'm implementing a workaround for media element mozCaptureStream and seeking.

Comment 45

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

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

Comment 46

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

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

Comment 47

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

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

Comment 48

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789811 [details]
Bug 1208316 - Add dummy audio track in pure video track test.

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

Comment 49

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789812 [details]
Bug 1208316 - Test active state in MediaStream constructors and clone tests.

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

Comment 50

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

Comment 51

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

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

Comment 52

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791247 [details]
Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

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

Comment 53

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791248 [details]
Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

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

Comment 54

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791249 [details]
Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

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

Comment 55

[Nils Ohlmeier [:drno]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

https://reviewboard.mozilla.org/r/77882/#review77626

LGTM

::: dom/media/tests/mochitest/head.js:279
(Diff revision 3)
> +  ok(!getMediaElementForTrack(track, idPrefix),
> +     "Element for track " + track.id + " with prefix " + idPrefix +
> +     " should not be in the tree");

As createMediaElementForTrack() gets called only from one place in pc.js which calls getMediaElementForTrack() to figure out if it need to make this call I think you can safely remove this check.

Comment 56

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

I did some changes to the logic in this patch, so if you could have another look on the diff/2-3 jib, that'd be lovely, thanks.

Comment 57

[Andreas Pehrson [:pehrsons]]

Here comes a rebase.

Comment 58

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

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

Comment 59

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789805 [details]
Bug 1208316 - End a media element when its MediaStream source goes inactive.

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

Comment 60

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789806 [details]
Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

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

Comment 61

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789807 [details]
Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

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

Comment 62

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

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

Comment 63

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

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

Comment 64

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

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

Comment 65

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789811 [details]
Bug 1208316 - Add dummy audio track in pure video track test.

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

Comment 66

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789812 [details]
Bug 1208316 - Test active state in MediaStream constructors and clone tests.

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

Comment 67

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

Comment 68

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

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

Comment 69

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791247 [details]
Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

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

Comment 70

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791248 [details]
Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

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

Comment 71

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791249 [details]
Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

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

Comment 72

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

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

https://reviewboard.mozilla.org/r/77880/#review80198

::: dom/media/DOMMediaStream.cpp:56
(Diff revision 4)
>  #define LOG(type, msg) MOZ_LOG(gMediaStreamLog, type, msg)
>  
>  const TrackID TRACK_VIDEO_PRIMARY = 1;
>  
> +static bool
> +ContainsLiveTracks(nsTArray<RefPtr<DOMMediaStream::TrackPort>>& aTracks)

Nit: Could aTracks be const?

Comment 73

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

Comment on attachment 8791247 [details]
Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

https://reviewboard.mozilla.org/r/78714/#review81242

Comment 74

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

Comment on attachment 8791248 [details]
Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

https://reviewboard.mozilla.org/r/78716/#review81244

Comment 75

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

Comment on attachment 8791249 [details]
Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

https://reviewboard.mozilla.org/r/78718/#review81246

::: dom/media/tests/mochitest/test_getUserMedia_active_autoplay.html:53
(Diff revision 3)
> +  return haveEvent(video, "ended", wait(5000, new Error("Timeout")));
> +}));
> +</script>

Is this missing a final

    ok(video.ended, "Video element should be ended");

?

Comment 76

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791249 [details]
Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

https://reviewboard.mozilla.org/r/78718/#review81246

> Is this missing a final
> 
>     ok(video.ended, "Video element should be ended");
> 
> ?

Can't hurt.

Comment 77

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

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

Comment 78

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789805 [details]
Bug 1208316 - End a media element when its MediaStream source goes inactive.

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

Comment 79

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789806 [details]
Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

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

Comment 80

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789807 [details]
Bug 1208316 - Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream.

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

Comment 81

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789808 [details]
Bug 1208316 - Punch a hole for media element captureStream to only go inactive as source ends.

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

Comment 82

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789809 [details]
Bug 1208316 - Test media flow per track instead of per stream.

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

Comment 83

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

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

Comment 84

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789811 [details]
Bug 1208316 - Add dummy audio track in pure video track test.

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

Comment 85

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789812 [details]
Bug 1208316 - Test active state in MediaStream constructors and clone tests.

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

Comment 86

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

Comment 87

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

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

Comment 88

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791247 [details]
Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

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

Comment 89

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791248 [details]
Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

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

Comment 90

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8791249 [details]
Bug 1208316 - Test that a MediaStream becoming active triggers autoplay.

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

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8a8d46844b
Implement MediaStream.active. r=jib, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4fa5f1d355
End a media element when its MediaStream source goes inactive. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0d1e7b1520
Rename MediaStreamTrack::NotifyEnded to OverrideEnded. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/063898d69f9e
Route notifications of ending tracks through MediaStreamTrack instead of DOMMediaStream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/f382cedda378
Punch a hole for media element captureStream to only go inactive as source ends. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/813197b7e985
Test media flow per track instead of per stream. r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e19f844b55e
Do not stop the local MediaStream in mediaStreamPlayback.js. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd1f3da33d9
Add dummy audio track in pure video track test. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea746929e72
Test active state in MediaStream constructors and clone tests. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6f053f56f4
Simplify stopTracksForStreamInMediaPlayback. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee420e34b901
Notify watchers on StreamListener::Forget(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f44cd2bddb
HTMLMediaElement.ended should follow MediaStream.active. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/a20a1d0d0c0e
A media element should autoplay a MediaStream that becomes active. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/b52349c52e06
Test that a MediaStream becoming active triggers autoplay. r=jib

Comment 92

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b8a8d46844b
https://hg.mozilla.org/mozilla-central/rev/0d4fa5f1d355
https://hg.mozilla.org/mozilla-central/rev/ea0d1e7b1520
https://hg.mozilla.org/mozilla-central/rev/063898d69f9e
https://hg.mozilla.org/mozilla-central/rev/f382cedda378
https://hg.mozilla.org/mozilla-central/rev/813197b7e985
https://hg.mozilla.org/mozilla-central/rev/6e19f844b55e
https://hg.mozilla.org/mozilla-central/rev/5dd1f3da33d9
https://hg.mozilla.org/mozilla-central/rev/0ea746929e72
https://hg.mozilla.org/mozilla-central/rev/9a6f053f56f4
https://hg.mozilla.org/mozilla-central/rev/ee420e34b901
https://hg.mozilla.org/mozilla-central/rev/f1f44cd2bddb
https://hg.mozilla.org/mozilla-central/rev/a20a1d0d0c0e
https://hg.mozilla.org/mozilla-central/rev/b52349c52e06

Comment 93

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/MediaStream now notes Firefox support for active in Fx52.

https://developer.mozilla.org/en-US/docs/Web/API/MediaStream/active updated to note support in Fx 52 and to define “active”.

Updated https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/ended to note this reflects the stream’s active/inactive state.

Updated https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/autoplay to clean up content and to note that autoplay begins once the stream becomes active.

Notes this support in Firefox 52 for developers.