714937 - replace GFX_PACKED_PIXEL with an inline function

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

There's no need for GFX_PACKED_PIXEL to be a macro anymore and having it as an inline function will make things cleaner.

Comment 2

[Luqman Aden [:Luqman]]

Attachment: Replace the GFX_PACKED_PIXEL macro with an inline function.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 4

[Luqman Aden [:Luqman]]

Attachment: Replace the GFX_PACKED_PIXEL macro with an inline function.

Use MOZ_ALWAYS_INLINE.

Comment 5

[Luqman Aden [:Luqman]]

Attachment: Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

Replaced the other macros as well.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 7

[Luqman Aden [:Luqman]]

Attachment: Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

https://tbpl.mozilla.org/?tree=Try&rev=055f068a1a03

Comment 8

[Luqman Aden [:Luqman]]

Attachment: Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

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.

Comment 9

[Luqman Aden [:Luqman]]

Attachment: Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

Builds on both android on os x now.

https://tbpl.mozilla.org/?tree=Try&rev=cd2ac344c57d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks good on Try, thanks for sticking with it! I'll check this in today unless someone else beats me to it.

Comment 11

[(no longer active)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

It wasn't. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52b3e3632d5

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a52b3e3632d5

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.