[Webkit-unassigned] [Bug 50714] [Skia] Improve PNG compression by stripping the alpha channel when it's fully opaque

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50714





--- Comment #11 from Peter Kasting <pkasting at google.com>  2010-12-17 18:38:32 PST ---
(From update of attachment 76934)
View in context: https://bugs.webkit.org/attachment.cgi?id=76934&action=review

Seems OK to me, just nits

> WebCore/ChangeLog:14
> +        This method can be further refined by applying the reduction in the presence of
> +        a single transparent color that can be stored in a tRNS chunk.

Nit: This comment would be great in the source code instead of here in the ChangeLog.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:67
> +static void preMultipliedBGRAtoRGB(const unsigned char* inputRow, int pixelsPerRow, unsigned char* outputRow)

Nit: Because it matters that these two functions have identical signatures, I suggest making a typedef for this function's signature and using that both to declare these functions as well as for the type of |rowTransformer| below.

Although, now that I say that, there is also another way, unless I'm misremembering my C++.  You can collapse these two functions into one, but use a template bool to maintain the compile-time speed advantage.  This eliminates duplication for added clarity and might make the call site clearer too:

template<bool copyAlpha> void unPreMultiplyAndConvert(...) {
    ...
    for (...) {
        ...
        if (copyAlpha) {
            outputPixel[3] = SkGetColorA(outputColor);
            outputPixel += 4;
        } else {
            outputPixel += 3;
        }
    }
}

...
unPreMultiplyAndConvert<degenerateAlphaInARGB(...)>(...);  // Not sure if this is legal, might have to de-inline

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:73
> +    unsigned char* outputPixel = outputRow;

Nit: Up to you, but moving the declaration and increment of |outputPixel| into the loop header as well might make things shorter and more clearly-scoped.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:91
> +    unsigned char* outputPixel = outputRow;

Same nit.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:127
> +static bool encodeImpl(const unsigned char* input, const IntSize& size, int bytesPerRow,
>                         Vector<unsigned char>* output)

Nit: Heck, might as well go all the way onto one line (since WK style doesn't care about line length).

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:175
> +                 PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);

Same nit.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.