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

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied 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 201825: Patch
https://bugs.webkit.org/attachment.cgi?id=201825&action=review

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


This looks like a step in the right direction. I have a couple of comments:

> Source/WebCore/ChangeLog:13
> +	   Together with automatic vectorization by the compiler this provides
slightly
> +	   over 2x speed-up on PNGImageDecoder::rowAvailable and shaves off
12-25% on
> +	   PNG decoding in general.

This is rather unimpressive. Vectorized premultiplication is usually > 4 times
faster (vector gain + preload gain + math gain).
Are you sure the compiler is not fucking up somewhere?

> Source/WTF/wtf/MathExtras.h:463
> +inline unsigned char 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);
> +}

I think that does not belong to MathExtras but you are not the first one to
abuse this header. :)

Let's keep the typing consistent: unsigned char -> uint16_t.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:408
> +static inline void setPixelRGB(ImageFrame::PixelData* dest, png_bytep pixel)

> +{
> +    *dest = 0xFF000000 | pixel[0] << 16 | pixel[1] << 8 | pixel[2];
> +}

You should check but I am pretty sure the compiler will do a bad job at
vectorizing this.

If it is the case, I have written optimized version for Qt that you are free to
steal.

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

Let's make this cleaner.

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

Let's make this cleaner.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:527
> +    unsigned char nonTrivialAlpha = 0;

I would either keep this as a boolean or give it a proper name for a mask.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:535
> +	       unsigned alpha = hasAlpha ? pixel[3] : 255;
> +	       buffer.setRGBA(address++, pixel[0], pixel[1], pixel[2], alpha);
> +	       nonTrivialAlpha |= alpha ^ 255U;

No need to move the "hasAlpha" test out of the loop and use setPixelRGB().