1403478 - Check media element's seekable is correct before and after calling ms.endOfStream() for "test_SeekableAfterEndOfStream*.html"

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

After bug1362165, the "loadedmetadata" of media element would be dispatched before "updateend" of source buffer.

These two tests were used to check v.seekable after called ms.endOfStream(), but now the checking would be done before calling ms.endOfStream().

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

Modify tests to make the v.seekable could always be checked after called
ms.endOfStream().

Also, in test_SeekableAfterEndOfStreamSplit_mp4, after called ms.endOfStream(),
the duration would be truncated to the intersaction of source buffers, and it
would be [(0.095000, 0.896666)]. The old target is wrong, so I change it to 0.5.

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

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

https://reviewboard.mozilla.org/r/183934/#review189112

::: commit-message-cda0d:1
(Diff revision 1)
> +Bug 1403478 - should check v.seekable after called ms.endOfStream().

Should simply state that we now test what the name of the test states it does (it wasn't doing that before)

"should check v.seekable after called ms.endOfStream()" should be:
"Should check v.seekable after calling ms.endOfStream()"

however I would change that into:
"Should check v.seekable after calling ms.endOfStream() as test name suggests"

::: commit-message-cda0d:3
(Diff revision 1)
> +Bug 1403478 - should check v.seekable after called ms.endOfStream().
> +
> +Modify tests to make the v.seekable could always be checked after called

I don't understand that sentence.

::: commit-message-cda0d:8
(Diff revision 1)
> +Modify tests to make the v.seekable could always be checked after called
> +ms.endOfStream().
> +
> +Also, in test_SeekableAfterEndOfStreamSplit_mp4, after called ms.endOfStream(),
> +the duration would be truncated to the intersaction of source buffers, and it
> +would be [(0.095000, 0.896666)]. The old target is wrong, so I change it to 0.5.

the old target isn't wrong.

what you're now appending is... see below.

::: dom/media/mediasource/test/test_SeekableAfterEndOfStreamSplit_mp4.html:28
(Diff revision 1)
> -      var updateCount = 0;
> -      sb.addEventListener("updateend", function () {
> -        updateCount++;
> -        if (updateCount == 1) {
> +      await once(v, "loadedmetadata");
> +      await once(sb, "updateend");
> +
> +      info("- append second buffer -");
> -          // 25819 is the offset of the first media segment's end
> +      // 25819 is the offset of the first media segment's end
> -          sb.appendBuffer(new Uint8Array(arrayBuffer, 25819));
> +      sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 25819));

this is wrong.

you're appending the init segment twice.

this test is supposed to append the data in two blobs.

one from 0 to 25819, and one from 25819 to the end.

This is what you now have to change the target. The original target of 1.3s was correct as bipbop2s.mp4 as its name indicate is around 2s long

Comment 3

[Alastor Wu [:alwu]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

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

Comment 4

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=213fd04458128028f9b657921876d085f65b9c96

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

https://reviewboard.mozilla.org/r/183934/#review189166

::: dom/media/mediasource/test/test_SeekableAfterEndOfStreamSplit.html:24
(Diff revisions 1 - 2)
>      fetchWithXHR("seek.webm", async function (arrayBuffer) {
>        info("- append first buffer -");
>        // 25523 is the offset of the first media segment's end
>        sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 25523));
>        await once(v, "loadedmetadata");
>        await once(sb, "updateend");

I wonder if this could be subject to intermittent timeout.

loadedmetadata and updateend will be fired at the roughly the same time. and does this allow to set an event listener between the time the event is fired and when the event is received.

JW should know more, please consult with him

::: dom/media/mediasource/test/test_SeekableAfterEndOfStream.html:23
(Diff revision 2)
>  
> -    fetchWithXHR("seek.webm", function (arrayBuffer) {
> +    fetchWithXHR("seek.webm", async function (arrayBuffer) {
> +      info("- append buffer -");
>        sb.appendBuffer(new Uint8Array(arrayBuffer));
> -      var promises = [];
> -      promises.push(once(sb, "updateend"));
> +      await once(v, "loadedmetadata");
> +      await once(sb, "updateend");

same question as above...

the previous test would ensure that both events were fired.

I'm concerned that the new code is racy...

If you're certain it's not, please ignore of course and proceed

::: dom/media/mediasource/test/test_SeekableAfterEndOfStream_mp4.html:22
(Diff revision 2)
>      var sb = ms.addSourceBuffer("video/mp4");
>  
> -    fetchWithXHR("bipbop/bipbop2s.mp4", function (arrayBuffer) {
> +    fetchWithXHR("bipbop/bipbop2s.mp4", async function (arrayBuffer) {
> +      info("- append buffer -");
>        sb.appendBuffer(new Uint8Array(arrayBuffer));
> -      var promises = [];
> +      await once(v, "loadedmetadata");

same here

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

https://reviewboard.mozilla.org/r/183934/#review189168

::: dom/media/mediasource/test/test_SeekableAfterEndOfStream.html:28
(Diff revision 2)
> -      promises.push(once(sb, "updateend"));
> -      promises.push(once(v, "loadedmetadata"));
> -      Promise.all(promises).then(function () {
> +      await once(sb, "updateend");
> +
> +      info("- check seekable -");
> +      ok(v.seekable.length, "Resource is seekable");
> +      is(v.seekable.start(0), 0, "Seekable's start point is correct");
> +      is(v.seekable.end(0), ms.duration, "Seekable's end point is correct");

technically, duration should have changed to v.buffered.end(0)

Comment 7

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> I wonder if this could be subject to intermittent timeout.
> 
> loadedmetadata and updateend will be fired at the roughly the same time. and
> does this allow to set an event listener between the time the event is fired
> and when the event is received.
> 
> JW should know more, please consult with him

If I understand correctly, the "updateend" would be sent after MediaElement's ready stated changed. 

It's done by pending the completion promise until calling [1], and we would reach there only when we've changed the ready state to HAVE_METADATA via HTMLMediaElement::MetadataLoaded(). Therefore, before resolving the promise, we've dispatched the async "loadedmetadata" event. 

From above, the "loadedmetadata" would always be dispatched before "updateend".

Please correct me if I'm wrong.

Thanks!

[1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/html/HTMLMediaElement.cpp#5716 

---

In addition, as your suggestion, I would also ask JW for an extra review.

Comment 8

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> technically, duration should have changed to v.buffered.end(0)

It said "Return a single range with a start time of 0 and an end time equal to duration." [1]

Before calling endOfStream(), the value of duration would be the one from meta data, instead of buffered range.

[1] https://w3c.github.io/media-source/index.html#htmlmediaelement-extensions

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

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

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1403478 - part2 : merge 'test_SeekableBefore*' and 'test_SeekableAfter*' into 'test_SeekableBeforeAndAfter*'.

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

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

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

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8913103 [details]
Bug 1403478 - part2 : merge 'test_SeekableBefore*' and 'test_SeekableAfter*' into 'test_SeekableBeforeAndAfter*'.

https://reviewboard.mozilla.org/r/184524/#review189798

::: commit-message-ae2c5:1
(Diff revision 1)
> +Bug 1403478 - part2 : merge 'test_SeekableBefore*' and 'test_SeekableAfter*' to 'test_SeekableBeforeAndAfter*'.

s/to/into

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #8)
> (In reply to Jean-Yves Avenard [:jya] from comment #6)
> > technically, duration should have changed to v.buffered.end(0)
> 
> It said "Return a single range with a start time of 0 and an end time equal
> to duration." [1]

I'm not referring to the value of the seekable attribute but the value of duration itself.

after a call to endOfStream() duration = buffered.end(0)

https://w3c.github.io/media-source/#end-of-stream-algorithm

step 3:
"Run the duration change algorithm with new duration set to the largest track buffer ranges end time across all the track buffers across all SourceBuffer objects in sourceBuffers. "

so doing:
is(v.seekable.end(0), v.buffered.end(0))
is identical to doing:
is(v.seekable.end(0), v.duration)

as v.duration == v.buffered.end(0)

Comment 14

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

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

https://reviewboard.mozilla.org/r/183934/#review190002

::: commit-message-cda0d:6
(Diff revision 4)
> +Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().
> +
> +This patch does two things,
> +
> +(1) check v.seekable after calling ms.endOfStream()
> +As test name suggests, we check seekable after finishing endOfStream()

after 'calling' endOfStream().

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8912612 [details]
Bug 1403478 - part1 : check v.seekable before and after calling ms.endOfStream().

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

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8913103 [details]
Bug 1403478 - part2 : merge 'test_SeekableBefore*' and 'test_SeekableAfter*' into 'test_SeekableBeforeAndAfter*'.

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

Comment 17

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f6fe7eba1cc79ddd6fd5ee7ddb39ba32e8f76b0

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2b6049d51c7
part1 : check v.seekable before and after calling ms.endOfStream(). r=jwwang,jya
https://hg.mozilla.org/integration/autoland/rev/78024b5b78dd
part2 : merge 'test_SeekableBefore*' and 'test_SeekableAfter*' into 'test_SeekableBeforeAndAfter*'. r=jya

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/c2b6049d51c7
https://hg.mozilla.org/mozilla-central/rev/78024b5b78dd