-
Notifications
You must be signed in to change notification settings - Fork 142
Prevent transparency loss in AVIF by falling back to WebP on older ImageMagick #2245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
b1ink0
wants to merge
48
commits into
WordPress:trunk
Choose a base branch
from
b1ink0:fix/imagemagick-old-version-avif-transparency-loss
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
ae7f22d
Add support for AVIF transparency checks during image uploads
b1ink0 a1aff66
Improve Imagick version string regex validation
b1ink0 be0817b
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 3fe6954
Add site health checks for Imagick AVIF transparency support
b1ink0 2247b61
Add custom image editor class to help with AVIF transparency detection
b1ink0 1a58d1c
Move Imagick AVIF transparency support site health test
b1ink0 fdf65ec
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 50f2be1
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 f8af1e9
Allow `webp_uploads_get_upload_image_mime_transforms()` to omit the f…
b1ink0 991d7c9
Add explicit return value check for `getImageAlphaChannel` method
b1ink0 fece741
Add transparency check caching to custom image editor class
b1ink0 9020b24
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 435f5ff
Refactor to use filename instead of file hash for transparency check
b1ink0 4da1758
Fix transparency detection to exclude fully opaque pixels
b1ink0 717e768
Fix image editor confilict with Dominant Color Images
b1ink0 c033c44
Dynimacally extend the image editor class
b1ink0 b28885e
Fix parameters order in `is_subclass_of`
b1ink0 6004d18
Simplify conditional checks
b1ink0 d218d0f
Fix transparency detection for fully transparent images
b1ink0 41f94e1
Improve doc comments
b1ink0 28f2583
Skip unnecessary transparency check when image format is already WebP
b1ink0 b421017
Improve doc comments
b1ink0 433ea1c
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 b00d2d0
Add unit tests for AVIF transparency detection
adamsilverstein 28d6020
Enhance transparency detection logic
b1ink0 4cf8302
Improve image editor selection logic
b1ink0 a28415f
Merge pull request #1 from WordPress/fix/imagemagick-old-version-avif…
b1ink0 99a2704
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 92f6528
Ensure helper class is loaded
b1ink0 23f528b
Add filter to transparency check function
b1ink0 06f67f7
Improve test code coverage
b1ink0 4e1ac64
Update test plugin GitHub action to use `tests-wordpress` instead of …
b1ink0 fec8109
Update commands to use `tests-wordpress` instead of `tests-cli`
b1ink0 50e74ad
Update all commands to usee `tests-wordpress`
b1ink0 d8acc81
Fix failing tests
b1ink0 0e0ee09
Fix failing tests on PHP 7.2
b1ink0 23e12d5
Fix failing tests on PHP 8.1
b1ink0 1045006
Finally fix failing tests on PHP 8.1
b1ink0 789906c
Use `method_exists` instead of `is_callable`
b1ink0 8140089
Return null when file property does not exist
b1ink0 2c50ed7
Improve conditional checks
b1ink0 d01f2d6
Narrow down types
b1ink0 315d6c3
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 3190c5b
Use fallback for more failure conditions
b1ink0 f0beb09
Improve test code coverage
b1ink0 346a61e
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 3f55c70
Merge branch 'trunk' into fix/imagemagick-old-version-avif-transparen…
b1ink0 59d39dc
Only consider default channels for mean calculation
b1ink0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
160 changes: 160 additions & 0 deletions
160
plugins/webp-uploads/class-webp-uploads-image-editor-imagick.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| <?php | ||
| /** | ||
| * WordPress Image Editor Class for Image Manipulation through Imagick | ||
| * for transparency detection | ||
| * | ||
| * @package webp-uploads | ||
| * | ||
| * @since n.e.x.t | ||
| */ | ||
|
|
||
| // @codeCoverageIgnoreStart | ||
| if ( ! defined( 'ABSPATH' ) ) { | ||
| exit; // Exit if accessed directly. | ||
| } | ||
| // @codeCoverageIgnoreEnd | ||
|
|
||
| if ( class_exists( 'WebP_Uploads_Image_Editor_Imagick_Base' ) ) { | ||
|
|
||
| /** | ||
| * WordPress Image Editor Class for Image Manipulation through Imagick | ||
| * for transparency detection. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @see WP_Image_Editor | ||
| */ | ||
| class WebP_Uploads_Image_Editor_Imagick extends WebP_Uploads_Image_Editor_Imagick_Base { | ||
| /** | ||
| * The current instance of the image editor. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @var WebP_Uploads_Image_Editor_Imagick|null $current_instance The current instance. | ||
| */ | ||
| public static $current_instance = null; | ||
|
|
||
| /** | ||
| * Stores already checked images for transparency. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @var array<string, bool> Associative array with file paths as keys and transparency detection results as values. | ||
| */ | ||
| private static $checked_images = array(); | ||
|
|
||
| /** | ||
| * 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() { | ||
| // @phpstan-ignore-next-line -- Parent class is created via class_alias at runtime. | ||
| $result = parent::load(); | ||
| if ( ! is_wp_error( $result ) ) { | ||
| self::$current_instance = $this; | ||
| } | ||
| return $result; | ||
| } | ||
|
|
||
| /** | ||
| * Get the file path of the image. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return string|null The file path of the image, or null if not available. | ||
| */ | ||
| public function get_file(): ?string { | ||
| if ( property_exists( $this, 'file' ) && is_string( $this->file ) ) { | ||
| return $this->file; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Looks for transparent pixels in the image. | ||
| * If there are none, it returns false. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return bool|WP_Error True or false based on whether there are transparent pixels, or an error on failure. | ||
| */ | ||
| public function has_transparency() { | ||
| if ( ! property_exists( $this, 'image' ) || ! $this->image instanceof Imagick ) { | ||
| return new WP_Error( 'image_editor_has_transparency_error_no_image', __( 'Transparency detection no image found.', 'webp-uploads' ) ); | ||
| } | ||
|
|
||
| $file_path = $this->get_file(); | ||
| if ( isset( $file_path, self::$checked_images[ $file_path ] ) ) { | ||
| return self::$checked_images[ $file_path ]; | ||
| } | ||
| $transparency = false; | ||
| $use_fallback = false; | ||
|
|
||
| try { | ||
| /* | ||
| * Check if the image has an alpha channel if false, then it can't have transparency so return early. | ||
| * | ||
| * Note that Imagick::getImageAlphaChannel() is only available if Imagick | ||
| * has been compiled against ImageMagick version 6.4.0 or newer. | ||
| */ | ||
| if ( Imagick::ALPHACHANNEL_UNDEFINED === $this->image->getImageAlphaChannel() ) { | ||
| self::$checked_images[ $file_path ] = false; | ||
| return false; | ||
| } | ||
|
|
||
| // Use mean and range to determine if there is any transparency more efficiently. | ||
| $rgb_mean = $this->image->getImageChannelMean( Imagick::CHANNEL_DEFAULT ); | ||
| $alpha_range = $this->image->getImageChannelRange( Imagick::CHANNEL_ALPHA ); | ||
|
|
||
| if ( isset( $rgb_mean['mean'], $alpha_range['maxima'] ) ) { | ||
| $maxima = (int) $alpha_range['maxima']; | ||
| $mean = (int) $rgb_mean['mean']; | ||
|
|
||
| if ( 0 > $maxima || 0 > $mean ) { | ||
| // For invalid values use fallback. | ||
| $use_fallback = true; | ||
| } elseif ( 0 === $maxima && 0 === $mean ) { | ||
| // Alpha channel is all zeros AND no RGB content indicates fully transparent image. | ||
| $transparency = true; | ||
| } elseif ( 0 === $maxima && $mean > 0 ) { | ||
| // Alpha maxima of 0 with RGB content present indicates no real alpha channel exists (hence fully opaque). | ||
| $transparency = false; | ||
| } elseif ( 0 < $maxima && 0 < $mean ) { | ||
| // Non-zero alpha values with RGB content present indicates some transparency. | ||
| $transparency = true; | ||
| } else { | ||
| // For any other case use fallback. | ||
| $use_fallback = true; | ||
| } | ||
| } else { | ||
| $use_fallback = true; | ||
| } | ||
|
|
||
| if ( $use_fallback ) { | ||
| // Fallback to walk through the pixels and look for transparent pixels. | ||
| $w = $this->image->getImageWidth(); | ||
| $h = $this->image->getImageHeight(); | ||
| for ( $x = 0; $x < $w; $x++ ) { | ||
| for ( $y = 0; $y < $h; $y++ ) { | ||
| $pixel = $this->image->getImagePixelColor( $x, $y ); | ||
| $color = $pixel->getColor( 2 ); | ||
| if ( $color['a'] < 255 ) { | ||
| $transparency = true; | ||
| break 2; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self::$checked_images[ $file_path ] = $transparency; | ||
| return $transparency; | ||
| } catch ( Throwable $e ) { | ||
| /* translators: %s is the error message */ | ||
| return new WP_Error( 'image_editor_has_transparency_error', sprintf( __( 'Transparency detection failed: %s', 'webp-uploads' ), $e->getMessage() ) ); | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new transparency checking functionality lacks test coverage. The repository has comprehensive test coverage for other functions (test-helper.php, test-load.php, etc.), but no tests are present for webp_uploads_imagick_avif_transparency_supported(), webp_uploads_check_image_transparency(), or the WebP_Uploads_Image_Editor_Imagick::has_transparency() method. Given the complexity and critical nature of this transparency detection logic, unit tests should be added to verify correct behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to add test coverage based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... copilot seems to have ignored me :( I'll give Claude a try locally...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because this PR uses a branch from a fork 🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still working on some feedback from Copilot. Once that’s done, I’ll write the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what Claude came up with: b1ink0#1