replace GFX_PACKED_PIXEL with an inline function

RESOLVED FIXED in mozilla18

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: Luqman)

Tracking

unspecified
mozilla18
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
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++]
(Assignee)

Comment 2

6 years ago
Created attachment 639149 [details] [diff] [review]
Replace the GFX_PACKED_PIXEL macro with an inline function.
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-
(Assignee)

Comment 4

6 years ago
Created attachment 639152 [details] [diff] [review]
Replace the GFX_PACKED_PIXEL macro with an inline function.

Use MOZ_ALWAYS_INLINE.
Attachment #639149 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 639158 [details] [diff] [review]
Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

Replaced the other macros as well.
Attachment #639152 - Attachment is obsolete: true
Attachment #639158 - Flags: review?(jmuizelaar)
Attachment #639158 - Flags: review?(jmuizelaar) → review+

Updated

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

Comment 7

6 years ago
Created attachment 658748 [details] [diff] [review]
Replace GFX_PACKED_PIXEL/GFX_PACKED_PIXEL_NO_PREMULTIPLY/GFX_PREMULTIPLY macros

https://tbpl.mozilla.org/?tree=Try&rev=055f068a1a03
Attachment #639158 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 658767 [details] [diff] [review]
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.
Attachment #658748 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 658804 [details] [diff] [review]
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
Attachment #658767 - 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
Last Resolved: 6 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.