Last Comment Bug 500784 - Video/audio files over 2^31 bytes in length are unseekable
: Video/audio files over 2^31 bytes in length are unseekable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Abhishek Bhatnagar
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-26 13:41 PDT by Gregory Maxwell
Modified: 2012-07-20 07:56 PDT (History)
14 users (show)
roc: blocking1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Get content length as a 64 bit value (1.44 KB, patch)
2009-06-26 13:59 PDT, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
hacky fix v0 (17.39 KB, patch)
2009-09-26 22:20 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
Proposed patch for bug 500784 (5.02 KB, patch)
2012-03-22 03:20 PDT, Abhishek Bhatnagar
cpearce: feedback-
Details | Diff | Splinter Review
Proposed patch for bug 500784 - Updated (1.40 KB, patch)
2012-04-09 20:28 PDT, Abhishek Bhatnagar
cpearce: review+
Details | Diff | Splinter Review

Description Gregory Maxwell 2009-06-26 13:41:52 PDT
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) 2009-06-26 13:59:33 PDT
Created attachment 385462 [details] [diff] [review]
Get content length as a 64 bit value
Comment 2 cajbir (:cajbir) 2009-06-26 14:05:26 PDT
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) 2009-06-26 15:51:30 PDT
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) 2009-06-26 19:30:55 PDT
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) 2009-06-27 03:12:59 PDT
Hmm. Chris P, do you want to keep working on this? Is liboggz 64-bit safe on Mac?
Comment 6 cajbir (:cajbir) 2009-06-27 03:53:24 PDT
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) 2009-06-27 04:05:33 PDT
I raised trac ticket 489 for the liboggplay issue:

https://trac.annodex.net/ticket/489
Comment 8 cajbir (:cajbir) 2009-06-27 04:11:30 PDT
Looks like liboggz is too:

int oggz_io_seek (OGGZ * oggz, long offset, int whence)
Comment 9 cajbir (:cajbir) 2009-06-27 04:33:04 PDT
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) 2009-06-27 16:42:26 PDT
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] 2009-06-27 16:51:39 PDT
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 2009-07-17 01:38:53 PDT
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 2009-07-17 01:40:29 PDT
(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) 2009-09-22 20:41:10 PDT
Not making 1.9.2.
Comment 15 Matthew Gregan [:kinetik] 2009-09-26 22:20:35 PDT
Created attachment 403086 [details] [diff] [review]
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) 2009-09-26 23:58:56 PDT
(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) 2009-09-27 00:14:45 PDT
(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 2009-09-27 02:39:27 PDT
(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] 2009-09-27 02:47:13 PDT
No.  But looking at them now, oggz_io_tell is still broken.  Local variable |offset| is declared as |long|.
Comment 20 Matthew Gregan [:kinetik] 2009-09-27 03:25:52 PDT
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) 2009-09-27 06:37:25 PDT
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] 2010-02-23 18:10:42 PST
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] 2010-04-04 23:19:14 PDT
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 2012-03-15 11:15:44 PDT
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 2012-03-22 03:20:46 PDT
Created attachment 608281 [details] [diff] [review]
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) 2012-03-22 14:15:40 PDT
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 2012-04-09 20:28:17 PDT
Created attachment 613479 [details] [diff] [review]
Proposed patch for bug 500784 - Updated

Second try.
Removed unneeded parts.
Tested.
Comment 28 Chris Pearce (:cpearce) 2012-04-10 15:09:17 PDT
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 2012-05-29 06:03:51 PDT
Gentle ping. Can we land this patch?
Comment 30 Ralph Giles (:rillian) needinfo me 2012-05-29 10:30:55 PDT
Thanks for the reminder. The patch still applies cleanly.

https://tbpl.mozilla.org/?tree=Try&rev=e68a9dd30e7f
Comment 32 Ed Morley [:emorley] 2012-05-31 05:53:07 PDT
https://hg.mozilla.org/mozilla-central/rev/4d71664b8ec0

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