Repository navigation
Media: Add a libvips image editor (WP_Image_Editor_Vips) - #13762
nickchomey wants to merge 84 commits into
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
6851721 to
9917ec0
Compare
|
@notglossy this PR is inspired by your work. Do you have any feedback on it? |
|
Thanks for working on this! It's great to see libvips being considered for WordPress core. Faster image processing with lower memory usage would be a really useful improvement for WordPress. I maintain a plugin with the same functionality on wordpress.org, Indigit Imagination, so I tested revision (75a5a0f) against problems I had already met. Here are the things I ran into while testing, and I can elaborate on each of them if you want me to. Host: PHP 8.4, libvips 8.16.1, unless noted. 1. Minimum libvips version. 2. AVIF is very CPU-heavy. A 33 MP JPEG converted to AVIF takes 42 CPU-seconds with this editor (Imagick: 9.6). With PHP's usual 30 s limit on typical Linux PHP-FPM, the upload is killed after about 5 s, because PHP counts the CPU time of all threads. The AVIF save options set no 3. Files are bigger than Imagick's. JPEG: +25% over the 7 files of one upload, at the same quality setting (82). There are two causes of about equal size. The JPEG save options do not optimise the Huffman tables ( 4. Metadata. For JPEG and WebP the PR strips all metadata from resized images. Core's Imagick editor keeps these profiles there: 5. Damaged files. A PNG cut off at 70% still gets a sub-size built from partial data, where Imagick and GD fail. libvips can make that an error with the loader option 6. 7. Untrusted loaders. libvips chooses a loader from the file's content, and 15 of its loaders are flagged untrusted. Through a normal WordPress upload only BMP reaches one (ImageMagick, the same exposure as in Imagick today). The editor has no loader allowlist: 8. No image size limit. A 1.5 MB grey PNG of 40,000 x 40,000 pixels costs 22 CPU-seconds in this editor, and Imagick refused it. Memory stayed low in my test, so this is about CPU only. Tests. The tests I found missing, most important first:
I have more points that are not in this list, and I can explain them if you want. I'm willing to help with this feature, and I can open a pull request against your branch with fixes and tests if you would like that. My plugin also handles each of these points, so it may be useful as a reference. |
|
Thanks for the review! I started addressing all of it yesterday and will probably finish early next week. I'd appreciate another review after that. |
…ick-specific test)
The keep saver option, used to preserve colour profiles when metadata is stripped, is only available from 8.15. Raise the floor to it and drop the version branches the floor makes unreachable: the 8.8 guard around vips_foreign_get_suffixes(), the 8.13 guard around the PNG palette option, and the 8.14 guard around gifsave interlace.
Pass libvips a keep mask of ICC, EXIF, XMP and IPTC instead of the strip boolean, which discarded every field including the colour profile. The protected set is the one the Imagick editor protects, so a resized image no longer loses how it is meant to be rendered. The strip_meta option is now the same for every format, so it moved out of the per-format switch.
The keep mask is only honoured in full from libvips 8.16. On 8.15, PNG text comments, the profiles ImageMagick writes and the image description survive it, so they are removed from the image before it is encoded. The list of fields the mask protects and the list it misses are derived from the same policy, so a field that is protected is never removed.
The filter is documented on the GD editor, which is where the Imagick editor points as well, and core's own static analysis requires each call site to say so.
The loader is asked to fail on truncation, but libvips reads pixels lazily, so a truncated PNG is rejected at load by some encoders and only when the pixels are first read by others. The test asserts the contract that holds either way: a truncated file produces no output. A truncated JPEG is still filled rather than rejected, matching the GD and Imagick editors.
The AVIF path was only covered by loading one and saving it as PNG. This saves as AVIF, checks the file really is one, and checks that the encoding effort defaults to 3 and that the filter is what the encoder is given.
The existing test rotated a square image 180 degrees and only checked that the size was unchanged, so a rotation that did nothing would have passed. This rotates a non-square image through every right angle and compares each corner of the result with the corner of the source it should have come from, which also covers the dimension swap a quarter turn has to make.
Only the horizontal axis was covered, and only by one corner. This checks both axes by comparing every corner with the corner it should have come from, on a fixture whose four corners are all different, so a flip on the wrong axis cannot pass.
The floor decides whether the editor is used at all, but reaching it through test() needs an older libvips than the one running the suite, so the comparison moves into its own method and is checked directly: 8.7, 8.12.1 and 8.14.5 are rejected, 8.15 and above are accepted.
The `keep` saver option that preserves the colour profile when metadata is reduced only exists from libvips 8.15, and passing an unknown option fails the save. Move the floor to 8.13 and pass `keep` only where it exists; below that the fields are removed by hand, which was already happening below 8.16. Restore the libvips 8.14 guard on the GIF interlace option, which the 8.15 floor had made unnecessary.
Whether the `keep` mask can be passed, and whether the unprotected fields also have to be removed by hand, were answered by two separate version checks: one in _save(), one in get_save_options() while building the saver options. get_metadata_strategy() now returns both answers for a version, so the policy lives in one place. get_save_options() takes that strategy instead of a strip_meta boolean, and _save() consults it for the removal.
75a5a0f to
6f8f8e5
Compare
|
Thanks for the detailed review. I've gone through all eight points; most are fixed now, and where I could I measured instead of taking the point on trust. A few I left out on purpose, and there are a couple of your claims I couldn't reproduce and would like your input on. Everything below was measured in Docker with libvips 8.16.1 unless I say otherwise. What changed
On the kernel (#3b). I ran a 28-case comparison of It isn't free either. About 10% more bytes, and on the small images I tested, 10% more wall time. So the default is On AVIF effort (#2). You were right, and I measured it to be sure. First I confirmed that libvips really does default to effort 4: encoding with no
So the default does blow past a 30 s PHP-FPM limit, as you said, though I measured 73 CPU-seconds rather than 42, so the figure depends on the content and the machine. Effort 3 cuts that to 26 s for a 0.35% size cost, and above 4 more effort buys nothing (effort 5 came out slightly larger). I've kept 3 as the default for now, and there are two things I'd still like your view on. Effort 3 is still only just under 30 s at 33.8 MP, so a larger image can still exceed the limit, and 2 may be the safer choice. And it applies on every AVIF save and stream, not only on a resize. On truncated files (#5). I couldn't reproduce the PNG case. With libvips 8.16.1 a truncated PNG fails when the pixels are read whether or not That also exposed a weak spot in our own tests. The original test used a PNG, so it passed without the change. It now covers a truncated GIF, which fails if the load options are removed. If you did see a sub-size produced from a truncated PNG, I'd like the details: the file, its size, and your libvips version. It may be a version difference worth understanding. On loaders (#7). Confirmed, exactly as you described. An SVG renamed to On metadata (#4). I agree this is a cross-editor policy, and that's how I treated it. The kept set is Imagick's, decided in one place ( I also measured the colour shift you predicted, and it depends on the image. For an sRGB-tagged image, dropping the profile is a no-op in the strict sense: the mean per-channel difference is 0.00, max 0, since interpreting sRGB values as sRGB changes nothing. For a genuine wide-gamut image the shift is real: converted to Display P3 and then de-tagged, the mean difference is 2.54 / 3.68 / 0.71 with a maximum of 81 out of 255. So keeping the profile is clearly right. Our fixture is sRGB-tagged, so On the floor and the 8.12.1 failure (#1). I tried to reproduce this one. In On the pixel cap (#8). Two corrections to how I first described it, both now fixed in the docblock. It isn't a format limit: a JPEG can describe 65500 pixels per side (verified: libvips encodes 17000- and 40000-pixel-wide JPEGs fine and only fails at 70000). And it isn't a limit the other editors share, since neither GD nor Imagick caps size at all. More importantly, it's a behaviour change rather than a pure guard. Deliberately left out. You mentioned more points beyond the list. If any of these are among them, I'd rather discuss than have you find them missing:
Testing that surfaced a real gap: both existing GIF fixtures are single-frame, so the flattening behaviour was untested. There's now a two-frame fixture ( Test coverage is up to 77 tests / 243 assertions / 1 skip, and now includes truncated PNG and GIF tests, an AVIF encode test, rotation correctness on non-square pixels, flip on both axes, the min-version gate and the metadata strategy. I welcome any further feedback on the PR! |
Adds
WP_Image_Editor_Vips, an image editor backend backed by libvips reached through PHP's FFI extension.It is listed first in
wp_image_editors, ahead ofWP_Image_Editor_ImagickandWP_Image_Editor_GD. It reports itself as unavailable when libvips, the FFI extension, orffi.enableis missing, sowp_get_image_editor()falls through to Imagick or GD and nothing changes on a server that cannot run it.Trac ticket: https://core.trac.wordpress.org/ticket/65473
Inspired by the vips-image-editor-ffi plugin, but heavily reworked to follow the patterns of the GD and Imagick editors and to implement equivalent test coverage.
What this adds
WP_Image_Editorimplementation: load, save, resize, crop, rotate, flip, stream,multi_resize(), andmake_subsize().Design notes
Several parts of this take a route that is not the obvious one, because the obvious libvips call does not work. Flagging them here so they do not read as mistakes.
EXIF orientation is applied with
autorot(), not by rotating and flipping. libvips applies all eight EXIF orientations itself and clears the orientation field as it goes. Using the parent class's rotate/flip switch would leave the field in place, and the saved file would then be oriented a second time when it is loaded again. This is the "EXIF Orientation can be reset afterwards" requirementmaybe_exif_rotate()documents.Rotate and flip reset the orientation tag by setting the
orientationfield to 1 and then callingautorot(). This mirrors Imagick'ssetImageOrientation( ORIENTATION_TOPLEFT ). Setting the field alone does not reach the file: libvips writes JPEG EXIF from the EXIF block rather than from the metadata field.autorot()is the operation that rewrites that block, and declaring the image upright first means it has nothing to apply, so the pixels are untouched and only the tag is corrected. Verified:pixdiff = 0against the un-reset image, and no change to PNG files that have no EXIF at all._resize()is a separate method because libvips is immutable — every operation returns a new image — soresize()andmake_subsize()both need a fresh image without disturbing$this->image. This is GD's shape rather than Imagick's, which mutates in place and works around it by snapshotting and restoring.Encoder support is probed by writing to a buffer, not a temp file, which avoids a temp file per format check.
Paletted PNGs are saved with
palette => true. libvips expands a palette on load, so the original colour type is read from the PNG IHDR chunk at load time and a palette is requested back on save. Without it a resized indexed PNG is written as true colour; the same fixture that is 25 KB on disk comes back at 342 KB.image_max_bit_depthis applied as an encoder option, not by downcasting.cast()to a lower band format fails on write withpngsave: invalid buffer sizefor any 16-bit source (libvips 8.16.1), so the filter is passed to the encoders that accept a bit depth instead.Metadata is reduced to the set Imagick keeps rather than stripped with libvips'
strip.stripis a boolean that drops every field, including the colour profile, which changes how the image renders once nothing tells the browser how to interpret the samples. libvips instead offers akeepmask, set to the same setWP_Image_Editor_Imagick::strip_meta()protects: ICC, EXIF, XMP and IPTC.image_strip_metastill decides whether any of this happens. The mask only exists from libvips 8.15, soget_metadata_strategy()is the one place that decides what a given version can do:keepmaskThe minimum libvips version is 8.13, which is the first release that can write a palette back to a PNG — the same feature the paletted-PNG note above depends on. Below it the editor reports itself unsupported rather than running without it.
The resampling kernel is passed explicitly and is filterable (
wp_image_editor_vips_resampling_kernel, defaultlanczos3), because libvips would otherwise choose silently. Measured over 28 cases against core's Imagick output and the original,lanczos3led SSIMULACRA2, SSIM, MS-SSIM and IW-SSIM. It did not lead Butteraugli (lanczos2was marginally better) or CIEDE2000 (Imagick's output did, withlanczos3second). It costs roughly 10% more bytes and about 20% more CPU thanlinear. Those are full-reference metrics against a chosen reference, so they measure distance rather than quality; the default islanczos3andlinearis one filter away.Truncated files are rejected through libvips'
fail_onloader option, with JPEG set tononeso a damaged JPEG is filled rather than rejected, matching the other editors. The option is load-bearing only for some formats: a truncated GIF or JPEG reads cleanly without it, while PNG and WebP fail in the decoder either way. The test therefore uses a GIF, which is the case the option actually decides.Encoding options are set for parity, not by default. JPEG gets
optimize_coding(measured 4.6% smaller at equal quality on a 33.8 MP source).image_save_progressiveis mapped to libvips'interlacefor JPEG, PNG and GIF, with the GIF case guarded becausegifsaveonly accepts it from libvips 8.14. AVIF gets aneffortof 3 (wp_image_editor_vips_avif_effort); libvips defaults it to 4, verified by encoding with and without the option, and on a 33.8 MP image that is 73 CPU-seconds against 26 at effort 3, for a 0.35% size cost. Effort 3 is still close to a typical 30-second PHP-FPM limit at that size, and effort above 4 produces a slightly larger file.WebP quality is mapped the way Imagick maps it. ImageMagick's WebP coder reserves quality 100 for lossless, so the same number would otherwise mean very different things in the two editors — libvips' Q100 is a lossy maximum, measured at 90,262 bytes where Imagick's Q100 is 311,954 and a lossless write of the same image is 309,444. Quality 100 is therefore written losslessly here.
libvips chooses its loader from the file's magic bytes, not the extension, so the format an upload claims is not necessarily the format decoded. Measured: an SVG renamed to
.jpgstill decodes throughsvgload. The decoder is therefore checked against an allowlist (wp_image_editor_vips_allowed_loaders), and a loader outside it is refused, which reports the format as unsupported and letswp_get_image_editor()fall through to Imagick.A pixel cap is enforced at load (
wp_image_editor_vips_max_pixels, default 16383², 268,402,689 px). libvips reads pixels lazily, so a small file can announce enormous dimensions and only cost that memory once something touches the pixels; the dimensions are known first, so the refusal comes first too. Two things about the default are worth stating plainly, because the obvious reading is wrong: it is not a format limit — a JPEG can describe 65500 pixels per side, verified by encoding 17000- and 40000-pixel-wide JPEGs and failing only at 70000 — and it is not a limit the other editors share, since neither GD nor Imagick restricts the size it will attempt. It is a deliberately conservative guard, and because_wp_image_editor_choose()selects an editor before the file is opened and does not retry, an over-size image fails to load rather than falling through to them. A site handling images that large has to raise the filter.Vendoring
php-vips is vendored rather than required through Composer, so that a site running this needs no Composer step and no network access at runtime. The tree is scoped with PHP-Scoper into
WordPress\VipsDependencies\...withJcupitt\Vipsdeliberately left unscoped, so the public API of the library is unchanged. The generatedautoload.phprecords the upstream repository, ref and commit it was built from, and regeneration is deterministic.tools/php-vips/installer.shperforms the vendoring. With no arguments it resolves the latest upstream release;--version=and--branch=select a specific ref. It hard-fails if the scoper step does not actually rewrite files, which it will do silently on PHP 8.5 with older scoper releases.Testing instructions
The local Docker environment builds a PHP image with libvips and the FFI extension. Add to
.env(which is not tracked):Then:
To confirm the fallback path, run the same group with FFI disabled.
npm run test:phpcannot pass-dthrough to PHP, so invoke the runner directly:ffi.enableis what makes it work rather than unloading the extension: the extension stays loaded, and libvips fails to initialise, soWP_Image_Editor_Vips::test()returns false andwp_get_image_editor()selects Imagick or GD instead.Test results
With VIPS enabled, on the branch rebased onto current trunk:
wp-image-editor-vipsimageTests_Media+WP_Test_REST_Attachments_ControllerRebasing onto current trunk exposed two VIPS-only failures, both since fixed and both worth recording because each was a real divergence from Imagick rather than a difference of taste:
invalid_imagerather thanerror_loading_image. The read sat inside the sametryas the decode, so a stream that throws was indistinguishable from a file that would not decode. Imagick reads a stream first and maps only that failure toerror_loading_image; this editor now does the same.test_quality_with_image_conversion_file_sizesassertion depends on. GD behaves like libvips here (85,330 bytes at Q100); it passes that test only because GD strips metadata, so its JPEG intermediate is much smaller.Static analysis: PHPStan reports no errors on the changed files, and PHPCS is clean.
All of these were re-measured after the rebase onto current trunk.
wp_generate_attachment_metadata: 7 tests, 18 assertions, 0 failures. Full suite: 26,505 tests, 4,563,120 assertions, 45 skips, 86 warnings, and one failure that is not related to this PR —Tests_Theme_ThemeDir::test_broken_themesasserts the exact contents ofget_broken_themes(), so it fails in any environment that has extra theme directories installed. It fails identically withffi.enable=0, i.e. with this PR's editor out of the picture entirely, and the PR touches no theme files. The warnings are the suite's own PHPUnit 10 deprecation notices forexpectDeprecated(), not failures, and none of them come from the image tests.Known limitations
image_max_bit_depthhas no effect on JPEG, WebP or GIF. libvips stores samples in a fixed set of band formats rather than at an arbitrary bit depth, and only the PNG and HEIF encoders accept a bit depth. Imagick applies the filter to every format because it lowers the depth of the in-memory image. This is documented inget_save_bit_depth().A resized paletted PNG can come out slightly larger than its source.
palette => truere-quantises at libvips' default quality, which lands close to the original but can overshoot: of five paletted fixtures, two shrink and three grow by up to a third (test8.png, 45,231 → 60,310 bytes). Passing aQvalue makes all five smaller but controls how aggressively colours are reduced, so it trades colour fidelity for bytes. Left alone rather than decided unilaterally.Encoding to AVIF or HEIC depends on the host's encoder plugins. Reading HEIC and HEIF works with the standard libheif decoder plugins, but writing needs an encoder plugin, and libvips loads those as plugins at runtime, so the list of formats the build was compiled with says nothing about whether they can actually encode. Those formats are therefore always verified by encoding rather than by that list, and a host without the plugin reports the format as unsupported. Core only ever writes one of them:
wp_get_image_editor_output_format()maps HEIC and HEIF to JPEG, so the HEIC probe matters for reading and forwp_getimagesize(), and only comes into play for writing if a site filters that mapping.No CI job runs these tests. The suite runs without libvips in CI, so the editor is not yet exercised automatically. Guidance on how core would like that set up is welcome. On Ubuntu 24.04 the runtime package is
libvips42t64rather thanlibvips42, and the packaged 8.15.1 is new enough for everything used here, so a stock runner should work without a custom image.Reviewer notes
Two changes outside the new class are not self-explanatory from the diff. The HEIC branch of
wp_getimagesize()no longer requiresinstanceof WP_Image_Editor_Imagick; any editor that claimed HEIC support and loaded the file can report the size, and GD never reaches it because it does not claim HEIC support. Separately, theimage_max_bit_depthdocblock in the Imagick class stated that the filter "only applies when resizing using the Imagick editor since GD does not support getting or setting bit depth", which stops being true once VIPS honours it.The pixel cap changes behaviour, not just resource use, and it is the one guard here I would like a second opinion on. Because editor selection happens before the file is opened and never retries, an image larger than the default now fails where Imagick would have attempted it. That is the intended protection, but it is a decision rather than a side effect.
There is an
@phpstan-ignoreinmaybe_exif_rotate(). Theis_callable( 'exif_read_data' )guard is reported asfunction.alreadyNarrowedTypebecause PHPStan's environment always has the extension, but the extension is optional in practice and@does not suppress a fatalCall to undefined function.class-wp-image-editor.php:489carries the identical construct; it is suppressed through a baseline entry, andtests/phpstan/baselines/function.alreadyNarrowedType.neonhas the same suppression for the same function insrc/wp-admin/includes/image.php. The baselines are documented as being for pre-existing errors and meant to shrink, so an inline ignore was used for new code. Happy to switch to whatever the project prefers._resize()has no Imagick equivalent. If reviewers prefer the editors to share a shape, this can be dropped in favour of Imagick's snapshot-and-restore approach.The two libvips behaviours in the design notes above (
set( 'orientation', 1 )not reaching the saved file, andcast()breakingpngsave) are the two places where reading the code without context will suggest a simpler implementation. Both were measured, not assumed.Use of AI Tools
AI assistance: Yes
Tool(s): GitHub Copilot
Model(s): DeepSeek V4.1
Used for: Pair programming - design, code and test implementation. Final implementation and tests were reviewed and edited by me.