1362165 - [MSE] readyState may be HAVE_NOTHING after init segment appended

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

When appending an init segment, we may fire "updateend" though readyState is still HAVE_NOTHING

Per spec, readyState must be moved to HAVE_METADATA:
https://w3c.github.io/media-source/#sourcebuffer-init-segment-received
step 6:
"6. If the HTMLMediaElement.readyState attribute is HAVE_NOTHING, then run the following steps:
"

And only after returning from the "init segment received" algorithm should updateend event be fired.

This is the cause of some mochitest intermittent failure (such as test_TruncatedDuration.html) and likely why we have some reports about streams not starting.

The reason for this is that many live streams start by seeking to a particular position right after appending some data.

However, if readyState is HAVE_NOTHING, then the seek process is to be aborted:
https://dev.w3.org/html5/spec-preview/media-elements.html#dom-media-seek
"If the media element's readyState is HAVE_NOTHING, abort these steps."

Comment 1

[Jean-Yves Avenard [:jya]]

Bug 879717 added a change that force the readyState to HAVE_METADATA if nothing has been painted yet.

This is against the MSE spec that only defines the state based on the buffered range, regardless of what got painted.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P1. Fix potential race accessing members.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P2. Don't expect that all data has been processed after metadata.

Assuming that the buffered range has been updated when loadedmetadata was fired was wrong. Only once the data has been fully appended to the source buffer, can seeking complete.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P3. Remove unecessary code.

We have no need to call endOfStream here as we don't rely on the ended event being fired.
Also, there's no need to track how many update count we will get when we only use appendBuffer once.

Adding extra test to help identify where the actual failure of the test could occur.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P4. Don't assume the duration found when reading metadata will be the final one.

When endOfStream() is called, the duration is changed to that of the last buffered frame's end time.

The ended event will only be fired if currentTime is equal to the duration. So if the duration has changed following a call to endOfStream, then playback won't be considered as ended.

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

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P5. Don't assume that when stalling all data has been loaded.

The stalled event can be fired as playback is starting and data has yet to be processed.
So instead we wait for playback to reach the end.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P6. Only use sourcebuffer again once previous operation completed.

It was incorrect to assume that once "loadeddata" event fired the sourcebuffer would be usable again. Only the "updateend" event guarantees such condition.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P7. Only complete appendBuffer once readyState has changed.

MSE specs require that the readyState be modified during either the Initialization Segment Received or the Coded Frame Processing algorithms.

At this stage, we only handle the Initialization Segment part (readyState moving from HAVE_NOTHING to HAVE_METADATA)

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

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P8. Fix mochitests.

Per spec loadedmetadata should be fired before updateend. fetchAndLoad want on the updateend event. So we can't wait for the loadedmetadata event only after updateend has been fired.

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

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P9. Update mochitests.

I'm not sure what this mochitest was supposed to test, but it was doing it wrong anyway.

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

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: Add web platform test.

readyState is supposed to be updated during MSE's Initialization Segment Received and Coded Frame Processing algorithm.
Only then would the appendBuffer operation terminates by queueing the update/updateend events.

These tests ensure that loadedmetadata and loadeddata are fired first.

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

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: Correct test.

the durationchange event will be queued during the initialization segment received algorithm.
Only once the init segment has been fully processed will update/updateend be queued.

As such, the updateend event must be fired before being able to  call appendBuffer once again.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1362165: P12. Updated wpt expectation.

New test will error until bug 1367993 is fixed.

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

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871942 [details]
Bug 1362165: P1. Fix potential race accessing members.

https://reviewboard.mozilla.org/r/143454/#review147292

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871943 [details]
Bug 1362165: P2. Don't expect that all data has been processed after metadata.

https://reviewboard.mozilla.org/r/143456/#review147294

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871944 [details]
Bug 1362165: P3. Remove unecessary code.

https://reviewboard.mozilla.org/r/143458/#review147296

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871945 [details]
Bug 1362165: P4. Don't assume the duration found when reading metadata will be the final one.

https://reviewboard.mozilla.org/r/143460/#review147298

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871946 [details]
Bug 1362165: P5. Don't assume that when stalling all data has been loaded.

https://reviewboard.mozilla.org/r/143462/#review147300

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871947 [details]
Bug 1362165: P6. Only use sourcebuffer again once previous operation completed.

https://reviewboard.mozilla.org/r/143464/#review147302

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871953 [details]
Bug 1362165: P12. Updated wpt expectation.

https://reviewboard.mozilla.org/r/143476/#review147304

Comment 21

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

Comment on attachment 8871948 [details]
Bug 1362165: P7. Only complete appendBuffer once readyState has changed.

https://reviewboard.mozilla.org/r/143466/#review147372

::: dom/media/mediasource/SourceBuffer.h:169
(Diff revision 1)
>                                                    uint32_t aLength,
>                                                    ErrorResult& aRv);
>  
>    void AppendDataCompletedWithSuccess(const SourceBufferTask::AppendBufferResult& aResult);
>    void AppendDataErrored(const MediaResult& aError);
> +  void ResumeAppendDataCompleted();

This function is used nowhere in this commit.

Comment 22

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

Comment on attachment 8871949 [details]
Bug 1362165: P8. Fix mochitests.

https://reviewboard.mozilla.org/r/143468/#review147374

::: dom/media/mediasource/test/test_DurationChange.html:22
(Diff revision 1)
>      var sb = ms.addSourceBuffer("video/webm");
>  
>      fetchWithXHR("seek.webm", function (arrayBuffer) {
>        sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 318));
> -      once(v, "loadedmetadata")
> +      var promises = [];
> +      promises.push(once(sb, "updateend"));

Per spec, we expect 'updateend' to come after 'loadedmetadata', right?

Comment 23

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

Comment on attachment 8871950 [details]
Bug 1362165: P9. Update mochitests.

https://reviewboard.mozilla.org/r/143470/#review147376

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871949 [details]
Bug 1362165: P8. Fix mochitests.

https://reviewboard.mozilla.org/r/143468/#review147374

> Per spec, we expect 'updateend' to come after 'loadedmetadata', right?

yes...

however the order set in promises do no matter.

but I'll change the order to make it clearer

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871942 [details]
Bug 1362165: P1. Fix potential race accessing members.

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

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871943 [details]
Bug 1362165: P2. Don't expect that all data has been processed after metadata.

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

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871944 [details]
Bug 1362165: P3. Remove unecessary code.

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

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871945 [details]
Bug 1362165: P4. Don't assume the duration found when reading metadata will be the final one.

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871946 [details]
Bug 1362165: P5. Don't assume that when stalling all data has been loaded.

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

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871947 [details]
Bug 1362165: P6. Only use sourcebuffer again once previous operation completed.

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

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871948 [details]
Bug 1362165: P7. Only complete appendBuffer once readyState has changed.

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

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871949 [details]
Bug 1362165: P8. Fix mochitests.

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

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871950 [details]
Bug 1362165: P9. Update mochitests.

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

Comment 34

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871951 [details]
Bug 1362165: Add web platform test.

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

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871952 [details]
Bug 1362165: Correct test.

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

Comment 36

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871953 [details]
Bug 1362165: P12. Updated wpt expectation.

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

Comment 37

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871951 [details]
Bug 1362165: Add web platform test.

https://reviewboard.mozilla.org/r/143472/#review147748

Comment 38

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8871952 [details]
Bug 1362165: Correct test.

https://reviewboard.mozilla.org/r/143474/#review147750

r+ with nit:

::: commit-message-51d93:6
(Diff revision 2)
> +Bug 1362165: Correct test. r?jgraham
> +
> +the durationchange event will be queued during the initialization segment received algorithm.
> +Only once the init segment has been fully processed will update/updateend be queued.
> +
> +As such, the updateend event must be fired before being able to  call appendBuffer once again.

Remove 2nd space in "to  call".

Comment 39

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871951 [details]
Bug 1362165: Add web platform test.

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

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871952 [details]
Bug 1362165: Correct test.

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

Comment 41

[Jean-Yves Avenard [:jya]]

Comment on attachment 8871953 [details]
Bug 1362165: P12. Updated wpt expectation.

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

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20e01cb7989d
P1. Fix potential race accessing members. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c89c6c8630f1
P2. Don't expect that all data has been processed after metadata. r=gerald
https://hg.mozilla.org/integration/autoland/rev/bc1e56d5cb9b
P3. Remove unecessary code. r=gerald
https://hg.mozilla.org/integration/autoland/rev/5334cc6d0ea9
P4. Don't assume the duration found when reading metadata will be the final one. r=gerald
https://hg.mozilla.org/integration/autoland/rev/bb63055aef6c
P5. Don't assume that when stalling all data has been loaded. r=gerald
https://hg.mozilla.org/integration/autoland/rev/55f7558b7c52
P6. Only use sourcebuffer again once previous operation completed. r=gerald
https://hg.mozilla.org/integration/autoland/rev/846a4761d0cd
P7. Only complete appendBuffer once readyState has changed. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/be4b872ba909
P8. Fix mochitests. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2ba56b296e40
P9. Update mochitests. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3329385fcf2d
Add web platform test. r=gerald
https://hg.mozilla.org/integration/autoland/rev/138425350cc6
Correct test. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1e51fbdefff8
P12. Updated wpt expectation. r=gerald

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20e01cb7989d
https://hg.mozilla.org/mozilla-central/rev/c89c6c8630f1
https://hg.mozilla.org/mozilla-central/rev/bc1e56d5cb9b
https://hg.mozilla.org/mozilla-central/rev/5334cc6d0ea9
https://hg.mozilla.org/mozilla-central/rev/bb63055aef6c
https://hg.mozilla.org/mozilla-central/rev/55f7558b7c52
https://hg.mozilla.org/mozilla-central/rev/846a4761d0cd
https://hg.mozilla.org/mozilla-central/rev/be4b872ba909
https://hg.mozilla.org/mozilla-central/rev/2ba56b296e40
https://hg.mozilla.org/mozilla-central/rev/3329385fcf2d
https://hg.mozilla.org/mozilla-central/rev/138425350cc6
https://hg.mozilla.org/mozilla-central/rev/1e51fbdefff8