715323 - nsBuiltinDecoderReader::NotifyDataArrived() should have a 64bit offset parameter

Status: RESOLVED FIXED

Description

[Chris Pearce (:cpearce)]

The function void nsBuiltinDecoderReader::NotifyDataArrived(const char* aBuffer, PRUint32 aLength, PRUint32 aOffset) and its overrides should really have aOffset as 64bit. Someday some crazy fool is going to be bitten by this. My money's on Greg Maxwell being the first.

Comment 1

[Chris DeCairos (:cade)]

 Should all instances of NotifyDataArrived be set to 64 bit offsets? 

When I search in MXR (http://mxr.mozilla.org/mozilla-central/search?string=NotifyDataArrived) for "NotifyDataArrived", I see it in nsWebMBufferedParser and nsWebMReader, along with nsBuiltinDecoderStateMachine, nsMediaDecoder and nsMediaStream.

Comment 2

[Chris Pearce (:cpearce)]

Yes, they should all be 64bits.

Comment 3

[Chris DeCairos (:cade)]

Created attachment 592054 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use a 64 bit offset Parameter

I think I've gotten every one of them.

Comment 4

[Chris Pearce (:cpearce)]

Comment on attachment 592054 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use a 64 bit offset Parameter

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

I think these should all be PRInt64, since nsMediaChannelStream::CopySegmentToCache() (which is were all of these offsets originate) passes in a PRInt64 value, and everywhere else we use PRInt64, and _occasionally_ we pass around a -1 offset to imply a failure condition. We may as well be consistent.

Comment 5

[Chris DeCairos (:cade)]

Created attachment 592949 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter

I've updated the patch so that all the offset parameters are now PRInt64 types.

Comment 6

[Chris Pearce (:cpearce)]

Comment on attachment 592949 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter

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

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1215,5 @@
>  }
>  
>  void nsBuiltinDecoderStateMachine::NotifyDataArrived(const char* aBuffer,
>                                                       PRUint32 aLength,
> +                                                     PRInt32 aOffset)

aOffset should be PRInt64? I get a compile error:

[...]content/media/nsBuiltinDecoderStateMachine.cpp(1220) : error C2511: 'void nsBuiltinDecoderStateMachine::NotifyDataArrived(const char *,PRUint32,PRInt32)' : overloaded member function not found in 'nsBuiltinDecoderStateMachine'

I think with that fixed we should be good to go. Thanks!

Comment 7

[Chris DeCairos (:cade)]

Created attachment 592971 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter

Whoops, bit of an oversight on my part there. Fixed in the new patch.

Comment 8

[Chris DeCairos (:cade)]

( Thanks for the reviews! )

Comment 9

[Chris Pearce (:cpearce)]

Comment on attachment 592971 [details] [diff] [review]
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter

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

Looks good! Thanks!

Comment 10

[Chris Pearce (:cpearce)]

I pushed the patch to try, looks like the tests pass:
https://tbpl.mozilla.org/?tree=Try&rev=729d0e214bc6

Comment 11

[Chris Pearce (:cpearce)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fad2329779e6

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fad2329779e6