Closed
Bug 715323
Opened 9 years ago
Closed 9 years ago
nsBuiltinDecoderReader::NotifyDataArrived() should have a 64bit offset parameter
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: cpearce, Assigned: cade)
Details
Attachments
(1 file, 2 obsolete files)
9.62 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Assignee: nobody → chris
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 2•9 years ago
|
||
Yes, they should all be 64bits.
Assignee | ||
Comment 3•9 years ago
|
||
I think I've gotten every one of them.
Attachment #592054 -
Flags: review?(cpearce)
Reporter | ||
Comment 4•9 years ago
|
||
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-
Assignee | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
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-
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
( Thanks for the reviews! )
Reporter | ||
Comment 9•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 10•9 years ago
|
||
I pushed the patch to try, looks like the tests pass: https://tbpl.mozilla.org/?tree=Try&rev=729d0e214bc6
Reporter | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad2329779e6
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•