500784 - Video/audio files over 2^31 bytes in length are unseekable

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Gregory Maxwell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5

Files over 2^31 bytes get interpenetrated as having invalid content lengths, and thus seeking does not work.

(long files also have a separate issue with the default controls; see bug 500764)

Server sends a correct content-length, etc.

For example:
http://myrandomnode.dyndns.org:8080/~gmaxwell/firefox/bach.test.ogg


Reproducible: Always

Comment 1

[cajbir (:cajbir)]

Attachment: Get content length as a 64 bit value

Comment 2

[cajbir (:cajbir)]

Seeking doesn't work because we are getting the content length of the file as a 32 bit value. This returns as a negative number which we interpret as if the content length not available and seeking is disabled.

The patch in attachment 385462 [details] [diff] [review] fixes this and the content length gets a valid value. Unfortunately we then crash in nsMediaCacheStream::Read (which is why I've CC'd Robert).

The offending line is:

> memcpy(aBuffer + count,
>       reinterpret_cast<char*>(mPartialBlockBuffer) + 
>         offsetInStreamBlock, bytes);

Here, bytes is a negative number. It's set to negative due to code further back:

> size = PR_MIN(size, PRInt32(bytesRemaining));

bytesRemaining is a PRInt64 calculated from the content length. Casting to a PRInt32 produces a negative number.

Changing this line such that size is set to the correct value causes crashes in other parts of nsMediaCache - I got lost in the code at that point.

Comment 3

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

liboggz internally uses off_t which is typedef'd as long (on Windows at least), and therefore 32bits anyway. So liboggz can't seek to after 2^31 anyway.

Comment 4

[cajbir (:cajbir)]

It's platform dependant for the type in liboggz - some platforms use a 64 bit type. But yes, it does look like Windows uses long.

Comment 5

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

Hmm. Chris P, do you want to keep working on this? Is liboggz 64-bit safe on Mac?

Comment 6

[cajbir (:cajbir)]

liboggz might be but it appears that liboggplay isn't. liboggplay's reader interface uses 'long' for the offset in 'seek' and 'tell' calls. Ditto with the reader's io_seek and io_tell which liboggz calls. That's probably not helping the issue.

Comment 7

[cajbir (:cajbir)]

I raised trac ticket 489 for the liboggplay issue:

https://trac.annodex.net/ticket/489

Comment 8

[cajbir (:cajbir)]

Looks like liboggz is too:

int oggz_io_seek (OGGZ * oggz, long offset, int whence)

Comment 9

[cajbir (:cajbir)]

I talked to conrad on irc about comment 8 and he agrees instead of 'long' it should be oggz_off_t. Unfortunately this breaks binary compatibility with other liboggz users so needs some thought on introducing a new interface for that.

Comment 10

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

I filed trac ticket 460 three months ago about using 32bit long and 32bit oggz_off_t (on Windows) as bytes-offsets during seeking in liboggz:

https://trac.annodex.net/ticket/460

I'm happy to do whatever we can to fix this in FF.

Comment 11

[Matthew Gregan [:kinetik]]

A related problem is that the "loaded" and "total" attributes on progress events are defined as "unsigned long", which is 32-bit according to the WebIDL spec.

Comment 12

[Viktor Gal]

I've created two simple patches to https://trac.annodex.net/ticket/460 for liboggplay + liboggz in order to support large file seeking. I won't commit the patch for liboggplay until the problem mentioned in comment 9 is not resolved.

Comment 13

[Viktor Gal]

(In reply to comment #12)
> I've created two simple patches to https://trac.annodex.net/ticket/460 for

I meant https://trac.annodex.net/ticket/489 instead of Ticket 460.

Comment 14

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

Not making 1.9.2.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: hacky fix v0

Ignoring stupid API compatibility problems, this is enough to fix things for me on a Mac/x86 build.  I might've missed stuff, and there's a bunch of size_t use that might need to be looked are more closely, but this patch is sufficient for the duration to be reported correctly (assuming all that Bach is indeed 162 hours, 56 minutes, and 27 seconds) and for seeking to work anywhere in the stream (at least, something plays at each seek point).

If you seek past the 2GB mark (e.g. to 157:01:40), you will constant hit the following assertion:
###!!! ASSERTION: Played block after the current stream position?: 'PRInt64(bo->mStreamBlock)*BLOCK_SIZE < bo->mStream->mStreamOffset', file /Users/kinetik/work/mozilla-central/content/media/nsMediaCache.cpp, line 933

Comment 16

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

(In reply to comment #15)
> Created an attachment (id=403086) [details]
> hacky fix v0

oggz_off_t is typedef'd to off_t which is 32 bit on Windows. We should typedef ogg_int64_t to oggz_off_t on Windows in oggz_off_t.h. Without that, we can't seek to get the duration of the 2.4GB Bach test file on Windows. Once we do that, seeking works perfectly on large files on Windows.

I spoke to Conrad about this on IRC some time ago, and he agreed that this was probably a sensible approach, but I never chased him up about it, and unfortunately nothing happened... :(

Comment 17

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

(In reply to comment #15)
> If you seek past the 2GB mark (e.g. to 157:01:40), you will constant hit the
> following assertion:
> ###!!! ASSERTION: Played block after the current stream position?:
> 'PRInt64(bo->mStreamBlock)*BLOCK_SIZE < bo->mStream->mStreamOffset', file
> /Users/kinetik/work/mozilla-central/content/media/nsMediaCache.cpp, line 933

This should be easy to fix. What are the values of bo->mStreamBlock and bo->mStream->mStreamOffset?

Comment 18

[Viktor Gal]

(In reply to comment #15)
> Created an attachment (id=403086) [details]
> hacky fix v0
> 
> Ignoring stupid API compatibility problems, this is enough to fix things for me
> on a Mac/x86 build.  I might've missed stuff, and there's a bunch of size_t use
> that might need to be looked are more closely, but this patch is sufficient for
> the duration to be reported correctly (assuming all that Bach is indeed 162
> hours, 56 minutes, and 27 seconds) and for seeking to work anywhere in the
> stream (at least, something plays at each seek point).
> 
> If you seek past the 2GB mark (e.g. to 157:01:40), you will constant hit the
> following assertion:
> ###!!! ASSERTION: Played block after the current stream position?:
> 'PRInt64(bo->mStreamBlock)*BLOCK_SIZE < bo->mStream->mStreamOffset', file
> /Users/kinetik/work/mozilla-central/content/media/nsMediaCache.cpp, line 933

have you checked the patches i've uploaded to https://trac.annodex.net/ticket/489 a while ago?

Comment 19

[Matthew Gregan [:kinetik]]

No.  But looking at them now, oggz_io_tell is still broken.  Local variable |offset| is declared as |long|.

Comment 20

[Matthew Gregan [:kinetik]]

One example:

(gdb) p bo->mStream->mStreamOffset
$24 = 2469139500
(gdb) p bo->mStreamBlock*BLOCK_SIZE
$25 = 2469175296
(gdb) p (bo->mStreamBlock*BLOCK_SIZE)-bo->mStream->mStreamOffset
$26 = 35796

Other than the assert, I'm not seeing anything obviously wrong yet... but my eyes are glazing over.  Something for tomorrow morning, I guess.

Comment 21

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

OK, that is a problem then ... potentially screwing up cache replacement, but probably not hurting if the cache isn't full. I can debug it when your patches land I guess

Comment 22

[Matthew Gregan [:kinetik]]

Another test URL, this one has video: http://www.0xdeadbeef.com/~blizzard/media/BergensBanen720p_mLogo.ogv

It's 5350618759 bytes (4.9GB), so it's well outside the 2^31 limit.  The video appears to behave okay (it has a duration and you can seek within the video), but it's all wrong--the duration is only 1.5hrs instead of ~7.

Comment 23

[Matthew Gregan [:kinetik]]

This should be a lot easier to fix in the new backend--should only need the fixes from my patch for the media cache plus some testing.

Comment 24

[Abhishek Bhatnagar]

I've been working on similar things and I'd like to take on this bug. Could I be assigned to it?

Comment 25

[Abhishek Bhatnagar]

Attachment: Proposed patch for bug 500784

Video/Audio files over 2^31 bytes now return proper durations. Based on Matthew Gregan and Chris Double's patches attached to the same bug.

Comment 26

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

Comment on attachment 608281 [details] [diff] [review]
Proposed patch for bug 500784

Review of attachment 608281 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsMediaCache.cpp
@@ +667,5 @@
>    }
>  }
>  
>  nsresult
>  nsMediaCache::ReadCacheFile(PRInt64 aOffset, void* aData, PRInt32 aLength,

aLength is 32bit, there's no way *aBytes should ever be greater than aLength. Don't make these changes to nsMediaCache, leave all the aBytes/bytes parameters/variables as 32bit. Any read greater than 32bit is not something we want to be doing anyway, it would require a memory buffer larger than 2GB!

Comment 27

[Abhishek Bhatnagar]

Attachment: Proposed patch for bug 500784 - Updated

Second try.
Removed unneeded parts.
Tested.

Comment 28

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

Comment on attachment 613479 [details] [diff] [review]
Proposed patch for bug 500784 - Updated

Review of attachment 613479 [details] [diff] [review]:
-----------------------------------------------------------------

Tested here too, works fine, thanks!

Comment 29

[Sid]

Gentle ping. Can we land this patch?

Comment 30

[Ralph Giles (:rillian)]

Thanks for the reminder. The patch still applies cleanly.

https://tbpl.mozilla.org/?tree=Try&rev=e68a9dd30e7f

Comment 31

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d71664b8ec0

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d71664b8ec0