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

RESOLVED FIXED in mozilla13

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cpearce, Assigned: cade)

Tracking

Trunk
mozilla13
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

6 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

6 years ago
Yes, they should all be 64bits.
(Assignee)

Comment 3

6 years ago
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.
Attachment #592054 - Flags: review?(cpearce)
(Reporter)

Comment 4

6 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

6 years ago
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.
Attachment #592054 - Attachment is obsolete: true
Attachment #592949 - Flags: review?(cpearce)
(Reporter)

Comment 6

6 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

6 years ago
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.
Attachment #592949 - Attachment is obsolete: true
Attachment #592971 - Flags: review?(cpearce)
(Assignee)

Comment 8

6 years ago
( Thanks for the reviews! )
(Reporter)

Comment 9

6 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

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 10

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

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad2329779e6
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/fad2329779e6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.