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, siliconforks.

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