The default bug view has changed. See this FAQ.

a/v sync problems when seeking

VERIFIED FIXED

Status

()

Core
Audio/Video
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: blizzard, Assigned: cpearce)

Tracking

({verified1.9.1})

Trunk
x86
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Flags: blocking1.9.1?

Comment 1

8 years ago
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.
Flags: blocking1.9.1? → blocking1.9.1+
(Reporter)

Comment 3

8 years ago
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'.
Assignee: nobody → chris
(Assignee)

Comment 4

8 years ago
Created attachment 378787 [details] [diff] [review]
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.
Attachment #378787 - Flags: superreview?(roc)
Attachment #378787 - Flags: review?(chris.double)

Comment 5

8 years ago
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
(Assignee)

Comment 7

8 years ago
Created attachment 378795 [details] [diff] [review]
Patch v2

With review comments addressed.
Attachment #378787 - Attachment is obsolete: true
Attachment #378795 - Flags: superreview?(roc)
Attachment #378795 - Flags: review?(chris.double)
Attachment #378787 - Flags: superreview?(roc)
Attachment #378787 - Flags: review?(chris.double)

Comment 8

8 years ago
Created attachment 378800 [details]
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.

Updated

8 years ago
Attachment #378795 - Flags: review?(chris.double) → review+
Created attachment 378802 [details] [diff] [review]
Patch v3

My update to avoid throwing away the last frame.
Attachment #378795 - Attachment is obsolete: true
Attachment #378795 - Flags: superreview?(roc)
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.
Attachment #378802 - Flags: review?(chris.double)

Updated

8 years ago
Attachment #378802 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/a983f0d35311
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13b3b54d6d39
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
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?
(Assignee)

Comment 13

8 years ago
(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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.