Closed Bug 1403478 Opened 7 years ago Closed 7 years ago

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

Categories

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

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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().
Summary: Correct the wrong test, "test_SeekableAfterEndOfStreamSplit.html" and "test_SeekableAfterEndOfStreamSplit_mp4.htm" → Correct the wrong tests, "test_SeekableAfterEndOfStreamSplit.html" and "test_SeekableAfterEndOfStreamSplit_mp4.html"
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
Attachment #8912612 - Flags: review?(jyavenard) → review-
Summary: Correct the wrong tests, "test_SeekableAfterEndOfStreamSplit.html" and "test_SeekableAfterEndOfStreamSplit_mp4.html" → Check media element's seekable is correct before and after calling ms.endOfStream() for "test_SeekableAfterEndOfStream*.html"
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
Attachment #8912612 - Flags: review?(jyavenard) → review+
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)
Priority: -- → P3
(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.
(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 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
Attachment #8913103 - Flags: review?(jyavenard) → review+
(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 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().
Attachment #8912612 - Flags: review?(jwwang) → review+
Pushed by alwu@mozilla.com: 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: