Prevent transparency loss in AVIF by falling back to WebP on older ImageMagick

[b1ink0]

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

[codecov]

Codecov Report

❌ Patch coverage is 78.28947% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.18%. Comparing base (4aca2bc) to head (3190c5b).

Files with missing lines	Patch %	Lines
...ploads/class-webp-uploads-image-editor-imagick.php	62.00%	19 Missing ⚠️
plugins/webp-uploads/hooks.php	75.00%	6 Missing ⚠️
plugins/webp-uploads/helper.php	91.30%	4 Missing ⚠️
plugins/webp-uploads/site-health/load.php	0.00%	2 Missing ⚠️
plugins/webp-uploads/load.php	0.00%	1 Missing ⚠️
...health/imagick-avif-transparency-support/hooks.php	80.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            trunk    #2245    +/-   ##
========================================
  Coverage   69.17%   69.18%            
========================================
  Files          90       94     +4     
  Lines        7708     7856   +148     
========================================
+ Hits         5332     5435   +103     
- Misses       2376     2421    +45     

Flag	Coverage Δ
multisite	69.18% <78.28%> (+<0.01%)	⬆️
single	34.71% <15.13%> (-0.68%)	⬇️

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.

[westonruter]

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.

[b1ink0]

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.

[adamsilverstein]

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?

[b1ink0]

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.

[b1ink0]

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

[westonruter]

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.

[b1ink0]

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

[westonruter]

Suggested change

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

[b1ink0]

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

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

[b1ink0]

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

[westonruter]

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

[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.

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: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: westonruter <westonruter@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.

[adamsilverstein]

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

[b1ink0]

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.

[westonruter]

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:

performance/plugins/performance-lab/load.php

Lines 18 to 22 in b4f9898

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

[adamsilverstein]

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

[adamsilverstein]

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 :)

[b1ink0]

Done in fece741.

[adamsilverstein]

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]

@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]

@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]

@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.

[b1ink0]

I have been trying to write tests to improve code coverage, but it seems that imagick::queryFormats returns an empty array when running tests, causing all the tests to be skipped. This PR specifically needs to test Imagick behavior, so this seems like a major blocker.

I was able to run the tests correctly by modifying the test-php command for webp-uploads from using tests-cli to tests-wordpress. We have both of these containers running anyway. The issue is that tests-cli is an Alpine-based container with a minimal setup, which excludes proper Imagick support.

Modified command:

-"test-php:webp-uploads": "wp-env run tests-cli --env-cwd=/var/www/html/wp-content/plugins/performance composer test:webp-uploads",
+"test-php:webp-uploads": "wp-env run tests-wordpress --env-cwd=/var/www/html/wp-content/plugins/performance composer test:webp-uploads",

So my question is: was there any reason to use tests-cli instead of tests-wordpress for testing that I am missing?

Should we at least use the tests-wordpress container for the webp-uploads plugin tests?

cc: @westonruter, @adamsilverstein

[westonruter]

@b1ink0 That's a good question. It would seem to make sense that the tests should be run using tests-wordpress so that the tests are running in an actual environment configured for serving WordPress and not just WP-CLI, which is what I understand tests-cli is for. I'd say go for it.

[b1ink0]

Now with tests-wordpress, I am able to get a lot of tests working, but the tests related to conversion to AVIF have issues.

Next issue: we can’t really convert to the AVIF format with Imagick even in the tests-wordpress environment. The error below occurs:

no encode delegate for this image format `AVIF' @ error/constitute.c/WriteImage/1412

The queryFormats check passes for "AVIF" because the decoder for AVIF is present.

I did some digging and found that this is an upstream WordPress Docker image issue. In the Docker template file, libmagickwand-dev is installed here:
https://github.com/docker-library/wordpress/blob/4eb711d9183e4e0e60df209be1fb41e5389a573d/Dockerfile.template#L66

But in the next step, the Dockerfile removes the recommended libraries here:
https://github.com/docker-library/wordpress/blob/4eb711d9183e4e0e60df209be1fb41e5389a573d/Dockerfile.template#L124

Now, libmagickwand-dev has a dependency on libmagickcore-7.q16-dev
(ref: https://packages.debian.org/trixie/libmagickwand-7.q16-dev)

That depends on libmagickcore-7.q16-10
(ref: https://packages.debian.org/trixie/libmagickcore-7.q16-dev)

Which depends on libheif1
(ref: https://packages.debian.org/trixie/libmagickcore-7.q16-10 whihc has dep (libheif-plugin-dev1d or libheif-plugin-aomdec))

libheif1 depends on libheif-plugin-dav1d or libheif-plugin-aomdec
(ref: https://packages.debian.org/trixie/libheif1)

Here, libheif-plugin-dav1d is the decoder and is a hard dependency, but libheif-plugin-aomdec is a recommended dependency (ref: https://packages.debian.org/trixie/libheif1). This recommended dependency is the encoder, and it is not included in the WordPress Docker image because recommended packages are removed, causing us to lose the encoder.

Now we have a few options:

1. Do not test the conversion to AVIF with Imagick.
2. I was able to get conversion to AVIF working by manually installing libheif-plugin-aomenc running apt-get install -y libheif-plugin-aomenc in the tests-wordpress docker container. We could override some things in wp-env to install this, but I’m not sure how that can be done.

For now, I am removing the AVIF conversion–related tests, as this PR has been pending for a long time.

[westonruter]

Another review from Gemini:

Based on the diff analysis of the performance repository, here is the review of the changes.

Review Summary

The changes address an issue where older versions of ImageMagick lose transparency when converting images to AVIF. The solution involves:

1. Detecting Transparency: A new WebP_Uploads_Image_Editor_Imagick class extends the base Imagick editor to add a has_transparency() method.
2. Fallback Mechanism: If AVIF is the configured output format but the Imagick version is too old to support AVIF transparency (< 7.0.25) and the image has transparency, the output format falls back to WebP.
3. Site Health Check: A new Site Health check informs users if their ImageMagick version is outdated regarding AVIF transparency support.

The implementation is robust, including a dynamic class alias to handle potential subclassing of WP_Image_Editor_Imagick by other plugins, although there is a potential infinite loop or recursion risk in webp_uploads_check_image_transparency that should be addressed.

Detailed Comments

plugins/webp-uploads/helper.php

webp_uploads_check_image_transparency( ?string $filename ): bool

• Potential Recursion/Infinite Loop:
  Inside this function, you call wp_get_image_editor( $filename, ... ).
  wp_get_image_editor calls _wp_image_editor_choose, which triggers the wp_image_editors filter.
  Your hooks.php adds webp_uploads_set_image_editors to this filter.
  Inside webp_uploads_set_image_editors, you call webp_uploads_get_image_output_format() and webp_uploads_imagick_avif_transparency_supported().

  Currently, webp_uploads_set_image_editors does not call webp_uploads_check_image_transparency, so a direct infinite loop is avoided.

  However, inside webp_uploads_check_image_transparency, you do:

  $editor = wp_get_image_editor( ... );
  if ( ... $editor instanceof WebP_Uploads_Image_Editor_Imagick ) { ... }

  If webp_uploads_set_image_editors logic changes or if another plugin intervenes, this could become fragile.

  More importantly, wp_get_image_editor creates a new instance of the editor. WebP_Uploads_Image_Editor_Imagick::load() calls parent::load(). If parent::load() does heavy lifting (loading the image into memory), doing this just to check transparency might be expensive if not careful.

  The function tries to optimize by checking WebP_Uploads_Image_Editor_Imagick::$current_instance when $filename is null.

  Issue: When $filename is passed (which is the case in webp_uploads_get_upload_image_mime_transforms), you are creating a new editor instance.

  $editor = wp_get_image_editor( $filename, ... );

  This loads the image from disk again.

  If webp_uploads_get_upload_image_mime_transforms is called multiple times (e.g. for every subsize generation), we might be re-loading the image into Imagick multiple times just to check transparency.

  webp_uploads_check_image_transparency does have a static $processed_images cache, which mitigates this significantly.

  Suggestion: Ensure that the static cache key $filename matches exactly what is passed.

• Use of wp_image_editor_supports:

  if ( ! class_exists( 'WebP_Uploads_Image_Editor_Imagick' ) ) {
      // Calls filter `wp_image_editors` internally which makes sure `webp_uploads_set_image_editors` is called.
      wp_image_editor_supports();
  }

  This is a clever (if slightly hacky) way to ensure your class is loaded. It relies on wp_image_editor_supports calling _wp_image_editor_choose which calls the filter.

plugins/webp-uploads/class-webp-uploads-image-editor-imagick.php

has_transparency()

• Logic: The logic checks for Alpha Channel existence, then tries getImageChannelMean/getImageChannelRange (fast), and falls back to pixel iteration (slow).

• Pixel Iteration: The fallback getImagePixelColor in a nested loop is extremely slow for large images.

  for ( $x = 0; $x < $w; $x++ ) {
      for ( $y = 0; $y < $h; $y++ ) { ... }
  }

  If getImageChannelMean is not available (rare in modern setups, but possible), this could time out.
  However, given the constraint (Imagick < 7.0.25), we are dealing with "older" versions, so this fallback might actually be hit.

  Suggestion: Consider if there's a way to avoid the pixel-by-pixel loop, or limit it to a sampling if absolute precision isn't required (though missing a single transparent pixel usually counts as "has transparency"). Alternatively, exportImagePixels might be faster than method calls per pixel.

• Type Hinting:

  public function has_transparency()

  Missing return type hint bool|WP_Error in the method signature, though the docblock has it. Since this is a new method not in the parent interface, adding the type hint has_transparency(): mixed (or specific) would be good if PHP 7.4+ is guaranteed (WP Core requires 7.2+, so maybe not).

plugins/webp-uploads/hooks.php

webp_uploads_set_image_editors

• Class Aliasing:

  class_alias( $editors[0], 'WebP_Uploads_Image_Editor_Imagick_Base' );

  This handles the case where another plugin has swapped the default Imagick editor. Good.

  Nit:

  ! ( WP_Image_Editor_Imagick::class === $editors[0] || is_subclass_of( $editors[0], WP_Image_Editor_Imagick::class ) )

  This check ensures we only try to extend if the current editor is indeed Imagick-based.

General

• Code Coverage: The @codeCoverageIgnoreStart blocks for ABSPATH checks are standard.
• Tests: The added tests in plugins/webp-uploads/tests/test-transparency.php cover the new functionality well, including mocking/skipping based on environment capabilities.

Verdict:
The logic is sound. The transparency check is cached, minimizing performance impact. The fallback to WebP for transparency on older Imagick versions is a user-friendly fix.

One specific concern:
In plugins/webp-uploads/helper.php:

if ( 'avif' === $output_format && ( ! webp_uploads_mime_type_supported( 'image/avif' ) || webp_uploads_check_image_transparency( $filename ) ) ) {
    $output_format = 'webp';
}

If webp_uploads_check_image_transparency returns true (has transparency), you force webp.
But wait, if the user has a newer Imagick that supports AVIF transparency, webp_uploads_check_image_transparency returns false early:

if ( 'avif' !== webp_uploads_get_image_output_format() || webp_uploads_imagick_avif_transparency_supported() ) {
    return false;
}

So webp_uploads_check_image_transparency only returns true if:

1. AVIF is enabled.
2. Transparency support is missing.
3. The image has transparency.

This logic holds up.

Approved.

[b1ink0]

Ohh now old tests are failing due to use of the tests-wordpress 🫠.

[b1ink0]

I created an issue for the AVIF encoding issue here yesterday: docker-library/wordpress#996.

It looks like they have already started working on it docker-library/wordpress#997. Hopefully, it gets merged soon.

cc: @westonruter

[b1ink0]

The tests are now passing for PHP 8.1+ and PHP 7.2+ versions due to the upstream fix, but PHP 8.1 and PHP 7.2 will not be fixed because support for this Docker image has been dropped (see docker-library/wordpress#992.

We have the option to skip these failing tests for this specific version, or we can remove PHP 8.1 from testing.

For now, I will skip these failing tests on these version.

[westonruter]

If these aren't set, then should it not fall back to the fallback below as well? When would these not be set?

[b1ink0]

Normally, these won’t fail and will most likely throw an exception if something fails in rare cases.

[b1ink0]

While adding the fallback, I ran PHPStan, and it seems the PHPStan rules have been updated, which are now flagging these. As of now, these functions should exist in ImageMagick version 6.4.0 or newer, as they were added in 2008 (around 18 years ago). In that case, should we keep these checks?

------ -------------------------------------------------------------------------------------------------------- 
  Line   plugins/webp-uploads/class-webp-uploads-image-editor-imagick.php                                        
 ------ -------------------------------------------------------------------------------------------------------- 
  :102   Call to function method_exists() with Imagick and 'getImageAlphaChannel' will always evaluate to true.  
  :110   Call to function method_exists() with Imagick and 'getImageChannelMean' will always evaluate to true.   
  :110   Call to function method_exists() with Imagick and 'getImageChannelRange' will always evaluate to true.  
 ------ -------------------------------------------------------------------------------------------------------- 

[westonruter]

Ah, yes. In that case, and since the try/catch will handle the case where they don't exist, then that seems good to remove the checks.

[b1ink0]

Done in 3190c5b.

[b1ink0]

Response to Gemini feedback @westonruter please forward this to Gemini:

However, inside webp_uploads_check_image_transparency, you do:
$editor = wp_get_image_editor( ... );
if ( ... $editor instanceof WebP_Uploads_Image_Editor_Imagick ) { ... }
If webp_uploads_set_image_editors logic changes or if another plugin intervenes, this could become fragile.

The webp_uploads_set_image_editors will not change, and I am not sure how another plugin can intervene.

---

More importantly, wp_get_image_editor creates a new instance of the editor. WebP_Uploads_Image_Editor_Imagick::load() calls parent::load(). If parent::load() does heavy lifting (loading the image into memory), doing this just to check transparency might be expensive if not careful.

The function tries to optimize by checking WebP_Uploads_Image_Editor_Imagick::$current_instance when $filename is null.

Issue: When $filename is passed (which is the case in webp_uploads_get_upload_image_mime_transforms), you are creating a new editor instance.

$editor = wp_get_image_editor( $filename, ... );
This loads the image from disk again.

f webp_uploads_get_upload_image_mime_transforms is called multiple times (e.g. for every subsize generation), we might be re-loading the image into Imagick multiple times just to check transparency.

webp_uploads_check_image_transparency does have a static $processed_images cache, which mitigates this significantly.

When the filename is null, it is most likely during sub-size generation. In that case, we use the current instance to get the file name. Since it’s a sub-size, the file name will be the same as the original image file name, which has already been processed.

For sub-sizes, we don’t create a new editor instance because the static property has already cached it, so we just return the result from it.

---

Use of wp_image_editor_supports:

if ( ! class_exists( 'WebP_Uploads_Image_Editor_Imagick' ) ) {
// Calls filter wp_image_editors internally which makes sure webp_uploads_set_image_editors is called.
wp_image_editor_supports();
}
This is a clever (if slightly hacky) way to ensure your class is loaded. It relies on wp_image_editor_supports calling _wp_image_editor_choose which calls the filter.

I would like to highlight that the wp_image_editors filter is almost always called before our code reaches this point. This will only execute in rare cases where the function is called directly or the wp_image_editors filter has not been called, which is still very rare.

Regarding the hacky part, this was the cleanest way to ensure our custom class is loaded. Alternatively, we could call the filter directly here or call the hooked function directly.

---

Pixel Iteration: The fallback getImagePixelColor in a nested loop is extremely slow for large images.

for ( $x = 0; $x < $w; $x++ ) {
for ( $y = 0; $y < $h; $y++ ) { ... }
}
If getImageChannelMean is not available (rare in modern setups, but possible), this could time out.
However, given the constraint (Imagick < 7.0.25), we are dealing with "older" versions, so this fallback might actually be hit.

Suggestion: Consider if there's a way to avoid the pixel-by-pixel loop, or limit it to a sampling if absolute precision isn't required (though missing a single transparent pixel usually counts as "has transparency"). Alternatively, exportImagePixels might be faster than method calls per pixel.

The exportImagePixels method was introduced along with getImageChannelRange and getImageChannelMean, so if those fail, exportImagePixels could also fail. In that case, the fallback could also fail.