Last Comment Bug 626273 - Frame accurate seeking isn't always accurate
: Frame accurate seeking isn't always accurate
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
http://www.massive-interactive.nl/htm...
: 607873 (view as bug list)
Depends on:
Blocks: 607873
  Show dependency treegraph
 
Reported: 2011-01-16 14:53 PST by Matthew Gregan [:kinetik]
Modified: 2011-01-23 20:13 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch v0 (53.81 KB, patch)
2011-01-16 19:09 PST, Matthew Gregan [:kinetik]
cpearce: review+
roc: approval2.0+
Details | Diff | Review

Description Matthew Gregan [:kinetik] 2011-01-16 14:53:14 PST
Bas pointed me at a testcase that shows we're 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.

The testcase uses MP4, WebM, and Ogg.  I can reproduce the bug with both the WebM and Ogg 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.  The other frame of lag is caused by the video not being invalidated in some circumstances.

So, there is a small hive of bugs causing this:

- Using float instead of double in the IDL (and everywhere that uses this) results in currentTime being rounded off incorrectly.  This results in the computed seek target being 1ms too low.

- Once the above is fixed, there's a fencepost error in seek logic causes us to pick too early a frame when seeking to a time that is exactly the end of one frame and the start of another.

- The video element is not invalidating correctly after changing frames in some cases.  This causes the "second frame lag" issue.  This is caused by calling UpdatePlaybackPosition before seeking (to effect the currentTime update for the seeking event), but not calling it again (or doing anything else to invalidate the frame) after Reader::Seek and RenderVideoFrame have run to set the new frame.

Note that the testcase is attempting to compute the "seek error" immediately after setting currentTime.  As I understand it, this is incorrect.  The spec  states that the "new current time" set asynchronously just before "seeking" is fired.  This behaviour does result in currentTime temporarily returning the "old" current time until "seeking" is fired, which may seem strange to users.

The other thing this testcase reveals is that the WebM seeking code should make use of the Ogg seeking code's CanDecodeToTarget optimization to avoid decoding lots of extra frames when stepping forward a small number of frames.

I have a patch for this which I'll attach shortly.
Comment 1 Matthew Gregan [:kinetik] 2011-01-16 19:09:00 PST
Created attachment 504344 [details] [diff] [review]
patch v0
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-16 19:34:01 PST
Wouldn't block on this, but I'll approve a patch.
Comment 3 Chris Pearce (:cpearce) 2011-01-16 19:46:57 PST
Comment on attachment 504344 [details] [diff] [review]
patch v0

>@@ -198,33 +198,33 @@ class AudioWriteEvent : public nsRunnabl
> class AudioSetVolumeEvent : public nsRunnable
> {
>  public:
>-  AudioSetVolumeEvent(AudioChild* aChild, float volume)
>+  AudioSetVolumeEvent(AudioChild* aChild, double volume)

Can you change volume to aVolume while we're here to match the convention used in the rest of the code?

Same thing in declarations of:

nsBuiltinDecoder::Seek(time) 
nsBuiltinDecoder::SetVolume(volume)
nsMediaDecoder::Seek(time)
nsMediaDecoder::SetVolume(time)
Comment 4 Matthew Gregan [:kinetik] 2011-01-16 19:52:15 PST
Comment on attachment 504344 [details] [diff] [review]
patch v0

Sure, will do that when I check in.
Comment 5 Matthew Gregan [:kinetik] 2011-01-17 17:31:43 PST
http://hg.mozilla.org/mozilla-central/rev/86c446a17a79
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-01-22 10:45:01 PST
Verified using Minefield nighty from 2011-01-22.
Comment 7 Chris Pearce (:cpearce) 2011-01-23 20:13:04 PST
*** Bug 607873 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.