493958 - a/v sync problems when seeking

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Christopher Blizzard (:blizzard)]

On trunk when seeking in a big file I see A/V sync problems when the video plays after the seek.  It's something persistent, too, doesn't fix itself when you pause and restart.

Use the URL above and seek around a bit - 20 minutes in, 30 minutes in, jump back and forth and it will eventually get out of sync.

This is on OSX, Minefield source build from this morning.

Comment 1

[cajbir (:cajbir)]

I think this may have something to do with bug 463358. The audio time isn't matching up to the seeked frame times.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree with Blizzard that this blocks. Seeking in a video is a common action. We should support it in ways that don't affect sync. I know bug 463358 hadn't blocked, but here we are.

Comment 3

[Christopher Blizzard (:blizzard)]

fwiw after seeing how much of a huge difference using keyframes makes in the seeking experience I think we should block on bug 463358 as well.  It's like night and day - no foolin'.

Comment 4

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

Attachment: Patch v1

* If Ogg gives us extra audio data in the frames we skip after the seek (to account for latency...), include the same amount in the first frame after seek.

Comment 5

[cajbir (:cajbir)]

Comment on attachment 378787 [details] [diff] [review]
Patch v1

>+          if (frame)
>+            delete frame;

Don't need null check for delete.

>           frame = NextFrame();
>+          audioData.AppendElements(frame->mAudioData);
>+          audioTime += frame->mAudioData.Length() /
>+                       (float)mAudioRate / (float)mAudioChannels;

frame can be NULL after the NextFrame call.

>+            size_t numExtraBytes = mAudioChannels * mAudioRate * audioTime;
>+            // numExtraBytes must be evenly divisble by number of channels.
>+            while (numExtraBytes % mAudioChannels)
>+              ++numExtraBytes;

numExtraBytes should be numExtraSamples? They're floats, not bytes.

Comment 6

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

     size_t count = mAudioChannels * mAudioRate * mCallbackPeriod;
+    // count must be evenly divisble by number of channels.
+    while (count % mAudioChannels)
+      ++count;

Eeek!

Please, count = mAudioChannels * PRInt32(NS_ceil(mAudioRate*mCallbackPeriod))

+            // liboggplay gave us more data than expected, we need to prepend
+            // extra data to current frame keep audio in sync.

This comment is a bit confusing. How about
  // Some of the audio data from previous frames actually belongs to this frame
  // and later frames. So rescue that data and stuff it into the first frame.

+            size_t numExtraBytes = mAudioChannels * mAudioRate * audioTime;

mAudioChannels*PRInt32(NS_ceil(mAudioRate*audioTime))

+            memcpy(dst, data, numExtraBytes);

Wrong copy size

Comment 7

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

Attachment: Patch v2

With review comments addressed.

Comment 8

[cajbir (:cajbir)]

Attachment: test for seeking to end

Download this file:

http://tinyvid.tv/file/2g4f61jmrrz06.ogg

Put the file in a local directory along with the attached .html file. Load the page in the browser, click 'end'. The assertion about there being no frame will fire.

Comment 9

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

Attachment: Patch v3

My update to avoid throwing away the last frame.

Comment 10

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

With this patch I sometimes see us failing to seek to a keyframe initially so we get I-frame garbage, but I don't think it's a regression in this patch. Must be a bug in the seek-to-keyframe logic. I'll file a new bug for that.

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/a983f0d35311
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13b3b54d6d39

Comment 12

[Tony Chung [:tchung]]

I made a https://bugzilla.mozilla.org/show_bug.cgi?id=466699#c24 that this is still broken for me.  Perhaps i should move the issue over here?

Comment 13

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

(In reply to comment #12)
> I made a https://bugzilla.mozilla.org/show_bug.cgi?id=466699#c24 that this is
> still broken for me.  Perhaps i should move the issue over here?

Filed Bug 495004 for this. Thanks for spotting this Tony.

Comment 14

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre Ubiquity/0.1.5
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre