Check images for true alpha transparency #8933
Conversation
Because ImageMagick cannot save images with true alpha transparency in indexed PNG format, we need to check whether an image has true alpha transparency before telling ImageMagick to use the png8 (indexed) format. See https://core.trac.wordpress.org/ticket/63448
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| * We need to avoid forcing indexed format for images with true alpha transparency, | ||
| * because ImageMagick does not support saving an image with true alpha transparency as an indexed PNG. | ||
| */ | ||
| if ( Imagick::COLORSPACE_GRAY === $this->image->getImageColorspace() && ! $is_indexed_png_with_true_alpha_transparency ) { |
There was a problem hiding this comment.
I would remove the colorspace_gray check and move this to L531 as the first check.
Like this:
/*
* We need to avoid forcing indexed format for images with true alpha transparency,
* because ImageMagick does not support saving an image with true alpha transparency as an indexed PNG.
*/
if ( ! $is_indexed_png_with_true_alpha_transparency ) {
$this->image->setOption( 'png:format', 'png8' );
} else {
$this->image->quantizeImage( 256, $this->image->getColorspace(), 0, false, false );
}There was a problem hiding this comment.
Well, if we did that, I think the code would probably work OK - but then there would be no indication of what the purpose of that branch actually is.
As far as I know, the reason that branch is there is specifically to work around an edge case with grayscale images. Otherwise I think there's no reason to have that branch at all? (I believe just calling quantizeImage would work for non-grayscale images.) So I would like to retain some indication of this, either in the code itself (so that setOption would be called only for grayscale images), or at least mention in a comment the reason for having that branch.
There was a problem hiding this comment.
Greyscale condition was only a supposition that setOption was the way to deal with them.
That was originally expected like that. But completely out of chance I found that the real right way to index any (both colour and greyscale) 1-bit alpha depth images was using the setOption
Quantization is just a way to play around with images with a true alpha transparency. I would simply comment this.
I'm not 100% confident yet, but given that the results in size are identical in 99% of the cases, I would say that setOption is some sort of quantization. As I said in my original post, the problem for me was that I could not see when png8 was viable because I could not identify the true alpha transparency. But now that there is a solution, quantization should be done as the last resort.
PS: I would add tests with all the alpha images we played with, checking if they preserve the alpha bits (theoretically with the current code they should fail because png8 is trimming them. With the mod I'm suggesting here, I've run the current 9 assertions and they are obviously working correctly as expected.
Because ImageMagick cannot save images with true alpha transparency in indexed PNG format, we need to check whether an image has true alpha transparency before telling ImageMagick to use the png8 (indexed) format.
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.