Prevent transparency loss in AVIF by falling back to WebP on older ImageMagick #2245
Conversation
b1ink0
added
[Type] Bug
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #2245 +/- ##
==========================================
- Coverage 68.86% 68.31% -0.56%
==========================================
Files 90 92 +2
Lines 7610 7681 +71
==========================================
+ Hits 5241 5247 +6
- Misses 2369 2434 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| $imagick = $image_property->getValue( $editor ); | ||
|
|
||
| if ( $imagick instanceof Imagick ) { | ||
| wp_cache_set( 'webp_uploads_image_has_transparency', (bool) $imagick->getImageAlphaChannel(), 'webp-uploads' ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| $reflection = new ReflectionClass( $editor ); | ||
| $image_property = $reflection->getProperty( 'image' ); | ||
| if ( PHP_VERSION_ID < 80100 ) { | ||
| $image_property->setAccessible( true ); | ||
| } | ||
| $imagick = $image_property->getValue( $editor ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I did try to create an anonymous class and added a method to expose the image property, but the Reflection API seemed better.
plugins/webp-uploads/helper.php
Outdated
| 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 ) ) { |
There was a problem hiding this comment.
| if ( (bool) preg_match( '/((?:[0-9]+\\.?)+)/', $imagick_version['versionString'], $matches ) ) { | |
| if ( (bool) preg_match( '/^\d+(?:\.\d+)+$/', $imagick_version['versionString'], $matches ) ) { |
There was a problem hiding this comment.
Updated regex to /\d+(?:\.\d+)+(?:-\d+)?/ for handling version string like this:
ImageMagick 7.1.1-15 Q16 aarch64 98eceff6a:20230729 https://imagemagick.org
Co-authored-by: Weston Ruter <westonruter@google.com>
There was a problem hiding this comment.
Would it make more sense to move this Site Health test to the Modern Image Formats plugin?
2 participants
Summary
Fixes #2237
Relevant technical choices
Prevents transparent backgrounds from being lost when converting PNG images to AVIF format on older ImageMagick
TODO: