Last Comment Bug 715323 - nsBuiltinDecoderReader::NotifyDataArrived() should have a 64bit offset parameter
: nsBuiltinDecoderReader::NotifyDataArrived() should have a 64bit offset parameter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Chris DeCairos (:cade)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 13:50 PST by Chris Pearce (:cpearce)
Modified: 2012-02-01 11:14 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Changed all the NotifyDataArrived methods to use a 64 bit offset Parameter (9.62 KB, patch)
2012-01-26 21:48 PST, Chris DeCairos (:cade)
cpearce: review-
Details | Diff | Review
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter (9.62 KB, patch)
2012-01-30 18:12 PST, Chris DeCairos (:cade)
cpearce: review-
Details | Diff | Review
Changed all the NotifyDataArrived methods to use PRInt64 for the offset parameter (9.62 KB, patch)
2012-01-30 19:45 PST, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Review

Description Chris Pearce (:cpearce) 2012-01-04 13:50:40 PST
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) 2012-01-26 20:00:56 PST
 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) 2012-01-26 20:15:00 PST
Yes, they should all be 64bits.
Comment 3 Chris DeCairos (:cade) 2012-01-26 21:48:32 PST
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) 2012-01-30 15:10:02 PST
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) 2012-01-30 18:12:28 PST
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) 2012-01-30 19:08:36 PST
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) 2012-01-30 19:45:25 PST
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) 2012-01-30 19:46:35 PST
( Thanks for the reviews! )
Comment 9 Chris Pearce (:cpearce) 2012-01-30 19:50:08 PST
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) 2012-01-31 14:06:19 PST
I pushed the patch to try, looks like the tests pass:
https://tbpl.mozilla.org/?tree=Try&rev=729d0e214bc6
Comment 12 Ed Morley [:emorley] 2012-02-01 11:14:16 PST
https://hg.mozilla.org/mozilla-central/rev/fad2329779e6

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