366727 - Standardize Cairo Pixel construction and clean the PNG decoder a little

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

Attachment: V1: Patch to implement NS_CAIRO_PIXEL and to apply it to the PNG decoder

As discussed in bug 331298, a macro in nsColor to help in construction of valid premultiplied ARGB will help to prevent issues in the different image decoders (see bug 360000 and others).

#define NS_CAIRO_PIXEL(a,r,g,b)                                         \
    ((a) == 0x00) ? 0x00000000 :                                        \
    ((a) == 0xFF) ? ((0xFF << 24) | ((r) << 16) | ((g) << 8) | (b))     \
                  : ((a) << 24) |                                       \
                    (NS_PREMULTIPLY(r,a) << 16) |                       \
                    (NS_PREMULTIPLY(g,a) << 8) |                        \
                    (NS_PREMULTIPLY(b,a))

This macro performs the trick, making sure that for zero alpha and opaque items the complex calculations are not performed.
Furthermore by doing this as a macro, this makes the compiler when the a is constant to compile the specific path.
So NS_CAIRO_PIXEL(0,r,g,b) compiles to 0,
and NS_CAIRO_PIXEL(0xFF,r,g,b) compiles to 0xFF000000 | r<<16 | g<<6 | b

This all prevents the need for copying the complex code into the different image decoders.

See the attached patch where the ugly premultiplication stuff is replaced by NS_CAIRO_PIXEL in the PNG decoder.

This even results in some improvement in the number of instructions per row loop:
                OLD     NEW
PNGDecoder:
Case 1:         18      13      
Case 2:         25      17      Old: one stack variable reference
Case 3:         58      45      Old: four stack variable references. New: One stack variable ref

Comment 1

[Stuart Parmenter]

Comment on attachment 251215 [details] [diff] [review]
V1: Patch to implement NS_CAIRO_PIXEL and to apply it to the PNG decoder

this should probably go in gfx/thebes/public/gfxColor.h as we hope to get rid of everything in gfx/public and gfx/src some day...

Comment 2

[Alfred Kayser]

Attachment: V2: add the macro's to gfxColor.h

Moved the macro's to gfxColor.h

Also added better comments for each of the macros, whilst also used 'GFX_' as prefix.

Comment 3

[Alfred Kayser]

Attachment: Testcases for PNG alpha transparancy

Comment 4

[Alfred Kayser]

Comment on attachment 251368 [details] [diff] [review]
V2: add the macro's to gfxColor.h

Vlad, could you also check if this is ok?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 251368 [details] [diff] [review]
V2: add the macro's to gfxColor.h

If stuart's ok with the imagelib changes, it looks fine to me -- note that I had problems with the fast divide by 255 macro a while back, can't remember with which compiler.  Just putting that note here in case we start seeing any weirdness.  (Colors were looking corrupted.)

Comment 6

[Alfred Kayser]

Who can do the checkin for me?

Comment 7

[Nickolay_Ponomarev]

gfx/thebes/public/gfxColor.h                   1.12
modules/libpr0n/decoders/png/Makefile.in       1.19
modules/libpr0n/decoders/png/nsPNGDecoder.cpp  1.56

Comment 8

[Nickolay_Ponomarev]

Is this something that can be unit tested (via reftest perhaps?)

Comment 9

[Alfred Kayser]

Thanks for the checkin.

And indeed attachment #3 [details] [diff] [review] is good material for automated testing, but it seems more something for Mochitest.