Skip to content

Conversation

@b1ink0
Copy link
Contributor

@b1ink0 b1ink0 commented Oct 30, 2025

Summary

Fixes #2237

Relevant technical choices

Prevents transparent backgrounds from being lost when converting PNG images to AVIF format on older ImageMagick

TODO:

  • Site health test
  • Add tests

@b1ink0 b1ink0 added this to the webp-uploads n.e.x.t milestone Oct 30, 2025
@b1ink0 b1ink0 added [Type] Bug An existing feature is broken [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Oct 30, 2025
@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 16.49485% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.54%. Comparing base (c9720dc) to head (f8af1e9).
⚠️ Report is 2 commits behind head on trunk.

Files with missing lines Patch % Lines
...ploads/class-webp-uploads-image-editor-imagick.php 0.00% 24 Missing ⚠️
...ealth/imagick-avif-transparency-support/helper.php 0.00% 23 Missing ⚠️
plugins/webp-uploads/helper.php 35.48% 20 Missing ⚠️
plugins/webp-uploads/hooks.php 40.00% 6 Missing ⚠️
...health/imagick-avif-transparency-support/hooks.php 0.00% 5 Missing ⚠️
plugins/webp-uploads/site-health/load.php 0.00% 2 Missing ⚠️
plugins/webp-uploads/load.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #2245      +/-   ##
==========================================
- Coverage   69.21%   68.54%   -0.68%     
==========================================
  Files          90       94       +4     
  Lines        7703     7796      +93     
==========================================
+ Hits         5332     5344      +12     
- Misses       2371     2452      +81     
Flag Coverage Δ
multisite 68.54% <16.49%> (-0.68%) ⬇️
single 35.14% <16.49%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

$imagick = $image_property->getValue( $editor );

if ( $imagick instanceof Imagick ) {
wp_cache_set( 'webp_uploads_image_has_transparency', (bool) $imagick->getImageAlphaChannel(), 'webp-uploads' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this set here in the cache since this function runs first and then later webp_uploads_get_image_output_format() runs to then read the value from the cache? This seems perhaps brittle. Normally setting and getting a cache value would happen in the context of the same function, not across separate functions, right? It feels like there may not be guarantees that the cached value would be set when it is checked for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it works but is definitely brittle. The other approach I can think of is using a global variable or a transient with a short expiration time to store the transparency status and hash of any uploaded file for the current request, and then using it in the webp_uploads_filter_image_editor_output_format to determine the output format. If you have any ideas for a temporary storage that can be used within the same request, I’d appreciate that.
@adamsilverstein if you also have a better idea to handle this case, I’d appreciate that as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are both part of the same request, perhaps you could apply a filter inside the webp_uploads_filter_image_editor_output_format function, then add a filter here to set the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple approaches. I came up with this to solve it:
The most elegant way is to replace the WP_Image_Editor_Imagick class with a custom class WebP_Uploads_Image_Editor_Imagick, which can be used to detect transparency and provide access to the protected variable.

But this will interfere with other plugins if they are trying to use their own custom class. I was thinking of just dynamically extending any custom classes extended from the WP_Image_Editor_Imagick class.

Copy link
Contributor Author

@b1ink0 b1ink0 Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are both part of the same request, perhaps you could apply a filter inside the webp_uploads_filter_image_editor_output_format function, then add a filter here to set the value?

The issue I faced was with the subsize-image generation. The webp_uploads_filter_image_editor_output_format filter is called with an empty filename, so with this approach the sub-size images were getting generated as AVIF.

Because of that, the only way to check the current processing image calling the output-format filter is to access the $image of the image-editor instance that is processing it. But since the core does not expose the image-editor instance through globals or filters, there seems to be no way to determine the currently processed image. To solve this, the only idea I could come up with was using our own custom Image Editor class.

I have implemented the mentioned approach in this commit 2247b61.

cc: @adamsilverstein

Comment on lines 1004 to 1009
$reflection = new ReflectionClass( $editor );
$image_property = $reflection->getProperty( 'image' );
if ( PHP_VERSION_ID < 80100 ) {
$image_property->setAccessible( true );
}
$imagick = $image_property->getValue( $editor );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad that the image property is protected. Note how in the Image Placeholder plugin it actually extends WP_Image_Editor_Imagick with a Dominant_Color_Image_Editor_Imagick which it then uses. This is problematic though since multiple plugins can't each register their own editor classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try to create an anonymous class and added a method to expose the image property, but the Reflection API seemed better.

function webp_uploads_imagick_avif_transparency_supported(): bool {
if ( extension_loaded( 'imagick' ) && class_exists( 'Imagick' ) ) {
$imagick_version = Imagick::getVersion();
if ( (bool) preg_match( '/((?:[0-9]+\\.?)+)/', $imagick_version['versionString'], $matches ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( (bool) preg_match( '/((?:[0-9]+\\.?)+)/', $imagick_version['versionString'], $matches ) ) {
if ( (bool) preg_match( '/^\d+(?:\.\d+)+$/', $imagick_version['versionString'], $matches ) ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated regex to /\d+(?:\.\d+)+(?:-\d+)?/ for handling version string like this:

ImageMagick 7.1.1-15 Q16 aarch64 98eceff6a:20230729 https://imagemagick.org

Copy link
Contributor Author

@b1ink0 b1ink0 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to move this Site Health test to the Modern Image Formats plugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just seeing this comment. I think it makes sense to move all Site Health tests for images to the plugin. See #1781

@b1ink0 b1ink0 marked this pull request as ready for review December 26, 2025 13:05
@b1ink0 b1ink0 requested a review from felixarntz as a code owner December 26, 2025 13:05
@github-actions
Copy link

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @johnmagbag1995, @wes-davis.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: johnmagbag1995, wes-davis.

Co-authored-by: b1ink0 <b1ink0@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: paulgibbs <djpaul@git.wordpress.org>
Co-authored-by: mindctrl <mindctrl@git.wordpress.org>

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

* @since n.e.x.t
*/

// @codeCoverageIgnoreStart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this needs to be ignored for code coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, this section gets flagged by Codecov as the constant is always defined in the test environment, and as there is no way to undefine or modify a const value in PHP, we have to ignore it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. However, this could be made slightly more concise:

if ( ! defined( 'ABSPATH' ) ) {
	exit; // @codeCoverageIgnore
}

This could be done at once for all files in the repo, since they all have it now, for example:

// @codeCoverageIgnoreStart
if ( ! defined( 'ABSPATH' ) ) {
exit; // Exit if accessed directly.
}
// @codeCoverageIgnoreEnd

* has been compiled against ImageMagick version 6.4.0 or newer.
*/
if ( is_callable( array( $this->image, 'getImageAlphaChannel' ) ) ) {
if ( ! $this->image->getImageAlphaChannel() ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you should explicitly check against Imagick::ALPHACHANNEL_UNDEFINED. getImageAlphaChannel returned an int initially, a boolean later.

* has been compiled against ImageMagick version 6.4.0 or newer.
*/
if ( is_callable( array( $this->image, 'getImageAlphaChannel' ) ) ) {
if ( ! $this->image->getImageAlphaChannel() ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked that this works correctly with PNG, WebP and AVIF uploads with transparencies?

}

// Walk through the pixels and look transparent pixels.
$w = $this->image->getImageWidth();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this strikes me as very heavy and deserving of caching at the very least to avoid repeating.

I would also be fine saying Imagick 6.4 is required for transparency support :)

$avif_support = webp_uploads_mime_type_supported( 'image/avif' );

if ( $avif_support && webp_uploads_check_image_transparency( $filename ) ) {
$avif_support = false;
Copy link
Member

@adamsilverstein adamsilverstein Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AVIF should still be supported in Imagick is new enough, right? this line sets it as false if the image has a transparency without regard to the Imagick version.

misread that call ^^

@adamsilverstein
Copy link
Member

@b1ink0 - overall this looks very good!

I wonder if this belongs in core directly: does this impact AVIF with transparency uploads in core (without the upload transform from PNG->AVIF)? If so this probably belongs in core directly since we aim to support AVIF as completely as possible. You probably noticed the existing PNG transparency handling in core, we could add similar handling for AVIFs.

@b1ink0
Copy link
Contributor Author

b1ink0 commented Jan 12, 2026

@adamsilverstein I was just thinking all this complex logic is just so that if an image does not have transparency and the site has an AVIF transparency unsupported version of ImageMagick, we allow the AVIF conversion. The simplest way could be to turn off AVIF conversion completely, even for non-transparent images.

@adamsilverstein
Copy link
Member

@adamsilverstein I was just thinking all this complex logic is just so that if an image does not have transparency and the site has an AVIF transparency unsupported version of ImageMagick, we allow the AVIF conversion. The simplest way could be to turn off AVIF conversion completely, even for non-transparent images.

Thats fine for the plugin! Happy to see this fix land here. I'm just thinking we may also be able to introduce a more general fix in core to handle this case. I will check to see if we have an existing ticket for this.

…ilename parameter by defaulting it to null

Co-authored-by: Weston Ruter <westonruter@google.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment on lines +27 to +39
public static $current_instance;

/**
* Load the image and set the current instance.
*
* @since n.e.x.t
*
* @return WP_Error|true True on success, WP_Error on failure.
*/
public function load() {
$result = parent::load();
if ( ! is_wp_error( $result ) ) {
self::$current_instance = $this;
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a static property to track the current instance creates a concurrency issue. In environments with concurrent requests or when processing multiple images in sequence, this can cause the static property to be overwritten, leading to incorrect behavior where transparency checks use the wrong image file. Consider alternative approaches such as passing the editor instance as a parameter or using instance-specific data.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, this isn't a Node server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transparent backgrounds lost when converting to AVIF under certain conditions

3 participants