495366 - liboggz oggz_get_next_page() and oggz_read_get_next_page() wrong

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

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

The liboggz functions liboggz oggz_get_next_page() and oggz_read_get_next_page() are wrong. This causes us to incorrectly report the duration of a media, and also will cause liboggplay to incorrectly report the offset of pages while seeking.

This bug spun off bug 494305, which contains two liboggz fixes. This bug is the first of those two fixes, and is filed separately so that we can land it separately for 191.

Conrad Parker has looked over the patch, and says it looks ok, so we should take it to fix the duration.

Here's my explanation of the bug that I sent Conrad:

There are two problems with oggz_get_next_page():

1. oggz_get_next_page() often returns the offset of the page after the next page, not the offset of the next page.

After a seek the ogg_sync_state buffer is empty. So the first time through oggz_get_next_page()'s main loop, we fill the buffer, and set bytes > 0. Subsequent iterations will advance to potential capture points. If ogg_sync_pageseek() finds a complete page without needing to rebuffer, we break out of the loop. At this point page_offset is the offset of the page, and |more| is the length of the page. Then because (bytes > 0), we set:

   oggz->offset = oggz_tell_raw (oggz) - bytes + page_offset

So oggz->offset now hold the offset of the first page after the previous offset (the seek position). But we return oggz->offset + more which is the offset of the page following the next page, so it's actually the one after the page we want.


2. When calling oggz_get_next_page() after a seek, if we have to fill the sync buffer twice, the offset is calculated incorrectly. After a seek the ogg_sync_state buffer is empty. So the first time through oggz_get_next_page()'s main loop, we fill the buffer. Subsequent iterations will advance to potential capture points. If ogg_sync_pageseek() finds a capture point but it doesn't have the entire page in the sync state buffer, it returns 0. oggz_get_next_page() then calls ogg_sync_buffer() which slides the buffer's window to the start of the capture pattern. Then oggz_get_next_page() reads, putting another 4K in the buffer, and moving the raw IO cursor. In the next iteration, ogg_sync_pageseek() finds the capture pattern and the complete page in the buffer, so succeeds, and breaks out of the loop (with page_offset=0, and bytes=length of second read).

Then because (bytes > 0), we set the oggz->offset to be:
  oggz_tell_raw (oggz) - bytes + page_offset;

But because we've done two reads, the IO cursor will be at the end of the second read, and that expression is wrong. "oggz_tell_raw (oggz) - bytes" is the offset of the start of the second chunk read, but the start of the page lies in the first chunk read. So it's out by the (previous page_offset) - (previous bytes).

We go on to return oggz->offset + more, which misses a page again, as I explained in point 1.

Comment 1

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

Attachment: Patch v1

Patch v1, as per bug 494305 "patch v2 part 1".

Comment 2

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

Requesting blocking, since this ensures that we report the duration of a media correctly, and because this we need this to fix bug 495159 and bug 495145 properly.

Comment 3

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

(In reply to comment #2)
> Requesting blocking, since this ensures that we report the duration of a media
> correctly, and because this we need this to fix bug 495159 and bug 495145
> properly.

I mean we need this to fix bug 495300 not bug 495145.

Comment 4

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/5005b48c78ae

Comment 5

[Joe Drew (not getting mail)]

This patch doesn't apply cleanly to mozilla-1.9.1; can you post an updated patch?

Comment 6

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

Attachment: Patch v1 (1.9.1 branch)

As v1, but for 1.9.1 branch.

Comment 7

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6728594f4cf2

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk and 1.9.1 on all platforms with builds like

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre ID:20090604031228

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre ID:20090604031153