Bug 52697 – Frame accurate seeking isn't always accurate

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Rob Coenen]

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.

[Rob Coenen]

Created attachment 79412 [details]
screenshot of the bug

[Rob Coenen]

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

[Jer Noble]

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.

[Jer Noble]

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.

[Jer Noble]

Created attachment 79485 [details]
Patch for testcase

[Jer Noble]

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.

[Jer Noble]

Created attachment 79497 [details]
Patch

[Rob Coenen]

I have updated the testcase at http://www.massive-interactive.nl/html5_video/smpte_test_universal.html with the patch provided

[Jer Noble]

(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.

[Rob Coenen]

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

[Jer Noble]

(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.

[Jer Noble]

(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"

[Jer Noble]

(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.

[Eric Seidel (no email)]

Comment on attachment 79497 [details]
Patch

Can we include the test case in this patch?

[Eric Carlson]

(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.

[Jer Noble]

(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.

[Andrew Scherkus]

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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 80994 [details]
Patch

[Andrew Scherkus]

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?

[Jer Noble]

(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.

[Andrew Scherkus]

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 :(

[Jer Noble]

Created attachment 81034 [details]
Patch

Fixed the WebKit/Safari Windows version of this bug as well.

[Jer Noble]

(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.

[Jer Noble]

What the..? How did that platform/gtk/Skipped change get in there?  Let me post a new patch.

[Andrew Scherkus]

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!

[Eric Carlson]

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.

[Rob Coenen]

(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...

[Rob Coenen]

(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

[Jer Noble]

Created attachment 81077 [details]
Patch

[Andrew Scherkus]

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 :)

[Jer Noble]

(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 Noble]

(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.

[Rob Coenen]

(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 ....

[Jer Noble]

Created attachment 81127 [details]
Patch

[Rob Coenen]

(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 :-)

[Rob Coenen]

(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

[WebKit Commit Bot]

Comment on attachment 81127 [details]
Patch

Clearing flags on attachment: 81127

Committed r77690: <http://trac.webkit.org/changeset/77690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77690 might have broken Qt Linux Release
The following tests are not passing:
media/video-frame-accurate-seek.html