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.

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

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.

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

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

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

With review comments addressed.

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.

My update to avoid throwing away the last frame.

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.

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

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?

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

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