Seek a HTMLMediaElement to it's duration property should also finish the playback and then trigger the "ended" event. However, this is not the case in some video files. Please see the test patch.

The problem is that there is a numerical issue while transforming a double to a int64. The duration of the test case is 4.137471 which is stored as a double. When we call seek(4.137471), the 4.137471 is transformed to an int64 and it becomes 4137470. (The operation is here: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#809). I will use the utilities functions of TimeUnits to solve it.

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

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

(In reply to Tzuhao Kuo [:kaku] from comment #0) > Created attachment 8711570 [details] [diff] [review] > test-patch.diff > > Seek a HTMLMediaElement to it's duration property should also finish the > playback and then trigger the "ended" event. However, this is not the case > in some video files. Please see the test patch. Can't find the test in this attachment. Can you make it a mochitest?

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang https://reviewboard.mozilla.org/r/32265/#review29117 ::: dom/media/test/test_bug1242338.html:42 (Diff revision 1) > + video.src = "bug482461-theora.ogv"; Use MediaTestManager to test multiple files to increase test coverage.

Comment on attachment 8711625 [details] MozReview Request: Bug1242338 - fix numerical issue in MediaDecoder::Seek(); r=jwwang https://reviewboard.mozilla.org/r/32263/#review29119

https://reviewboard.mozilla.org/r/32265/#review29117 > Use MediaTestManager to test multiple files to increase test coverage. Okay.

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32265/diff/1-2/

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32265/diff/2-3/

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang https://reviewboard.mozilla.org/r/32265/#review29123 ::: dom/media/test/test_bug1242338.html:14 (Diff revision 3) > +SimpleTest.waitForExplicitFinish(); manager.runTests() will do this for you. ::: dom/media/test/test_bug1242338.html:57 (Diff revision 3) > + is(test.name, video.name, test.name + ": Name should match in onLoadedmetadata()"); These lines copied from test_playback.html should be removed since they are not relavant to the purose of this test. ::: dom/media/test/test_bug1242338.html:74 (Diff revision 3) > + if (video.seenSeeking && video.seenSeeked) { I think this test can simply check if 'ended' event is fired without concerning 'seeking' and 'seeked'.

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32265/diff/3-4/

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang https://reviewboard.mozilla.org/r/32265/#review29129 ::: dom/media/test/test_bug1242338.html:19 (Diff revision 4) > + video.prevTime = 0; prevTime not used at all. ::: dom/media/test/test_bug1242338.html:40 (Diff revision 4) > + checkMetadata(test.name, video, test); checkMetadata() is not relavant to this test.

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32265/diff/4-5/

Comment on attachment 8711625 [details] MozReview Request: Bug1242338 - fix numerical issue in MediaDecoder::Seek(); r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32263/diff/1-2/

Comment on attachment 8711626 [details] MozReview Request: Bug1242338 - test cases; r=jwwang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32265/diff/5-6/

Thanks for the review. Try looks ok, https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4c803b677a. Add checkin-needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69bb4ab3afc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/ce532d96a0e2

https://hg.mozilla.org/mozilla-central/rev/69bb4ab3afc5 https://hg.mozilla.org/mozilla-central/rev/ce532d96a0e2

Quick fly-by note. The previous code was testing for overflow; this is what: nsresult rv = SecondsToUsecs(aTime, timeUsecs); NS_ENSURE_SUCCESS(rv, rv); was doing. Doing: timeUsecs = TimeUnit::FromSeconds(aTime).ToMicroseconds() is *not* equivalent ; this will assert should an overflow internally occurs in TimeUnit::FromSeconds() The replacement code should have been something like: TimeUnit time = TimeUnit::FromSeconds(aTime); if (!time.IsValid()) { return NS_ERROR_FAILURE; } timeUsecs = time.ToMicroseconds();

https://hg.mozilla.org/mozilla-central/file/584870f1cbc5d060a57e147ce249f736956e2b62/dom/media/TimeUnits.h#l58 It is rounded down to INT64_MAX so time.IsValid() is always true. Maybe it should call Invalid() for the case.