626273 - Frame accurate seeking isn't always accurate

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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

Attachment: patch v0

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't block on this, but I'll approve a patch.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment on attachment 504344 [details] [diff] [review]
patch v0

Sure, will do that when I check in.

Comment 5

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/86c446a17a79

Comment 6

[u279076]

Verified using Minefield nighty from 2011-01-22.