644045 - Video track of Ogg file freezes after a few frames but audio continues

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[youngjin.lee.ku]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.151 Safari/534.16
Build Identifier: Firefox 4.0: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

HTML5 video tag which worked fine on Firefox 3 stopped working on the latest Firefox 4. The same video tag works fine on Safari and Chrome. 

Firefox 4 was able to play first a few frames, but then it got stuck. You can easily see the problem in the Web site I set up (http://people.ku.edu/~yjlee/Test/firefox4_html5_video_problem.html). 

Reproducible: Always

Steps to Reproduce:
1. Create a simple Web site using *.ogv and html5 video tag.
2. Or simply visit the Web site I set up (http://people.ku.edu/~yjlee/Test/firefox4_html5_video_problem.html)
3.
Actual Results:  
After playing first a few frames, Firefox 4 got stuck.

Expected Results:  
Firefox 4 should be able to play html5 videos. Previous version of Firefox had no problem playing this video.

Comment 1

[Matthias Versen [:Matti]]

confirming with Seamonkey trunk on win32. Works fine in Opera11

Comment 2

[Matthew Gregan [:kinetik]]

Looks like there's either something wrong with the video timestamps in the file or our timestamp handling code.  OggIndex asserts with:

326                 assert((prev_granulepos & ((1 << shift) - 1)) > 0);
(gdb) p prev_granulepos
$1 = 66571993088
(gdb) p shift
$2 = 31
(gdb) p ((1 << shift) - 1)
$3 = 2147483647

Comment 3

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

Attachment: Patch

It looks to me like the file has an invalid granulepos in first Theora non-header page.

`OggIndex -d -p` gives:

[...]
[T] page @58848 length=61495 granulepos=66571993088 end_time=2066ms s=3314235337 packet_starts=32 packet_ends=31 checksum=3950262009
[T] keyframe time_ms=[143165576466,143165576533] granulepos=4611686018427387904 packetno=3
[T] frame time_ms=[143165576533,143165576600] granulepos=66571993059 packetno=4
[T] frame time_ms=[143165576600,143165576666] granulepos=66571993060 packetno=5
[T] frame time_ms=[143165576666,143165576733] granulepos=66571993061 packetno=6
[...]
[T] frame time_ms=[143165578266,143165578333] granulepos=66571993085 packetno=30
[T] frame time_ms=[143165578333,143165578400] granulepos=66571993086 packetno=31
[T] frame time_ms=[143165578400,143165578466] granulepos=66571993087 packetno=32
[T] frame time_ms=[2000,2066] granulepos=66571993088 packetno=33
[...]

Our Theora granulepos-to-time code works by reading pages until we find the first page with a granulepos, and then we use that as the granulepos of the last frame on that page, and we back-propagate the granulepos, subtracting 1 each time, to determine each previous frames' granulepos.

The problem with this file is that the "pframe" chunk of the first known granulepos (66571993088) is 0 (the granule shift is 31, the maximum). i.e. for the first known granulepos, (granulepos-((granulepos>>shift)<<shift))==0. We can't just subtract 1 to back propagate from the known granulepos to get the granulepos of the preceding frames, as the subtraction will "borrow" from the iframe chunk of the granulepos, screwing up the granulepos-to-time calculation. Instead of subtracting 1 from the granulepos in this case, we need to "borrow" from the iframe chunk so we can decrement the pframe chunk, as we do when we back propagate after a keyframe (where the pframe chunk is also 0).

This patch makes that change. Requesting review from Tim, seeing as he's our resident Ogg/Theora granulepos expert.

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 521365 [details] [diff] [review]
Patch

So, if I'm reading this code right, as you're back-propagating, the first time you encounter a keyframe, you'll mark the preceding frame as requiring a keyframe 2**31 frames prior to decode, and propagate this backwards until you find a second keyframe, where you'll set the granpos correctly. I.e., the backpointers for all the frames that come after that second keyframe will be wrong.

Wouldn't it make more sense to work forwards? The frameno of each frame is easy to compute given the frameno of the final frame. The only question is what to use as the keyframe backpointer. You can start with the one from the granulepos you did find. If that's in the future, then you can use the max_offset strategy you use here. If you encounter an actual keyframe, then you can use that for subsequent frames. This means you'll only have invalid backpointers for frames you really can't decode.

I don't know what the effect of an invalid backpointer is on the rest of the media pipeline, but I assume it would interfere with seeks very near the beginning?

These are pretty obscure corner cases (in the above example you'd need to have two keyframes in the first 64kB of data), but they are possible with FFmpeg's default "minimal muxing overhead" behavior.

Comment 5

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

Attachment: Patch v2: Work forwards

Reworked to loop forwards rather than backwards, which is much more sensible.

Comment 6

[Timothy B. Terriberry (:derf)]

Comment on attachment 521401 [details] [diff] [review]
Patch v2: Work forwards

>+      // this later case, we're probably wrong, but we can't decode the frame

latter?

>+      ogg_int64_t keyframeno = frames[0]->mKeyframe ? firstFrameNo :
>+        NS_MAX(firstFrameNo - (ogg_int64_t)(1u << shift) - 1, (ogg_int64_t)1);

th_granule_frame() returns a frame index (0-based) instead of a frame count (1-based), so shouldn't the max here be against 0, not 1?

>+          NS_ASSERTION(frameno >= keyframeno, "Keyframeno must preceed frameno.");
>+          granulepos = (keyframeno << shift) + (frameno - keyframeno);

So, when you set keyframeno above, in the "stream doesn't start with a keyframe" case you use the maximum offset. If the second frame is still not a keyframe, then (frameno - keyframeno) here will overflow that offset, which you aren't checking for. Instead of asserting that frameno >= keyframeno, you could move the decision here: if frameno >= keyframeno and (frameno - keyframeno) < (1U << shift), then do what you're doing here. Otherwise, use NS_MAX(firstFrameNo - (ogg_int64_t)(1u << shift) - 1, (ogg_int64_t)0) in place of keyframeno, but don't actually store it in the keyframeno variable. The check for overflow is important. Even if you've seen a valid keyframe, you could (in theory, and in practice with 0-byte drop frames) have up to 255 packets on the first page, which would allow you to overflow a small maximum offset (the default is only 64 frames) if the file doesn't actually obey its maximum keyframe spacing.

>+        NS_ASSERTION(granulepos > 0 || (!version_3_2_1 && granulepos >= 0),
>+                     "Invalid granulepos for Theora version");

This could also just be granulepos >= version_3_2_1, which covers both cases.

Comment 7

[Timothy B. Terriberry (:derf)]

> (1U << shift), then do what you're doing here. Otherwise, use
> NS_MAX(firstFrameNo - (ogg_int64_t)(1u << shift) - 1, (ogg_int64_t)0) in place

Sorry, that should be frameno, not firstFrameNo.

Comment 8

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

(In reply to comment #6)
> Comment on attachment 521401 [details] [diff] [review]
> Patch v2: Work forwards

> >+      ogg_int64_t keyframeno = frames[0]->mKeyframe ? firstFrameNo :
> >+        NS_MAX(firstFrameNo - (ogg_int64_t)(1u << shift) - 1, (ogg_int64_t)1);
> 
> th_granule_frame() returns a frame index (0-based) instead of a frame count
> (1-based), so shouldn't the max here be against 0, not 1?

Right, I was trying to store the frame index for Theora version < 3.2.1, and the frame number for Theora version >= 3.2.1. It's confusing that I'm using "frame number" in the comments and frameno in the variable names...

Because lastFrameNo has version_3_2_1 added in, and because firstFrameNo is calculated relative to that, lastFrameNo, firstFrameNo, keyframeno and frameno should be frame numbers for Theora version >= 3.2.1, otherwise they're indexes.

So maybe that should be:

      ogg_int64_t keyframeno = frames[0]->mKeyframe ? firstFrameNo :
        NS_MAX(firstFrameNo - ((ogg_int64_t)1 << shift) - 1, version_3_2_1);

And I should change the comments, and remove the "no" from all the variable names.

Comment 9

[Timothy B. Terriberry (:derf)]

(In reply to comment #8)
> Right, I was trying to store the frame index for Theora version < 3.2.1, and
> the frame number for Theora version >= 3.2.1. It's confusing that I'm using
> "frame number" in the comments and frameno in the variable names...

Ah, indeed you are. I actually missed the "+ version_3_2_1" at the end (though I should have noticed that would have screwed up the granpos calculations if it weren't there!).

> So maybe that should be:
> 
>       ogg_int64_t keyframeno = frames[0]->mKeyframe ? firstFrameNo :
>         NS_MAX(firstFrameNo - ((ogg_int64_t)1 << shift) - 1, version_3_2_1);

Agreed, with the caveat that the next comment I made still applies (i.e., you should be using this formula in the loop, not as an initial condition, or you'll overflow the offset).

Comment 10

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

Attachment: Patch v3

With review comments addressed.

Comment 11

[Timothy B. Terriberry (:derf)]

Comment on attachment 522279 [details] [diff] [review]
Patch v3

> +      ogg_int64_t keyframe = firstFrame;

I think this should be
+      ogg_int64_t keyframe = packet.granulepos>>shift;

If the backpointer from packet.granulepos points prior to firstFrame, then using it here instead of firstFrame will correctly recover all the original granuleposes. I also think it's better to use granuleposes that refer back to a non-existent frame when the stream doesn't start with a keyframe rather than pretend we can get valid decode results before we see a keyframe. My proposal will do this regardless of whether keyframe < firstFrame or not (in the case that it isn't, we'll trigger the 
> +        } else if (frame >= keyframe &&
check, which is actually impossible to hit with the current code).

+          ogg_int64_t k = NS_MAX(frame - (((ogg_int64_t)1 << shift) - 1), version_3_2_1);

What happens when frame < version_3_2_1? I mean, the current behavior is that (frame - k) will overflow and we'll trigger a bunch of assertions, but perhaps there should be an explicit check for firstFrame < version_3_2_1 before the loop with some kind of explicit handling? I'm not sure what behavior you actually want in such an invalid case (and I'm not actually opposed to the current behavior), but worth thinking about.

+        // Check that the frame's granule number is one more than the
+        // previous frame's.
It might also be worth doing this for the last frame (if there is one), after the loop, to double-check we got back to packet.granulepos.

Anyway, those are all just suggestions.
r+ with or without any of those changes.

Comment 12

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

Attachment: Patch v4

Updated:
* Use "ogg_int64_t keyframe = packet.granulepos>>shift;"
* Assert that the th_granule_frame(second to last frame's gp)+1 == th_granule_frame(packet.granulepos).

Based on current trunk. Carrying forward r=derf.

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/839c87b7e4bb

Comment 14

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

Can we have a test here?

Comment 15

[u279076]

Video in comment 0 plays fine -- VERIFIED FIXED