Last Comment Bug 646333 - Video constants should all consistently be either #define or const T
: Video constants should all consistently be either #define or const T
Status: RESOLVED FIXED
[good first bug]
: student-project
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Martin Richard
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-29 21:46 PDT by Chris Pearce (:cpearce)
Modified: 2011-09-15 07:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0 : #define statements transformed in const values (15.37 KB, patch)
2011-08-31 05:18 PDT, Martin Richard
cpearce: review-
Details | Diff | Splinter Review
The updated patch (15.38 KB, patch)
2011-09-12 04:39 PDT, Martin Richard
cpearce: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-03-29 21:46:00 PDT
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.
Comment 1 Martin Richard 2011-08-31 05:18:47 PDT
Created attachment 557121 [details] [diff] [review]
patch v0 : #define statements transformed in const values
Comment 2 Chris Pearce (:cpearce) 2011-09-06 15:50:24 PDT
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?
Comment 3 Martin Richard 2011-09-12 04:39:54 PDT
Created attachment 559843 [details] [diff] [review]
The updated patch

I updated the patch according to the review
Comment 4 Chris Pearce (:cpearce) 2011-09-12 15:34:55 PDT
Comment on attachment 559843 [details] [diff] [review]
The updated patch

Review of attachment 559843 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. Thanks!
Comment 5 Chris Pearce (:cpearce) 2011-09-14 13:16:56 PDT
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!
Comment 6 :Ehsan Akhgari 2011-09-15 07:36:31 PDT
https://hg.mozilla.org/mozilla-central/rev/ea752547a5b6

Thanks a lot Martin for your first patch!  It was great, keep up the good work.  :-)

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