-
Notifications
You must be signed in to change notification settings - Fork 128
Fixes palette-based PNG uploads failing original full-size AVIF/WebP conversion under GD #2024
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
base: trunk
Are you sure you want to change the base?
Fixes palette-based PNG uploads failing original full-size AVIF/WebP conversion under GD #2024
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #2024 +/- ##
==========================================
+ Coverage 67.18% 67.25% +0.07%
==========================================
Files 93 93
Lines 7750 7776 +26
==========================================
+ Hits 5207 5230 +23
- Misses 2543 2546 +3
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:
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @IlyaZha, @mytory, @chimok. 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.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
@b1ink0 Thank you, this looks great to me so far, just one comment.
It would be great to get @adamsilverstein's review on this as well.
plugins/webp-uploads/hooks.php
Outdated
return $file; | ||
} | ||
|
||
if ( 'png' !== strtolower( pathinfo( $file['name'], PATHINFO_EXTENSION ) ) ) { |
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.
Should this look at the attachment metadata instead to see if the MIME type is image/png
?
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.
Yes, we can just check if $file['type']
is image/png
.
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.
Done in 7443e68.
Co-authored-by: Weston Ruter <[email protected]>
Hi @b1ink0 and thanks for the PR.
Good catch! I understand the technical requirement for GD here and agree we need to address them to resolve this issue. My only concern is that by converting a palette image (up to 8 bit) to truecolor (eg. 24 bit), we might inadvertently bloat the image size. We tried to address this for Imagick in https://core.trac.wordpress.org/ticket/36477. I'd love to see some before/after size numbers for indexed PNG uploads converted to WebP and AVIF to confirm that isn't the case. I suspect we are ok given the lossy compression of WebP/AVIF vs the lossless PNG compression, but it would be worth checking and perhaps adding a unit test for this aspect. |
plugins/webp-uploads/hooks.php
Outdated
|
||
// Check if required GD functions exist. | ||
if ( | ||
! function_exists( 'imagecreatefrompng' ) || |
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.
+1 to defensive programming; however all of these functions have been around since PHP4/5 as as far as I know should be available to all WordPress installs with GD. So maybe we can remove this check?
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.
That would address the code coverage issue as well, if it is impossible to create a scenario where any of these functions don't exist.
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.
Oh, but actually will these functions be available if GD is not installed? If PHP is not compiled with --with-gd
then I don't think they will.
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.
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.
If PHP is not compiled with GD,
Just to clarify for my own understanding - is GD bundled by default and would need to be disabled for the build?
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 still think we can remove these - if GD is the chosen image editor we can be confident PHP supports GD. See https://github.com/WordPress/performance/pull/2024/files#r2217413978
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.
If PHP is not compiled with GD,
Just to clarify for my own understanding - is GD bundled by default and would need to be disabled for the build?
GD is bundled with PHP source code by default, but it must be explicitly enabled during compilation and can be omitted from the final build.
https://www.php.net/manual/en/image.installation.php
I still think we can remove these - if GD is the chosen image editor we can be confident PHP supports GD. See https://github.com/WordPress/performance/pull/2024/files#r2217413978
You're right, I'll remove these redundant function checks.
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.
Done in ca8599c.
plugins/webp-uploads/hooks.php
Outdated
} | ||
|
||
$image = imagecreatefrompng( $file['tmp_name'] ); | ||
if ( false === $image || imageistruecolor( $image ) ) { |
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 confused by this check. If $image is false, the conversion failed. In that case, don't we want to continue to the processing below?
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.
When imagecreatefrompng()
returns false, it means the creation of the PNG failed. In that case, we can't convert it to truecolor because we need a valid GDImage resource for imagepalettetotruecolor()
to work properly. Returning early in this scenario is the best choice since there's no valid image resource to operate on.
I also combined two different checks in that condition, which I realize makes the logic less clear. Separate those out in 7f757c7 to improve readability.
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.
thanks for clarifying
Co-authored-by: Adam Silverstein <[email protected]>
I ran some tests on a few indexed PNGs. As expected, converting them to truecolor PNGs increased the file size up to 2.5x in some cases. However, when these converted truecolor PNGs were then encoded to AVIF or WebP, the resulting file sizes were consistently smaller than the original indexed PNGs encoded directly to AVIF/WebP. cc: @adamsilverstein |
Thanks for the PR and all the work here @b1ink0. I will try testing with the Bedrock container you linked to earlier. |
I was able to verify the issue with the Bedrock container and some code to force using GD. I also verified the PR fixes the issue. This should be good to go. We should probably port this fix to WordPress core since this will also affect anyone using the core output filter directly with the same configuration. |
return $file; | ||
} | ||
if ( isset( $file['type'] ) && is_string( $file['type'] ) ) { | ||
if ( 'image/png' !== strtolower( $file['type'] ) ) { |
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.
do we need this check? it seems duplicated by the following check with wp_check_filetype
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 checks aren't duplicated they handle different scenarios. I added the first check for $file['type']
per this #2024 (comment) feedback to use the MIME type from attachment metadata (this is available when the file is uploaded and present in $_FILES
for ref.) instead of just relying on the file extension. The elseif
ensures we fall back to wp_check_filetype()
when $file['type']
isn't available.
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.
Ok, makes sense. We also have wp_check_filetype_and_ext
in core that might be useful here.
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.
Done in e39b2c7.
@b1ink0 - this looks very close, thanks! I left a few additional comments, let me know what you think. |
Co-authored-by: Adam Silverstein <[email protected]>
Summary
Fixes #2018, #1561, #1864
Relevant technical choices
This change hooks into both
wp_handle_upload_prefilter
andwp_handle_sideload_prefilter
to detect palette-based (indexed-color) PNGs and convert them in-place to truecolor while preserving transparency before any AVIF/WebP encoding takes place. It preserves all existing upload metadata and only affects PNGs handled by the GD image editor.