Please see this testcase that shows Safari is not always seeking to the correct frame when attempting frame accurate seeking. The testcase is in the bug URL. Chrome had a similar bug and fixed it: http://code.google.com/p/chromium/issues/detail?id=69499. We should fix it too. Using the single step "|> +1 frame" button, each click should advance by one frame, and the time in the top right of the video frame should match the computed time beside it in the surrounding content. (Try frame stepping to 00:00:00:13, the calculated timecode is correctly 00:00:00:13 but the burned-in timecode shows that the frame is stuck at 00:00:00:12 2x then on the netx click bth burned in and calculated timecodes jump to 00:00:00:14. Frame 00:00:00:13 gets never shown) The testcase uses MP4, WebM, and Ogg. I can reproduce the bug with the H264 file. When single stepping, we seem to be 1 or 2 frames off the expected time. One frame of this is caused by bugs in the seeking logic.
Created attachment 79412 [details] screenshot of the bug
added a screenshot with the actual bug: video should be at frame 00:00:00:13 but gets stuck at frame 00:00:00:12
I believe the problem at 00:13 is due to a rounding error: - The sample containing the "00:13" frame begins at 46800/90000 (where 90000 is the media time scale). - This converts to 0.52000000000000002 in float math. - The test case attempts to set the current time to 0.52 - JavaScript converts that to float 0.5199999809265137. - The media engine jumps precisely to 0.5199999809265137. - This is before the beginning of the desired sample, and so sample isn't shown at that time.
Unfortunately, seeking directly to a frame boundary will always be problematic in floating-point math. Chromium solved this problem by effectively shifting the start time of every frame earlier by 1/2 the previous frame's duration. This means Chromium will always be off by 1/2 a frame. You can verify this in the Chromium nightlies, and loading the testcase URL, and setting the video's time to 0.51. It will seek to the frame containing 00:13, even though it's well within the 00:12 frame's duration. I don't recommend we fix this bug in the same way as chromium.
Created attachment 79485 [details] Patch for testcase
The original testcase has a cascading float imprecision bug. I've attached a patch for the testcase that addresses that problem, as well as addressing the original float-impresion problem by adding a small epsilon to the frame calculation.
Created attachment 79497 [details] Patch
I have updated the testcase at http://www.massive-interactive.nl/html5_video/smpte_test_universal.html with the patch provided
(In reply to comment #8) > I have updated the testcase at http://www.massive-interactive.nl/html5_video/smpte_test_universal.html with the patch provided Whoops, looks like that patch I provided now breaks when you skip backwards. I shouldn't have switched the epsilon to negative if you were skipping backwards. Rob, once this WebKit patch lands, the "var epsilon" parameter should be able to be set to 0, and still get frame-accurate seeking within WebKit.
I have build Webkit with the patch and it now correctly displays frame 00:00:00:13. But after some testing I got the timecode at 00:00:20:13 burned in, but calculated = 00:00:20:14 video.currentTime = 20.559600830078125 at this point when set in Chromium video.currentTime to 20.559600830078125 it correctly seeks to 00:00:20:14
(In reply to comment #10) > I have build Webkit with the patch and it now correctly displays frame 00:00:00:13. > But after some testing I got the timecode at 00:00:20:13 burned in, but calculated = 00:00:20:14 > > video.currentTime = 20.559600830078125 at this point Technically, that's correct. 20.5596s works 513.99s at a 25fps rate. That time is within the 513rd frame's duration, so the video should be displaying 00:20:13. You likely have a bug in your secondsToTimecode function. > when set in Chromium video.currentTime to 20.559600830078125 it correctly seeks to 00:00:20:14 That's a bug in Chromium then.
(In reply to comment #11) > Technically, that's correct. 20.5596s works 513.99s at a 25fps rate. Whoops, that should read "513.99 frames"
(In reply to comment #11) > You likely have a bug in your secondsToTimecode function. Indeed, it appears the correct calculation for "frames" in secondsToTimeCode() is the following: var frames = Math.floor((time%1)*fps); Because you're rounding your frames calculation to the nearest frame, you're off by a frame half the time.
Comment on attachment 79497 [details] Patch Can we include the test case in this patch?
(In reply to comment #14) > (From update of attachment 79497 [details]) > Can we include the test case in this patch? I agree that a test case would be useful. I suspect that one of the existing test files has frame times that are suitable.
(In reply to comment #14) > (From update of attachment 79497 [details]) > Can we include the test case in this patch? The test case media is rather large. (3Mb for the .mp4, 5.6Mb each for the .ogv and .webm versions.) However, as Eric suggests, we may have appropriate media already in the LayoutTests.
I guess what I had before matches the correct behvaiour Jer described! The bug in chromium was that seeking to an exact frame position would end up at the previous frame due to a simple < versus <= bug. I'm happy to change my fix to make it consistent instead of the midpoint calculation fix I added last week.
(In reply to comment #15) > I agree that a test case would be useful. I suspect that one of the existing test files has frame times that are suitable. It looks like test.ogv isn't going to be suitable for this task, as the sample durations vary by quite a bit. I'll try to recreate .ogv, .webm, and .mp4 versions with stable frame durations.
Created attachment 80994 [details] Patch
Comment on attachment 80994 [details] Patch Nice test!! Appreciate the ogv version -- we'll get someone to generate the baselines. View in context: https://bugs.webkit.org/attachment.cgi?id=80994&action=review > LayoutTests/media/video-frame-accurate-seek.html:7 > +<video oncanplaythrough='event.target.currentTime=(14.6/25)'></video> Sanity check: is this supposed to be 14/25 as opposed to 14.6?
(In reply to comment #20) > (From update of attachment 80994 [details]) > Nice test!! Appreciate the ogv version -- we'll get someone to generate the baselines. > > View in context: https://bugs.webkit.org/attachment.cgi?id=80994&action=review > > > LayoutTests/media/video-frame-accurate-seek.html:7 > > +<video oncanplaythrough='event.target.currentTime=(14.6/25)'></video> > > Sanity check: is this supposed to be 14/25 as opposed to 14.6? Yes, that was intentional: I added that check for Chromium. :) In the latest nightly, jumping to time 14.6/25 jumps to frame 15. Note that WebKit/Safari Windows will fail this test at first; I've got a similar fix there as well.
Appreciate it :P I checked Chromium 10.0.648.11 dev mac/linux and it's displaying 12/12/14. I think 13/25 is producing 0.51999998092651367188 and we don't properly convert it to 520000 microseconds internally or something. Math is hard :(
Created attachment 81034 [details] Patch Fixed the WebKit/Safari Windows version of this bug as well.
(In reply to comment #22) > Appreciate it :P > > I checked Chromium 10.0.648.11 dev mac/linux and it's displaying 12/12/14. > > I think 13/25 is producing 0.51999998092651367188 and we don't properly convert it to 520000 microseconds internally or something. Math is hard :( Interesting, because now Chromium's behavior is exactly the same as WebKit/QTKit. I'm not entirely sure how Ogg/Theora represents time internally, but here's what was causing the problem in the QuickTime media engine. Maybe it can help you figure out what's going on in Chromium: QuickTime .mov files (as well as MPEG-4 .mp4 files) represent time as a ratio of two integers: the TimeValue / TimeScale. (TimeScale is the number of TimeValues per second.) But WebKit represents time as a floating-point number of seconds. So to convert from a float, to a QuickTime TimeValue, we multiply the float by the TimeScale, and cast the result back to an integer value. In this test-25fps.mp4 file, the TimeScale is 2500, and each frame is 100 TimeValue long. The math worked out like this: (13/25) * 2500 = 1299.999; static_cast<long>(1299.999) = 1299; // static_cast truncates Since frame 13 starts at TimeValue 1300, we display frame 12. The fix was to round to the nearest integer instead of truncating. Practically speaking, TimeScales are never smaller than 100*FPS, so the maximum inaccuracy this rounding adds is +- 0.5% of a frame duration.
What the..? How did that platform/gtk/Skipped change get in there? Let me post a new patch.
Found the culprit, complete with ironic comment (by me nonetheless!!) // Try to preserve as much accuracy as possible. float microseconds = seconds * base::Time::kMicrosecondsPerSecond; base::TimeDelta seek_time = base::TimeDelta::FromMicroseconds(static_cast<int64>(microseconds)); Changing the static_cast<int64> to llroundf() does the trick. Turns out this was in our webkit<->chromium glue code, which is why it was caught by a layout test but not the C++ unit test I wrote. Thanks again for drawing my attention to this!
Comment on attachment 81034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81034&action=review > LayoutTests/media/video-frame-accurate-seek.html:9 > +<script src="media-file.js"></script> > +<script src="video-paint-test.js"></script> > +<body onload="setSrcByTagName('video', findMediaFile('video', 'content/test-25fps')); init()"> > +<p>Test that setting currentTime is frame-accurate. The three videos below should be showing frames 12, 13, and 14.</p> > +<video oncanplaythrough='event.target.currentTime=(12/25)'></video> > +<video oncanplaythrough='event.target.currentTime=(13/25)'></video> > +<video oncanplaythrough='event.target.currentTime=(14.6/25)'></video> > +</div> > +</body> As long as you are going to update the patch, can you clean up this new test by adding html and head elements? Bonus points for formatting it to make it easier to read. > LayoutTests/platform/gtk/Skipped:5618 > + > +# This media test fails; possibly because the AAC codec is not installed on the test bots: > +# https://bugs.webkit.org/show_bug.cgi?id=53125 > +media/audio-mpeg4-supported.html As you note, this doesn't belong in this patch.
(In reply to comment #26) > Found the culprit, complete with ironic comment (by me nonetheless!!) > > // Try to preserve as much accuracy as possible. > float microseconds = seconds * base::Time::kMicrosecondsPerSecond; > base::TimeDelta seek_time = > base::TimeDelta::FromMicroseconds(static_cast<int64>(microseconds)); > > Changing the static_cast<int64> to llroundf() does the trick. > > Turns out this was in our webkit<->chromium glue code, which is why it was caught by a layout test but not the C++ unit test I wrote. Thanks again for drawing my attention to this! I have applied this fix but it's only fixing the 00:00:00:13 issue. video.currentTime = 189.92606401443481 @2FPS should set the video to 00:00:07:15 but the actually displayed frame is 00:00:07:14 still 1 frame off...
(In reply to comment #28) > (In reply to comment #26) > > Found the culprit, complete with ironic comment (by me nonetheless!!) > > > > // Try to preserve as much accuracy as possible. > > float microseconds = seconds * base::Time::kMicrosecondsPerSecond; > > base::TimeDelta seek_time = > > base::TimeDelta::FromMicroseconds(static_cast<int64>(microseconds)); > > > > Changing the static_cast<int64> to llroundf() does the trick. > > > > Turns out this was in our webkit<->chromium glue code, which is why it was caught by a layout test but not the C++ unit test I wrote. Thanks again for drawing my attention to this! > > I have applied this fix but it's only fixing the 00:00:00:13 issue. > > video.currentTime = 189.92606401443481 @25FPS should set the video to 00:00:07:15 but the actually displayed frame is 00:00:07:14 > still 1 frame off... ^^ should be 25 FPS BTW testing with fractions such as video.currentTime = 188/25, video.currentTime = 189/25, video.currentTime = 190/25 is not the correct way to test, a this will always produce exact frame times. When using a slider on a video player you'll always end up with complete other fractions such as the one I found (7.597042560577393) falls in between 189/25 and 190/25
Created attachment 81077 [details] Patch
I tested with: 189.92606401443481 / 25 = 7.597042561 ...and internally that timestamp is represented as 7597043 microseconds. The first frame that matches the seek condition is 7560000 (7.56s = 07:14 @ 25fps) since each frame duration is 40000 that means that the next frame is 7600000 and is actually past the requested seek point. Again not an expert here -- just trying to make numbers work and tests pass :)
(In reply to comment #28) > video.currentTime = 189.92606401443481 @2FPS should set the video to 00:00:07:15 but the actually displayed frame is 00:00:07:14 > still 1 frame off… Rob, your secondsToTimecode() function is still producing incorrect values. I think you'll see that if you uncomment the "apple's approach" calculations, you will start producing correct timecode strings.
(In reply to comment #29) > BTW testing with fractions such as > > video.currentTime = 188/25, video.currentTime = 189/25, video.currentTime = 190/25 is not the correct way to test, a this will always produce exact frame times. > When using a slider on a video player you'll always end up with complete other fractions such as the one I found (7.597042560577393) falls in between 189/25 and 190/25 That's actually incorrect. The original bug was being caused because such fractional numbers as 13/25 were /not/ producing exact frame times.
(In reply to comment #32) > (In reply to comment #28) > > video.currentTime = 189.92606401443481 @2FPS should set the video to 00:00:07:15 but the actually displayed frame is 00:00:07:14 > > still 1 frame off… > > Rob, your secondsToTimecode() function is still producing incorrect values. I think you'll see that if you uncomment the "apple's approach" calculations, you will start producing correct timecode strings. Jer- I did that, the reason I commented it out again is that the problem with that approach is that when I click "+1 frame" (starting at 00:00:00:00) the video goes to 00:00:00:01 but the calculated timecode remains 00:00:00:00 then at the 2nd click video is at 00:00:00:01, calculated at 00:00:00:02, the 3th gives 00:00:00:03 vs 00:00:00:02, the 4th gives 00:00:00:04 vs 00:00:00:03 and then the 5th gives 00:00:00:05 vs 00:00:00:05 ....
Created attachment 81127 [details] Patch
(In reply to comment #35) > Created an attachment (id=81127) [details] > Patch cool, I have updated the javascript code on http://www.massive-interactive.nl/html5_video/smpte_test_universal.html and added new .mp4, .ogv and .webm test files. it now works perfectly on WebKit using the patches added to this ticket. (only tested Mac) I'm finally unable to break it by puncing in weird fractions nor by scrubbing around like a maniac :-)
(In reply to comment #36) > (In reply to comment #35) > > Created an attachment (id=81127) [details] [details] > > Patch > > cool, I have updated the javascript code on http://www.massive-interactive.nl/html5_video/smpte_test_universal.html and added new .mp4, .ogv and .webm test files. > > it now works perfectly on WebKit using the patches added to this ticket. (only tested Mac) I'm finally unable to break it by puncing in weird fractions nor by scrubbing around like a maniac :-) and just for the record: it now seems to break Chromium
Comment on attachment 81127 [details] Patch Clearing flags on attachment: 81127 Committed r77690: <http://trac.webkit.org/changeset/77690>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/77690 might have broken Qt Linux Release The following tests are not passing: media/video-frame-accurate-seek.html