Last Comment Bug 516847 - oggz calculates granulepos incorrectly for duplicate frames
: oggz calculates granulepos incorrectly for duplicate frames
Status: RESOLVED FIXED
: verified1.9.1
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
http://tinyvid.tv/file/jef5tcauf97o.ogg
: 498279 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-15 18:12 PDT by cajbir (:cajbir)
Modified: 2009-11-23 17:11 PST (History)
6 users (show)
roc: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.6-fixed


Attachments
patch v0 (733 bytes, patch)
2009-09-15 23:26 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
patch v0 with update.sh etc. (2.56 KB, patch)
2009-09-15 23:32 PDT, Matthew Gregan [:kinetik]
cajbir.bugzilla: review+
roc: approval1.9.2+
dveditz: approval1.9.1.6+
Details | Diff | Review

Description cajbir (:cajbir) 2009-09-15 18:12:28 PDT
TinyVid recently moved to using the thusnelda encoder to encode the Theora videos. This encoder is more aggressive at marking duplicate frames so decoders don't need to do so much work decoding them.

Unfortunately liboggplay based players (Firefox being one) don't seem to handle this well. The first ten seconds of the video in the URL don't play correctly. The time skips and it misses some of the frames that contain different content.

Steps to reproduce:

1) Visit URL
2) Play

What happens:

1) Watch the time in the controls skip from 1 to 3 to 10 seconds. 
2) Notice the first ten seconds of the video are all black.

What should happen:

1) Time increases in a sane manner
2) The first ten seconds show the title information (Crazy G, U900 text).

Note that this video crashes Firefox 3.5.3 so you'll need to test on trunk or Firefox 3.5.4pre.
Comment 1 Matthew Gregan [:kinetik] 2009-09-15 22:33:20 PDT
Looks like this is to do with the granulepos/presentation time calculation in oggz and oggplay.

Starting at the first data packet, I see the following granulepos from oggz_tell_granulepos (DF == duplicate frame):

256
512 (DF)
768 (DF)
259 (DF)
...
264 (DF)
2560 (DF)
2816 (DF)
3072 (DF)
268 (DF)

oggplay_callback_theora has the following early exit:
94	  if ( (granulepos > 0) && (common->last_granulepos > granulepos)) {

So we end up skipping the decode and frame enqueue of a large number of frames.
Comment 2 Matthew Gregan [:kinetik] 2009-09-15 23:26:05 PDT
Created attachment 400954 [details] [diff] [review]
patch v0

oggz's auto granulepos calculation does not handle zero-byte (duplicate frame) packets correctly.  When the packet size is zero, it reads a byte of random garbage from the heap.
Comment 3 Matthew Gregan [:kinetik] 2009-09-15 23:32:57 PDT
Created attachment 400955 [details] [diff] [review]
patch v0 with update.sh etc.
Comment 4 Matthew Gregan [:kinetik] 2009-09-15 23:34:20 PDT
It's tricky to get this with mochitest, I think.  We could check the currentTime when timeupdate fires to ensure it's advancing sanely, but I suspect this sort of test will be prone to random orange due to timing dependencies.
Comment 5 Matthew Gregan [:kinetik] 2009-09-22 01:33:55 PDT
http://hg.mozilla.org/mozilla-central/rev/30dc0db2b136
Comment 6 Matthew Gregan [:kinetik] 2009-09-22 01:36:12 PDT
Comment on attachment 400955 [details] [diff] [review]
patch v0 with update.sh etc.

Simple patch that fixes poor playback with duplicate frames caused by reading outside the bounds of a buffer.

I wonder if we want to take this for 1.9.1.5, too, since duplicate frames are going to become significantly more common once the Theora 1.1 (Thusnelda) decoder is released.
Comment 7 Conrad Parker 2009-09-22 03:01:41 PDT
Applied to liboggz upstream, commit 5405282442397372897cd6d148820eb518ffaf83
Comment 8 Matthew Gregan [:kinetik] 2009-09-22 17:40:36 PDT
*** Bug 498279 has been marked as a duplicate of this bug. ***
Comment 9 Chris Pearce (:cpearce) 2009-10-05 20:04:32 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/46421ae80897
Comment 10 Daniel Veditz [:dveditz] 2009-10-26 14:48:28 PDT
Comment on attachment 400955 [details] [diff] [review]
patch v0 with update.sh etc.

Approved for 1.9.1.5, a=dveditz for release-drivers
Comment 11 Matthew Gregan [:kinetik] 2009-10-27 15:36:08 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8754481f4c53
Comment 12 Al Billings [:abillings] 2009-11-23 17:11:39 PST
Verified for 1.9.1.6 using video url. Video plays back smoothly with 1.9.1.6 where it jumps around for 1.9.1.5.

 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)

Note You need to log in before you can comment on or make changes to this bug.