[webkit-reviews] review granted: [Bug 116151] Make PNGImageDecoder::rowAvailable auto-vectorizable : [Attachment 201936] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Allan Sandfeld Jensen
<allan.jensen at digia.com>'s request for review:
Bug 116151: Make PNGImageDecoder::rowAvailable auto-vectorizable
https://bugs.webkit.org/show_bug.cgi?id=116151

Attachment 201936: Patch
https://bugs.webkit.org/attachment.cgi?id=201936&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=201936&action=review


Looks good.

I cq- because I would like you to clarify why you handle nonTrivialAlpha in
this weird way.

> Source/WebCore/platform/graphics/Color.h:211
> +inline uint16_t fastDivideBy255(uint16_t value)
> +{
> +    // This is an approximate algorithm for division by 255, but it gives
accurate results for 16bit values.
> +    uint16_t approximation = value >> 8;
> +    uint16_t remainder = value - (approximation * 255) + 1;
> +    return approximation + (remainder >> 8);
> +}

Good idea!

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:415
> +    nonTrivialAlpha |= ~(a ^ 255U); // Arithmetic calculation of (a != 255).


Because the compiler has to set bool to 1 or 0, it will actually add a branch
here.
So using
    nonTrivialAlpha |= (a == 255).

If you change "bool nonTrivialAlpha" to "unsigned char nonTrivialAlphaMask",
you will avoid that.
I don't get why you don't just write 
   nonTrivialAlphaMask |= (a - 0xff)
and check nonTrivialAlpha at the end.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:426
> +    nonTrivialAlpha |= ~(a ^ 255U); // Arithmetic calculation of (a != 255).


Ditto.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:536
> +	       nonTrivialAlpha |= ~(alpha ^ 255U); // Arithmetic calculation of
(alpha != 255).

Ditto.