Closed Bug 626273 Opened 13 years ago Closed 13 years ago

Frame accurate seeking isn't always accurate


(Core :: Audio/Video, defect)

Not set



Tracking Status
blocking2.0 --- -


(Reporter: kinetik, Assigned: kinetik)





(1 file)

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:  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.
blocking2.0: --- → ?
Attached patch patch v0Splinter Review
Assignee: nobody → kinetik
Attachment #504344 - Flags: review?(chris)
Wouldn't block on this, but I'll approve a patch.
blocking2.0: ? → -
Blocks: 607873
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:

Attachment #504344 - Flags: review?(chris) → review+
Comment on attachment 504344 [details] [diff] [review]
patch v0

Sure, will do that when I check in.
Attachment #504344 - Flags: approval2.0?
Closed: 13 years ago
Resolution: --- → FIXED
Verified using Minefield nighty from 2011-01-22.
You need to log in before you can comment on or make changes to this bug.