Limit Quantization to Indexed images

[SirLouen]

@siliconforks great analysis, you have saved me a good time of research

But if we look at the original Bug, the problem is that the Indexed types images were being converted into Truecolor type. So the @adamsilverstein idea was great but was not taking in consideration, that there are images within the 8-bit depth color range that are actually Truecolor.

AFAIK, the issue is that ImageMagick doesn't provide a method to identify if an image is Indexed or if it's Truecolor, so the sole hint we can find within the Imagick object is the fact that the image has 256 colors or less.

So I think that the @elvismdev idea was also on the right direction, but knowing this, I believe that it could be slightly optimized and overall, the whole algorithm, to apply Quantization for images with lower than 256 colors and still get a result on compression, and to ignore any image that is not Indexed but still have a color depth of 8+ bits.

Trac ticket: https://core.trac.wordpress.org/ticket/63448

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props sirlouen, adamsilverstein, siliconforks, nosilver4u.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[siliconforks]

AFAIK, the issue is that ImageMagick doesn't provide a method to identify if an image is Indexed or if it's Truecolor, so the sole hint we can find within the Imagick object is the fact that the image has 256 colors or less.

There probably is a method to check whether an image is indexed or not. It looks like identifyImage might work:

https://www.php.net/manual/en/imagick.identifyimage.php

But I'm not sure if that's really what we want here. Possibly just checking the number of colors is sufficient?

So I think that the @elvismdev idea was also on the right direction, but knowing this, I believe that it could be slightly optimized and overall, the whole algorithm, to apply Quantization for images with lower than 256 colors and still get a result on compression, and to ignore any image that is not Indexed but still have a color depth of 8+ bits.

That sounds basically right - but isn't this PR sort of doing the opposite of that? Is this check for 256 <= $max_colors supposed to be $max_colors <= 256?

wordpress-develop/src/wp-includes/class-wp-image-editor-imagick.php

Line 500 in e886270

if ( 0 < $indexed_pixel_depth && 8 >= $indexed_pixel_depth && 256 <= $max_colors ) {

Even if that is fixed I'm still not sure the logic entirely makes sense here. Possibly what we want to look at is the number of colors in the image before resizing, rather than after?

It may be simpler to just revert https://core.trac.wordpress.org/changeset/59589 for now and then try to get a revised version of it in a future release because I think it's going to need more extensive changes.

[SirLouen]

@siliconforks this is the problem: identifyImage is identifying "Indexed" images as TrueColor and colorSpace as sRGB (check for example cloudflare-status.png from the unit-tests

About the code, you are right, these fricking Yoda Conditions mess me up constantly, it's the other way around. It's funny because the 5 testing images all happen to have exactly, 256 colors, hence the equals part was passing. In fact, I'm not sure, but I feel that all the 5 images are basically identical in properties, so basically just one of them would do the same stunt and the rest seem to be filler.

It may be simpler to just revert https://core.trac.wordpress.org/changeset/59589 for now and then try to get a revised version of it in a future release because I think it's going to need more extensive changes.

The thing is that Indexed images grow bigger (much bigger, like X2 in numerous instances). So imagine how playful can be to have 100K images occupying 2X when the issue was already solved. Not taking action now in any direction is harming websites.

Even if that is fixed I'm still not sure the logic entirely makes sense here. Possibly what we want to look at is the number of colors in the image before resizing, rather than after?

Resizing doesn't seem to be the problem. The problem is the Quantization. The number of colors is just the only trustworthy way I've researched to separate between Indexed and non Indexed images. And that is the exact moment where it can be checked.

So lets jump into the bull and take it by the horns: We could always get to a perfectly clean solution with maximum optimization parameters, logic and well-being, but while anyone practical decides to jump in the bull and give a better solution, my result solves the increased size on Indexed images and solves the trouble with degradation with colors in one shot.

[siliconforks]

About the code, you are right, these fricking Yoda Conditions mess me up constantly, it's the other way around. It's funny because the 5 testing images all happen to have exactly, 256 colors, hence the equals part was passing. In fact, I'm not sure, but I feel that all the 5 images are basically identical in properties, so basically just one of them would do the same stunt and the rest seem to be filler.

I don't think there's any need to use a Yoda condition here. According to the WordPress PHP Coding Standards: "Yoda conditions for <, >, <= or >= are significantly more difficult to read and are best avoided." (Personally, I would prefer it to be written as $max_colors <= 256.)

However, I don't think that's sufficient to fix the PR - it's still calling getImageDepth() and that's being used in the calculation of $max_colors. The call to getImageDepth() is almost always going to return 8, even for an image like https://core.trac.wordpress.org/raw-attachment/ticket/63448/vivid-green-bird-sample-original.png (the only exception seems to be for PNG images with 16 bits per channel, which are rare). So $max_colors is almost always going to be <= 256.

I just don't think getImageDepth() is actually doing anything useful here. It appears it was originally used because of an incorrect assumption of what the return value was, and I think it probably needs to be removed entirely.

Resizing doesn't seem to be the problem. The problem is the Quantization. The number of colors is just the only trustworthy way I've researched to separate between Indexed and non Indexed images. And that is the exact moment where it can be checked.

Isn't the issue in https://core.trac.wordpress.org/ticket/36477 that the number of colors often increases when an image is resized? So you might start with an original image which has 256 colors, but then you resize it and the resized version of it has more than 256 colors, so it no longer can use a palette. Then the size of the file will increase (possibly enormously).

So lets jump into the bull and take it by the horns: We could always get to a perfectly clean solution with maximum optimization parameters, logic and well-being, but while anyone practical decides to jump in the bull and give a better solution, my result solves the increased size on Indexed images and solves the trouble with degradation with colors in one shot.

I'm happy to continue trying to fix this, but it's just that it took 9 years to try to fix https://core.trac.wordpress.org/ticket/36477 and it's still not working correctly, so I don't know how long this is going to take...

[SirLouen]

I don't think there's any need to use a Yoda condition here. According to the WordPress PHP Coding Standards: "Yoda conditions for <, >, <= or >= are significantly more difficult to read and are best avoided." (Personally, I would prefer it to be written as $max_colors <= 256.)

Everyday one learns something new. Happy to read this, I could not understand using Yoda conditions in these comparisons for ages because it broke my logic but in fact, in the Wikipedia article, they suggested that they could be used and were great (which in fact, for a combined and its not bad after all, like in this case, if the original comparisons were:

0 < $indexed_pixel_depth && $indexed_pixel_depth <= 8

Instead of full Yoda codntions, would make a lot of sense to me.

This said, I've also tested against the clean images directly in PHP test file and I've got this:

Type = Palette, which probably could be a great indicator that as you said, identifyImage could be an alternative used in the right place (I think, not 100% sure), but the problem is that once the conversion is done, it's converted into TrueColor, before any other compression filters are applied)

[SirLouen]

Just for information purposes it seems there are more Indexed types
GrayscaleAlpha and PaletteAlpha

[SirLouen]

In my new final version, I took the idea commented by @siliconforks and @elvismdev of using getImageProperty instead of identifyImage Type because it was the most consistent of all to show the PNG Indexed property

But I also found that the option used for Greyscales, was actually the option that did the same as the whole Quantization process (checked the result size, and it's the same). So now, no additional unit-tests are required (because there is actually a Grayscale image among the set)

So with this last version, we have the best of both worlds, full performance, all test passing + non indexed images are not being affected.

Btw, for props don't forget to consider ticket #63338 for helping with testing

[siliconforks]

In my new final version, I took the idea commented by @siliconforks and @elvismdev of using getImageProperty instead of identifyImage Type because it was the most consistent of all to show the PNG Indexed property

OK, I know I suggested using getImageProperty in https://core.trac.wordpress.org/ticket/63448#comment:19 - but keep in mind that I have not extensively tested this and it's possible there might be issues with it (e.g., I'm not sure how well it is supported across different ImageMagick versions).

Other than that, this PR now looks much simpler and cleaner than the original code in https://core.trac.wordpress.org/changeset/59589

[adamsilverstein]

the existing tests in this file use unlink ref, why are you choosing to change this to wp_delete_file?

[adamsilverstein]

reviewing unit tests, nearly all tests use unlink, let's stick to that convention

[adamsilverstein]

Looks great! Requested some small changes.

[nosilver4u]

I did some further tests on old versions of ImageMagick & PHP, as there had been concerns regarding the use of getImageProperty. I went all the way back to Ubuntu 16.04 with PHP 7 and ImageMagick 6.8.9, and the color type reported by getImageProperty( 'png:IHDR.color-type-orig' ) is consistent to that stored in the IHDR chunk.

On the other hand getImageDepth (on 6.8.9 and 6.9.7) always reports a bit depth of 8 for images with depths of 1, 2, and 4. So it'll be good to have this changed to be more accurate and consistent all around!

[nosilver4u]

I just noticed that the quantizeImage() method was removed completely in favor of the 'png8' method of palette reduction. Though none of the methods in ImageMagick are perfect, quantizeImage() produces the best quality across various images.

Also, from what I have in the code for EWWW IO, I believe some versions of IM change gray indexed images to grayscale, which was why we had the conditional to also apply the png8 method for those. If needed, I can try to do some comparisons on some of my test images to verify that.

[siliconforks]

I just noticed that the quantizeImage() method was removed completely in favor of the 'png8' method of palette reduction. Though none of the methods in ImageMagick are perfect, quantizeImage() produces the best quality across various images.

Also, from what I have in the code for EWWW IO, I believe some versions of IM change gray indexed images to grayscale, which was why we had the conditional to also apply the png8 method for those. If needed, I can try to do some comparisons on some of my test images to verify that.

The png8 format is still being applied to gray indexed images, it's just being applied unconditionally now (so it is applied to both gray and color indexed images).

For the quantizeImage method - I did not notice a difference between these two cases:

1. Calling quantizeImage and then converting to png8
2. Skipping the quantizeImage call and just converting to png8 directly

Are there cases where it makes a difference?

[nosilver4u]

The png8 format is still being applied to gray indexed images, it's just being applied unconditionally now (so it is applied to both gray and color indexed images).

For the quantizeImage method - I did not notice a difference between these two cases:

1. Calling quantizeImage and then converting to png8
2. Skipping the quantizeImage call and just converting to png8 directly

Are there cases where it makes a difference?

No, I don't think you'd see a difference in those two scenarios. The trick is to NOT set the png8 option unless absolutely necessary, as that applies a sub-par quantization algorithm. I'm in the middle of updating our own implementation to use getImageProperty (instead of direct file access), so I'll go through my test images to see if I can find some good examples.

[nosilver4u]

Okay, I found a couple examples where it stands out the most. Sometimes it isn't so noticeable unless you know what to look for. In particular, the png8 method tends to use harsher edges, so it's mostly problematic when there is transparency in the image.

With png:format = png8:

Only quantizeImage:

This one is much more noticeable, and if I remember correctly, was something I created specifically to illustrate the issues I had seen other folks posting about.

With png:format = png8:

Only quantizeImage:

Here's the original for oval-or8.png in case you want to add it into the test suite (hoping GitHub doesn't muck with it, should be 25,707 bytes):

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 60246
GitHub commit: 97ed384

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

[adamsilverstein]

Hey @nosilver4u - apologies for crossing paths, I committed this fix before seeing your recent comments. I think what we have is still an improvement, however I'd like to make it as robust as possible by adding your test images to the test suite as well as improving the logic further.

When you have a moment can you please upload these test images to a trac ticket (fine to re-open the existing one), I am concerned as well that GitHub may have messed with your uploads)