Feature: CSS Gradient Low Quality Image Placeholders#2550
Conversation
|
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 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #2550 +/- ##
==========================================
+ Coverage 70.35% 70.66% +0.31%
==========================================
Files 91 92 +1
Lines 7867 8090 +223
==========================================
+ Hits 5535 5717 +182
- Misses 2332 2373 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…sed for initial image processing and dominant colours
…never do anything in any test
…ominant_color_get_dominant_color_data. So not redundant to call it afterwards
…e GD tests tun, though not for imagick)
…t stuff coming in other PR. Also, cleaner to just convert in one place for all editors
3d68d73 to
bf90a65
Compare
bf90a65 to
893feee
Compare
|
@westonruter i've addressed your comments now. Two comments remain unresolved - minified css and the inline style for hovering to reveal the lqip css gradient. I responded to those, happy to handle them however you want. |
|
hover css removed now |
|
oh weird, the min.css file i created got skipped by the repo-level gitignore Lines 29 to 38 in 8d70987 Should I just remove the code that loads it? Evidently they arent used anywhere. That's probably something to address in another issue and PR |
|
No, it's good that the minified file wasn't committed. We need to add CSS minification to the build process the same way we have for JS. |
Register a webpack config for the dominant-color-images plugin that generates assets/lqip.min.css via the shared cssMinifyTransformer, and add the plugin to pluginsWithBuild so the distribution build depends on it. This produces the lqip.min.css that hooks.php enqueues when SCRIPT_DEBUG is off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
With 6636887 the CSS file should get automatically re-minified whenever the plugin is built (via |
| --lqip-ca: mod(round(down, calc((var(--lqip) + 524288) / 262144)), 4); | ||
| --lqip-cb: mod(round(down, calc((var(--lqip) + 524288) / 65536)), 4); | ||
| --lqip-cc: mod(round(down, calc((var(--lqip) + 524288) / 16384)), 4); | ||
| --lqip-cd: mod(round(down, calc((var(--lqip) + 524288) / 4096)), 4); | ||
| --lqip-ce: mod(round(down, calc((var(--lqip) + 524288) / 1024)), 4); | ||
| --lqip-cf: mod(round(down, calc((var(--lqip) + 524288) / 256)), 4); | ||
| --lqip-ll: mod(round(down, calc((var(--lqip) + 524288) / 64)), 4); | ||
| --lqip-aaa: mod(round(down, calc((var(--lqip) + 524288) / 8)), 8); |
There was a problem hiding this comment.
PhpStorm's static analysis is flagging these:
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 262144)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 65536)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 16384)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 4096)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 1024)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 256)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 64)) and 4 must be of the same type: , , or .
Argument type mismatch. The arguments round(down, calc((var(--lqip) + 524288) / 8)) and 8 must be of the same type: , , or .
There was a problem hiding this comment.
i added a property. let me know if that doesnt fix the static analysis problem
| * @return string|WP_Error Dominant hex color string, or an error on failure. | ||
| * @return array{r: int, g: int, b: int}|WP_Error RGB values (0-255), or WP_Error on failure. | ||
| */ | ||
| public function get_dominant_color() { |
There was a problem hiding this comment.
Feedback from Claude Opus 4.8:
Performance: full-resolution pixel scan when no medium subsize exists
Dominant_Color_Image_Editor_GD::get_dominant_color() now iterates every pixel of the source image in pure PHP (imagecolorat + sRGB→linear LUT + float accumulation), replacing the old resize-to-1×1 approach whose cost was independent of image dimensions. At the same time, dominant_color_get_attachment_file_path() no longer returns false when the requested medium size is missing — the if ( ! isset( $imagedata['sizes'][ $size ] ) ) { return false; } guard was dropped, so it now falls back to the full-resolution original.
In the normal upload flow this is fine: the medium subsize (~300px) is persisted before metadata generation runs, so the loop is ~67k iterations. But when medium is unavailable — e.g. medium_size_w/medium_size_h set to 0, or an original smaller than the medium threshold — a large original (a multi-megapixel -scaled image or bigger) gets scanned pixel-by-pixel in PHP during wp_generate_attachment_metadata. On a several-megapixel image that's multiple seconds of CPU and can exceed max_execution_time, blocking the upload/regeneration. The get_lqip_grid_values() path adds a further redundant has_transparency() pass.
Suggested fix: resize a working copy down to a small fixed size (as the original did) before the per-pixel averaging loop, so cost stays bounded regardless of source resolution.
Severity: conditional (only bites when medium is absent) — worth addressing but not CI-blocking. Not covered by the test suite, since fixtures are small.
The pixel-scan concern is GD-specific. Imagick is not affected.
The difference is in how each engine averages:
- GD (
get_dominant_color) does the averaging itself, in a pure-PHP nestedforloop over every pixel (imagecoloratper pixel). Cost scales withwidth × heightin the PHP layer → the timeout risk. - Imagick (
get_dominant_color) offloads the averaging to ImageMagick's C core: it clones, thenresizeImage( 1, 1, FILTER_LANCZOS, 1 )and reads that single pixel. The averaging happens during the resize, so cost is essentially bounded regardless of source resolution — the same O(1)-from-PHP's-view approach the old GD code used before this PR.
The shared part of the change — dominant_color_get_attachment_file_path() falling back to the full-resolution original when medium is absent — applies to both engines, but it only turns into a problem for GD, because only GD then walks that full-resolution image pixel-by-pixel in PHP. For Imagick, feeding a larger image into a C-level resize-to-1×1 stays cheap (Imagick already holds the decoded image in memory regardless).
get_lqip_grid_values() is bounded for both (GD imagecopyresampled → 3×2, Imagick resizeImage → 3×2), so it doesn't add to the concern.
So the finding applies specifically to the GD editor; sites using Imagick won't hit it.
There was a problem hiding this comment.
I wonder whether we should even be doing this looping over pixels at all... It was added to address #2535 because GD didnt seem to have such a mechanism as Imagick did.
It seems to me that:
- GD is uncommon
- As noted in the OP, the dominant color used generally doesnt have much effect on the final css gradient that's generated, which is why the 1x1 approach remained in use rather than something like ColorThief.
- This feature is so... minor... (just a flash of a gradient while the image downloads for the first time). If the gradient is slightly off from a non-perfect initial dominant color, people wont notice or even care.
- Sticking with 1x1 method wouldnt break any dominant color values that have already been extracted and stored in anyone's db.
Perhaps I should just revert the GD editor to 1x1 to avoid the complexity and overhead?
There was a problem hiding this comment.
Ugh, now the fixes have broken tests because the generated color is different. All the more reason to just go back to basics.
Summary
Fixes #2519 and #2535
Note:
This is stacked on/includes #2524, which isn't totally necessary but I think made improvements to the test functions. I can rebase this without any of that if desired.
Relevant technical choices
Implemented the CSS Gradient-based LQIP mechanism described in #2519. Extensive testing was done with various ways to generate the dominant color (the original used ColorThief, which has a php equivalent). On rare occasions, the fancier methods produced somewhat better gradients, but at great processing cost. Ultimately I found that the 1x1 pixel was sufficiently-good - especially given that it is already implemented.
I also changed the GD and Imagick editors to extract the color in linear rgb, as per #2535. Updated some tests to reflect the different results. They all pass for me.
Here's a video that shows the gradients using the previous colorspace (left) and the new colorspace (right), generated by imagick. Generally no difference, but its somewhat better in a few cases.
Kooha-2026-06-25-15-22-14.mp4
Nothing was done to accomodate any sort of transition. To use this, all images would need to be reprocessed. Both the dominant color hex and lqip values are generated and stored in postmeta.
Use of AI Tools
I used Deepseek V4 Flash via Github Copilot to assist with this. I reviewed and iterated on everything.