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

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Gregory Maxwell, Assigned: Abhishek Bhatnagar)

Tracking

unspecified
mozilla15
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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

8 years ago
Created attachment 385462 [details] [diff] [review]
Get content length as a 64 bit value

Comment 2

8 years ago
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.
Assignee: nobody → roc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9.2+
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

8 years ago
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.
Hmm. Chris P, do you want to keep working on this? Is liboggz 64-bit safe on Mac?

Comment 6

8 years ago
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

8 years ago
I raised trac ticket 489 for the liboggplay issue:

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

Comment 8

8 years ago
Looks like liboggz is too:

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

Comment 9

8 years ago
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.
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.
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

8 years ago
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

8 years ago
(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.
Not making 1.9.2.
Assignee: roc → nobody
Flags: blocking1.9.2+ → blocking1.9.2-
Status: ASSIGNED → NEW
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
(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... :(
(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

8 years ago
(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?
No.  But looking at them now, oggz_io_tell is still broken.  Local variable |offset| is declared as |long|.
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.
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
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.
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.
(Assignee)

Comment 24

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

Updated

5 years ago
Assignee: nobody → abhishekbh
(Assignee)

Comment 25

5 years ago
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.
Attachment #608281 - Flags: review?(gavin.sharp)

Updated

5 years ago
Attachment #608281 - Flags: review?(gavin.sharp) → review?(kinetik)
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!
Attachment #608281 - Flags: feedback-
(Assignee)

Comment 27

5 years ago
Created attachment 613479 [details] [diff] [review]
Proposed patch for bug 500784 - Updated

Second try.
Removed unneeded parts.
Tested.
Attachment #608281 - Attachment is obsolete: true
Attachment #613479 - Flags: review?(cpearce)
Attachment #608281 - Flags: review?(kinetik)
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!
Attachment #613479 - Flags: review?(cpearce) → review+

Comment 29

5 years ago
Gentle ping. Can we land this patch?
Thanks for the reminder. The patch still applies cleanly.

https://tbpl.mozilla.org/?tree=Try&rev=e68a9dd30e7f
Keywords: checkin-needed

Comment 31

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d71664b8ec0
Keywords: checkin-needed
Target Milestone: --- → mozilla14

Updated

5 years ago
Target Milestone: mozilla14 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/4d71664b8ec0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.