Closed Bug 715323 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: cpearce, Assigned: cade)

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
 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.
Yes, they should all be 64bits.
I think I've gotten every one of them.
Attachment #592054 - Flags: review?(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.
Attachment #592054 - Flags: review?(cpearce) → review-
I've updated the patch so that all the offset parameters are now PRInt64 types.
Attachment #592054 - Attachment is obsolete: true
Attachment #592949 - Flags: review?(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!
Attachment #592949 - Flags: review?(cpearce) → review-
Whoops, bit of an oversight on my part there. Fixed in the new patch.
Attachment #592949 - Attachment is obsolete: true
Attachment #592971 - Flags: review?(cpearce)
( Thanks for the reviews! )
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!
Attachment #592971 - Flags: review?(cpearce) → review+
Keywords: checkin-needed
I pushed the patch to try, looks like the tests pass:
https://tbpl.mozilla.org/?tree=Try&rev=729d0e214bc6
https://hg.mozilla.org/mozilla-central/rev/fad2329779e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.