Closed Bug 714937 Opened 8 years ago Closed 7 years ago

replace GFX_PACKED_PIXEL with an inline function

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jrmuizel, Assigned: Luqman)

Details

(Whiteboard: [mentor=jrmuizel][lang=c++])

Attachments

(1 file, 5 obsolete files)

No description provided.
There's no need for GFX_PACKED_PIXEL to be a macro anymore and having it as an inline function will make things cleaner.
Whiteboard: [mentor=jrmuizel][lang=c++]
Comment on attachment 639149 [details] [diff] [review]
Replace the GFX_PACKED_PIXEL macro with an inline function.

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

::: gfx/thebes/gfxColor.h
@@ +107,5 @@
> +/** 
> + * Pack the 4 8-bit channels (A,R,G,B) 
> + * into a 32-bit packed premultiplied pixel.
> + */
> +PRUint32 inline

Use MOZ_ALWAYS_INLINE instead.

@@ +118,5 @@
> +    } else {
> +        return  ((a) << 24) |
> +                (GFX_PREMULTIPLY(r,a) << 16) |
> +                (GFX_PREMULTIPLY(g,a) << 8) |
> +                (GFX_PREMULTIPLY(b,a)); 

GFX_PREMULTIPLY and GFX_PACKED_PIXEL_NO_PREMULTIPLY could also be changed to inline functions. But you can do that in other patches.
Attachment #639149 - Flags: review-
Use MOZ_ALWAYS_INLINE.
Attachment #639149 - Attachment is obsolete: true
Attachment #639152 - Flags: review+
Replaced the other macros as well.
Attachment #639152 - Attachment is obsolete: true
Attachment #639158 - Flags: review?(jmuizelaar)
Attachment #639158 - Flags: review?(jmuizelaar) → review+
Assignee: nobody → laden
Status: NEW → ASSIGNED
Keywords: checkin-needed
This patch is badly bitrotted (it doesn't apply at all anymore). Please rebase and attach a new patch. Also, please make sure that your patch follows the guidelines below so that all the necessary commit information is present within it.
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Also, bonus points if you post a link to a successful Try run with the patch! (If you don't, that's fine. I run patches myself before landing if I can't see that they already were)
Keywords: checkin-needed
Missed some OS X specific stuff so this should fix that. As for the android build failures, for some reason they don't seem to have MOZ_ALWAYS_INLINE defined.
Attachment #658748 - Attachment is obsolete: true
Looks good on Try, thanks for sticking with it! I'll check this in today unless someone else beats me to it.
Keywords: checkin-needed
This might have been part of a shutdown timeout (see this log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=15029553&tree=Mozilla-Inbound&full=1> for an example).

I backed it out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e4864101e1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a52b3e3632d5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 658804 [details] [diff] [review]
Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

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

::: gfx/thebes/gfxColor.h
@@ +97,5 @@
>   *
>   * equivalent to (((c)*(a))/255)
>   */
> +PRUint8 MOZ_ALWAYS_INLINE gfxPreMultiply(PRUint8 c, PRUint8 a) {
> +    return GFX_DIVIDE_BY_255((c)*(a));

Would have been nice to get rid of the stray parens here, as in

return GFX_DIVIDE_BY_255(c * a);

, now this isn't a macro anymore.
You need to log in before you can comment on or make changes to this bug.