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

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Images

[Cosmin Truta]

I noticed that, when exporting PNG images, the alpha channel is included, regardless whether it contains any meaningful transparency or just fully-opaque pixels. In my experience, the overwhelming majority of images available on the internet do not have transparent or semi-transparent pixels.

Removing a fully-opaque alpha channel (when it's safe to do so) will not only enhance the compression ratio, but will also improve the speed. Although a safe alpha channel removal requires a-priori inspection of all the image pixels' alphas, this pays off when the zlib compressor has less work to do, afterwards. Overall, in my tests, there is a 10%-20% speed improvement.

Speaking about speed, there is the issue of running the alpha channel inspection test, only to notice that there actually is some transparency. In this case, this will be an overhead, with no actual savings. However, this should be below 10% in the (statistically very rare) worst case. Images tend to have transparency around edges, while the center pixels are opaque. Because of this, in the wide majority of cases, the alpha test will exit very early, and will cost almost nothing.

In PNG terminology, this operation is called a lossless image reduction of the PNG image type. There are many other possibilities to reduce the image type, and they are mentioned in an article that I wrote long ago, named "A Guide to PNG Optimization".
http://optipng.sourceforge.net/pngtech/optipng.html (see Section 2.1)

[Cosmin Truta]

Created attachment 75963 [details]
Patch

I am attaching the patch, which I tested by exporting images from DeviantArt's Muro application (http://muro.deviantart.com)
There are no new layout tests, as I did not find a test that exports PNG images, and I did not know how to write one. I will greatly appreciate any help.

[Cosmin Truta]

(In reply to comment #1)
> There are no new layout tests, as I did not find a test that exports PNG images, and I did not know how to write one. I will greatly appreciate any help.

Well, I did find a test: fast/canvas/toDataURL-alpha.html. But I can't really test it using ImageDiff (I can only have confirmation that I haven't broken anything), because ImageDiff expands the types of compared images up to RGBA.

There are tools that display the IHDR information (e.g. pngcheck). But I still don't know how to put something like that in layout testing.

[Adam Barth]

Comment on attachment 75963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75963&action=review

> WebCore/ChangeLog:16
> +        No new tests.

We should figure out a way to test this change.

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

This should be on one line.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:71
> +static void preMultipliedBGRAtoRGB(const unsigned char* input,
> +                                   int numberOfPixels,
> +                                   unsigned char* output)

As should this.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:90
> -static void preMultipliedBGRAtoRGBA(const unsigned char* input, int numberOfPixels,
> +static void preMultipliedBGRAtoRGBA(const unsigned char* input,
> +                                    int numberOfPixels,

This should stay on one line.  WebKit doesn't have an 80 col limit.

[Eric Seidel (no email)]

Comment on attachment 75963 [details]
Patch

r- per adam's comments.

[Cosmin Truta]

As the bug title suggests, this is done for the Skia-based platforms only.
I opened the bug 51120 to address the other platforms.

[Cosmin Truta]

There was a t(In reply to comment #5)
> As the bug title suggests, this is done for the Skia-based platforms only.
> I opened the bug 51120 to address the other platforms.

There is a typo in this message. (Oops!)

I opened the ** bug 51220 ** to address the other platforms.

[Cosmin Truta]

Created attachment 76848 [details]
Patch

Changed formatting to comply with reviewer's comments, and added a layout test.

[David Levin]

Comment on attachment 76848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76848&action=review

One bug to fix and a few things to consider.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:55
> +        inputBitmap.setPixels(const_cast<unsigned char*>(input));

s/input/row/

Would be nice to improve the test to catch this bug.  

Seems like accidentally using the first row again and again could be an easier mistake to make.

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

consider calling "input" "row" so that the code stays similar looking in these three places.

> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:85
> +static void preMultipliedBGRAtoRGBA(const unsigned char* input, int numberOfPixels, unsigned char* output)

Consider changing "input" to "row" (same as above).

[Cosmin Truta]

Created attachment 76932 [details]
Patch

(In reply to comment #8)
> One bug to fix and a few things to consider. [...]

Nice catch!
I fixed it, and followed all suggestions, and I even improved the naming a little bit.

[Cosmin Truta]

Created attachment 76934 [details]
Patch

Fixed a typo in a comment.

[Peter Kasting]

Comment on attachment 76934 [details]
Patch

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.

[David Levin]

Comment on attachment 76934 [details]
Patch

This patch won't apply to tip of tree (plus there were some comments from Peter). fwiw, I'd suggest that you and Noel talk to each other.

[Cosmin Truta]

(In reply to comment #12)
> This patch won't apply to tip of tree (plus there were some comments from Peter). fwiw, I'd suggest that you and Noel talk to each other.

I noticed. I already prepared a patch that applies to the newer ToT and addresses some of Peter's comments.

I will also write to Noel Gordon.