Closed
Bug 646333
Opened 13 years ago
Closed 13 years ago
Video constants should all consistently be either #define or const T
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: cpearce, Assigned: martius)
Details
(Keywords: student-project, Whiteboard: [good first bug])
Attachments
(2 files)
15.37 KB,
patch
|
cpearce
:
review-
|
Details | Diff | Splinter Review |
15.38 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
In the content/media code we've got lots of constants, some of which are #defined, and others are decalred const T = value. It's important for all of these domain conversions to use a consistent name and define/const so they're easy to grep for and verify.
Updated•13 years ago
|
Keywords: student-project
Whiteboard: [good first bug]
Updated•13 years ago
|
Assignee: nobody → martius
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #557121 -
Flags: review?(chris)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 557121 [details] [diff] [review] patch v0 : #define statements transformed in const values Review of attachment 557121 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for slow review. Just some minor changes, fix them up and I'll grant review. :) Thanks for doing this! ::: content/media/VideoUtils.h @@ +149,5 @@ > // The maximum height and width of the video. Used for > // sanitizing the memory allocation of the RGB buffer. > // The maximum resolution we anticipate encountering in the > // wild is 2160p - 3840x2160 pixels. > +static const PRUint32 MAX_VIDEO_WIDTH = 4000; Make these two PRInt32 to avoid adding a signed/unsigned comparison compile error in nsBuiltinDecoderReader. ::: content/media/nsBuiltinDecoderStateMachine.cpp @@ +58,5 @@ > > // Wait this number of seconds when buffering, then leave and play > // as best as we can if the required amount of data hasn't been > // retrieved. > +static const double BUFFERING_WAIT = 30; This should be an integral type, PRUint32 should suffice. ::: content/media/nsMediaDecoder.cpp @@ +56,5 @@ > > using namespace mozilla; > > // Number of milliseconds between progress events as defined by spec > +static const double PROGRESS_MS = 350; PRUint32. @@ +61,3 @@ > > // Number of milliseconds of no data before a stall event is fired as defined by spec > +static const double STALL_MS = 3000; PRUint32. ::: content/media/nsMediaStream.cpp @@ +62,5 @@ > #include "mozilla/Util.h" // for DebugOnly > #include "nsContentUtils.h" > > +static const PRUint32 HTTP_OK_CODE = 200; > +static const PRUint32 HTTP_PARTIAL_RESPONSE_CODE = 206; s/PRUint32 HTTP_PARTIAL_RESPONSE_CODE/PRUint32 HTTP_PARTIAL_RESPONSE_CODE/ ::: content/media/ogg/nsOggCodecState.cpp @@ +781,3 @@ > > // Minimum possible size of a compressed index keypoint. > +static const PRInt64 MIN_KEY_POINT_SIZE = 2; Make this, and all the offsets below size_t. ::: content/media/ogg/nsOggReader.cpp @@ +67,5 @@ > // target is less than SEEK_DECODE_MARGIN ahead of the current playback > // position, we'll just decode forwards rather than performing a bisection > // search. If we have Theora video we use the maximum keyframe interval as > // this value, rather than SEEK_DECODE_MARGIN. This makes small seeks faster. > +static const PRUint32 SEEK_DECODE_MARGIN = 2000000; Ooops, looks like we don't use this anymore. Can you remove it?
Attachment #557121 -
Flags: review?(chris) → review-
Assignee | ||
Comment 3•13 years ago
|
||
I updated the patch according to the review
Attachment #559843 -
Flags: review?(chris)
Reporter | ||
Comment 4•13 years ago
|
||
Comment on attachment 559843 [details] [diff] [review] The updated patch Review of attachment 559843 [details] [diff] [review]: ----------------------------------------------------------------- Excellent. Thanks!
Attachment #559843 -
Flags: review?(chris) → review+
Reporter | ||
Comment 5•13 years ago
|
||
Landed in mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea752547a5b6 I updated the commit message field to include the bug number, and to be in the active voice. Bug will be changed to Resolved > Fixed once mozilla-inbound merges with mozilla-central. Thanks for the patch Martin!
Whiteboard: [good first bug] → [good first bug][inbound]
Target Milestone: --- → mozilla9
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ea752547a5b6 Thanks a lot Martin for your first patch! It was great, keep up the good work. :-)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][inbound] → [good first bug]
You need to log in
before you can comment on or make changes to this bug.
Description
•