Closed Bug 1208316 Opened 5 years ago Closed 3 years ago

Implement active/inactive state for MediaStreams

Categories

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

33 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(14 files)

58 bytes, text/x-review-board-request
smaug
: review+
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
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.
No longer depends on: 1223696
Duplicate of this bug: 1212613
Rank: 25
Priority: -- → P2
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
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
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 on attachment 8789804 [details]
Bug 1208316 - Implement MediaStream.active.

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

r+ to the .webidl
Attachment #8789804 - Flags: review?(bugs) → review+
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 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?
Attachment #8789804 - Flags: review?(jib) → review+
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...)
Attachment #8789805 - Flags: review?(jib) → review+
Comment on attachment 8789806 [details]
Bug 1208316 - Rename MediaStreamTrack::NotifyEnded to OverrideEnded.

https://reviewboard.mozilla.org/r/77876/#review76850
Attachment #8789806 - Flags: review?(jib) → review+
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?
Attachment #8789807 - Flags: review?(jib) → review+
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))
Attachment #8789808 - Flags: review?(jib) → review+
Comment on attachment 8789810 [details]
Bug 1208316 - Do not stop the local MediaStream in mediaStreamPlayback.js.

https://reviewboard.mozilla.org/r/77884/#review76860
Attachment #8789810 - Flags: review?(jib) → review+
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?
Attachment #8789811 - Flags: review?(jib) → review+
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.
Attachment #8789812 - Flags: review?(jib) → review+
Comment on attachment 8789813 [details]
Bug 1208316 - Simplify stopTracksForStreamInMediaPlayback.

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

.

    _O/\O_
Attachment #8789813 - Flags: review?(jib) → review+
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 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 on attachment 8790601 [details]
Bug 1208316 - Notify watchers on StreamListener::Forget().

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

lgtm.
Attachment #8790601 - Flags: review?(jib) → review+
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
Depends on: 1295352
Also removing the dependency on bug 1172394 since I'm implementing a workaround for media element mozCaptureStream and seeking.
No longer depends on: 1172394
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.
Attachment #8789809 - Flags: review?(drno) → review+
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.
Attachment #8789808 - Flags: review+ → review?(jib)
Here comes a rebase.
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?
Attachment #8789808 - Flags: review?(jib) → review+
Comment on attachment 8791247 [details]
Bug 1208316 - HTMLMediaElement.ended should follow MediaStream.active.

https://reviewboard.mozilla.org/r/78714/#review81242
Attachment #8791247 - Flags: review?(jib) → review+
Comment on attachment 8791248 [details]
Bug 1208316 - A media element should autoplay a MediaStream that becomes active.

https://reviewboard.mozilla.org/r/78716/#review81244
Attachment #8791248 - Flags: review?(jib) → review+
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");

?
Attachment #8791249 - Flags: review?(jib) → review+
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.
Pushed by pehrsons@gmail.com:
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
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.
You need to log in before you can comment on or make changes to this bug.