1390443 - Rework bug 1322087

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

See https://reviewboard.mozilla.org/r/97546/#review97916 for the detail.

nextFrameStatus changes are dispatched as state-change tasks and playbackEnded is notified as a regular task. It is possible for the main thread to observe nextFrameStatus==NEXT_FRAME_UNAVAILABLE and !mDecoder->IsEnded() and cause 'waiting' to fire incorrectly.

Bug 1322087 P2 introduced a new API, MediaResource::IsExpectingMoreData() to work around this issue. However, it turned out to be a bad idea since for some MediaResource sub-classes like HLSResource, it is unclear how to evaluate IsExpectingMoreData().

The correct fix is, we should dispatch nextFrameStatus change and playbackEnded as a single task when playback reaches the end in MDSM. In order to do that, I will introduce a new variant to NextFrameStatus, which is NEXT_FRAME_UNAVAILABLE_ENDED to notify the next frame is unavailable for playback has reached the end.

This also fixes the spec. issue (https://github.com/whatwg/html/issues/2885) where 'ended' should be fired before 'seeked' when seeking to the end of the resource.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

It turns out running reaches-the-end algorithm (https://html.spec.whatwg.org/multipage/media.html#reaches-the-end) before queuing 'seeked' event will break other tests. For now I will put focus on reworking bug 1322087 P2 and removing MediaResource::IsExpectingMoreData().

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1390443. P1 - don't change nextFrameStatus when MDSM reaches the end of playback.

Instead, MediaDecoder::NextFrameStatus() checks IsEnded() and returns
NEXT_FRAME_UNAVAILABLE to ensure we have HAVE_CURRENT_DATA when playback
is ended on the main thread.

This will fix the timing issue (comment 0) which causes 'waiting' to fire.

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1390443. P2 - remove unused IsExpectingMoreData().

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

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1390443. P3 - MediaSourceDecoder::CanPlayThroughImpl() should return true when its MediaSource is ended.

P1 removes the check for IsExpectingMoreData() and reveals a bug where
readyState doesn't change to HAVE_ENOUGH_DATA when the MediaSource is ended.

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

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

This fixes the bug where mWaitingForKey is reset only when mPaused is false.
We should reset mWaitingForKey to NOT_WAITING_FOR_KEY when the key is
available and decoding can continue.

http://w3c.github.io/encrypted-media/#resume-playback

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

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8898159 [details]
Bug 1390443. P3 - MediaSourceDecoder::CanPlayThroughImpl() should return true when its MediaSource is ended.

https://reviewboard.mozilla.org/r/169522/#review174792

::: commit-message-760e8:4
(Diff revision 1)
> +Bug 1390443. P3 - MediaSourceDecoder::CanPlayThroughImpl() should return true when its MediaSource is ended.
> +
> +P1 removes the check for IsExpectingMoreData() and reveals a bug where
> +readyState doesn't change to HAVE_ENOUGH_DATA when the MediaSource is ended.

that is not correct.

The last line:
return buffered.ContainsWithStrictEnd(ClampIntervalToEnd(interval));

ClampIntervalToEnd, clamp the available data if we are in ended mode and CanPlayThrough will then return true.

So we check that we have continuous data between currentTime and the end.

The MediaSource could be ended, yet have a too large gap in the data, in which case we can't play through.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898157 [details]
Bug 1390443. P1 - don't change nextFrameStatus when MDSM reaches the end of playback.

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

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898158 [details]
Bug 1390443. P2 - remove unused IsExpectingMoreData().

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

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

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

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

We don't need P3 since bug 1389844 fixed the issue.

Comment 11

[Jean-Yves Avenard [:jya]]

The reason  this was there (in P3) was to not fire waitungforkey until all decoded frames were played. It looks like this behaviour is lost now.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175622

::: dom/html/HTMLMediaElement.cpp:5749
(Diff revision 2)
>    }
>  
>    enum NextFrameStatus nextFrameStatus = NextFrameStatus();
> +  if (mWaitingForKey == NOT_WAITING_FOR_KEY) {
> +    if (nextFrameStatus == NEXT_FRAME_UNAVAILABLE &&
> +        mDecoder && !mDecoder->IsEnded()) {

i think NextFrameBufferedStatus should be the one taking care of the decoder being ended or not. the MSE variant already does so.

if this change goes before 1391666, it needs to be uplifted to 56, and use the mochitest in 1391666

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175650

::: dom/html/HTMLMediaElement.cpp:5770
(Diff revision 2)
>        // unavailable.
> -    } else if (mDecoder && !mDecoder->IsEnded()) {
> -      nextFrameStatus = mDecoder->NextFrameBufferedStatus();
> +      mWaitingForKey = WAITING_FOR_KEY_DISPATCHED;
> +      DispatchAsyncEvent(NS_LITERAL_STRING("waitingforkey"));
> +    }
> +  } else { // mWaitingForKey == WAITING_FOR_KEY_DISPATCHED
> +    if (nextFrameStatus == NEXT_FRAME_AVAILABLE) {

You could just assert mWaitingForKey==WAITING_FOR_KEY_DISPATCHED instead of making it a comment.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8898158 [details]
Bug 1390443. P2 - remove unused IsExpectingMoreData().

https://reviewboard.mozilla.org/r/169520/#review175652

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8898157 [details]
Bug 1390443. P1 - don't change nextFrameStatus when MDSM reaches the end of playback.

https://reviewboard.mozilla.org/r/169518/#review175654

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898157 [details]
Bug 1390443. P1 - don't change nextFrameStatus when MDSM reaches the end of playback.

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

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898158 [details]
Bug 1390443. P2 - remove unused IsExpectingMoreData().

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

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

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

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175622

> i think NextFrameBufferedStatus should be the one taking care of the decoder being ended or not. the MSE variant already does so.
> 
> if this change goes before 1391666, it needs to be uplifted to 56, and use the mochitest in 1391666

The change to NextFrameBufferedStatus is out of the scope of this bug.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f8ac9b281da
P1 - don't change nextFrameStatus when MDSM reaches the end of playback. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/05b2bf2e6017
P2 - remove unused IsExpectingMoreData(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5bcf66374df8
P3 - rewrite the logic about mWaitingForKey. r=cpearce

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175758

::: dom/html/HTMLMediaElement.cpp:5848
(Diff revision 3)
>                            "Stream HAVE_ENOUGH_DATA", this));
>      ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA);
>      return;
>    }
>  
>    if (!mPaused || mAutoplaying) {

this should have been removed during your rebase as this is no longer necessary.

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175758

> this should have been removed during your rebase as this is no longer necessary.

Bug 1392184 is opened.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing web-platform-tests, e.g. /encrypted-media/clearkey-mp4-playback-temporary-multikey-sequential-readyState.html:

https://hg.mozilla.org/integration/autoland/rev/cac477424370c01254ada2e415009f1919932d48
https://hg.mozilla.org/integration/autoland/rev/181aba32c3bc20ddb1e73747f992367559d80a1b
https://hg.mozilla.org/integration/autoland/rev/0b3859c872250ef3fb8cccf8536345a4bd16b233

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5bcf66374df892367c7228890bfe68c98e9a3862&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=124479560&repo=autoland

E.g.
[task 2017-08-21T07:01:08.653960Z] 07:01:08     INFO - TEST-START | /encrypted-media/clearkey-mp4-playback-temporary-multikey-sequential-readyState.html
[task 2017-08-21T07:01:08.730605Z] 07:01:08     INFO - PID 3135 | 1503298868721	Marionette	DEBUG	Register listener.js for window 2147483683
[task 2017-08-21T07:02:08.779812Z] 07:02:08     INFO - 
[task 2017-08-21T07:02:08.780306Z] 07:02:08     INFO - TEST-UNEXPECTED-TIMEOUT | /encrypted-media/clearkey-mp4-playback-temporary-multikey-sequential-readyState.html | org.w3.clearkey, successful playback, temporary, mp4, multiple keys, sequential, readyState - Test timed out
[task 2017-08-21T07:02:08.781246Z] 07:02:08     INFO - TEST-UNEXPECTED-TIMEOUT | /encrypted-media/clearkey-mp4-playback-temporary-multikey-sequential-readyState.html | expected OK

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898157 [details]
Bug 1390443. P1 - don't change nextFrameStatus when MDSM reaches the end of playback.

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

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898158 [details]
Bug 1390443. P2 - remove unused IsExpectingMoreData().

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

Comment 27

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

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

Comment 28

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8898160 [details]
Bug 1390443. P3 - rewrite the logic about mWaitingForKey.

https://reviewboard.mozilla.org/r/169524/#review175758

> Bug 1392184 is opened.

It turns out P3 needs to remove this if statement otherwise we will have test failreus in comment 24.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aa4ef11712a
P1 - don't change nextFrameStatus when MDSM reaches the end of playback. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/42fccd5c39ef
P2 - remove unused IsExpectingMoreData(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/a8e94f30387e
P3 - rewrite the logic about mWaitingForKey. r=cpearce

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8aa4ef11712a
https://hg.mozilla.org/mozilla-central/rev/42fccd5c39ef
https://hg.mozilla.org/mozilla-central/rev/a8e94f30387e