image: preserve ICC profile through WebP decode/encode#30211
Conversation
|
Updated 9:11 PM PT - May 3rd, 2026
❌ @robobun, your commit 38f60d5 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30211That installs a local version of the PR into your bun-30211 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds WebP RIFF mux/demux support and forwards ICCP profiles through the image pipeline: libwebpmux/demux are compiled, WebP decode extracts ICCP, WebP encode can attach ICCP into a VP8X container, codec wiring passes profiles through, and tests/docs updated to reflect WebP preservation. (46 words) ChangesWebP ICC Profile Support
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 1/5 review remaining, refill in 46 minutes and 1 second. Comment |
|
Builds 50708, 50730, 50762: all hard failures are pre-existing flakes unrelated to this change.
50762 is rebased onto |
There was a problem hiding this comment.
Thanks for addressing the anim_* nit in 9b09a88. The implementation and tests look solid, but since this adds hand-written FFI struct layouts (WebPChunkIterator, WebPData), pinned ABI version constants, and copy_data=0 borrowed-buffer ownership across the C boundary, I'd like a human to sign off on the native integration.
Extended reasoning...
Overview
This PR extends ICC colour-profile preservation (added for JPEG/PNG in #30201) to WebP. It touches six files: the libwebp build config (scripts/build/deps/libwebp.ts) to compile src/demux/ and src/mux/; src/image/codec_webp.zig with ~120 new lines of FFI bindings (extern structs, ABI constants, demux on decode, mux on encode); src/image/codecs.zig and src/image/Image.zig to wire icc_profile through and update doc comments; packages/bun-types/bun.d.ts for the public-facing doc; and six new tests in test/js/bun/image/image.test.ts covering PNG↔WebP profile transfer (lossy + lossless), WebP→WebP round-trip, resize survival, and the no-profile fast path.
Security risks
Low. The new code parses untrusted image bytes via libwebp's well-fuzzed demux API; the ICCP chunk is treated as opaque bytes (duped, never interpreted). No auth/crypto/permissions surface. The main hazard class is memory safety at the FFI boundary — extern struct layout mismatches, ABI version drift, or lifetime errors with copy_data=0 borrowing — which would manifest as crashes/UB rather than exploitable injection. I traced the defer ordering (WebPMuxDelete before WebPFree(bitstream), errdefer on the RGBA dupe covering ICC OOM) and the WebPChunkIterator/WebPData layouts against libwebp v1.6.0 headers; they look correct.
Level of scrutiny
Medium-high. This is not a mechanical change: it hand-defines C ABI structs in Zig, hard-codes WEBP_DEMUX_ABI_VERSION/WEBP_MUX_ABI_VERSION constants pinned to a specific libwebp commit, manages cross-language buffer ownership with borrowed pointers, and adds new TUs to a vendored-dependency build. The pattern closely follows the existing JPEG/PNG ICC plumbing and the tests are thorough (including a gate showing 5/6 fail without the fix), but native integration of this kind is where subtle layout/lifetime bugs hide and benefits from a second pair of eyes.
Other factors
My one prior inline nit (backwards dependency-direction comment for anim_decode/anim_encode) was addressed in 9b09a88 by dropping those TUs entirely and correcting the comment — that thread is resolved. The Buildkite failure is an unrelated pre-existing Windows flake in test-http-should-emit-close-when-connection-is-aborted.ts (also seen on #30150/#30188/#30208/#30212/#30180); image.test.ts itself passes on all completed shards. No bugs were flagged by the bug-hunting system.
Follow-up to #30201. Links libwebpmux + libwebpdemux so the ICCP chunk in a VP8X container is extracted on decode and attached on encode. Without this, converting a non-sRGB source (Display P3, Adobe RGB, Jpegli XYB) to or from WebP reinterprets the pixel values as sRGB and visibly shifts colour. Decode: after WebPDecodeRGBA, run WebPDemux on the input and copy out the ICCP chunk payload (if present) into Decoded.icc_profile. Encode: when EncodeOptions.icc_profile is set, wrap the raw VP8/VP8L bitstream via WebPMuxSetImage + WebPMuxSetChunk("ICCP") + WebPMuxAssemble. When it isn't, keep the bare-bitstream fast path so sRGB WebPs stay a single RIFF chunk with no VP8X wrapper. Closes #30197.
WebPAnimDecoder/WebPAnimEncoder layer on top of demux/mux, not the reverse; the core TUs reference no anim_* symbols, and Bun has no animated-WebP support. The previous comment had the dependency direction backwards.
9b09a88 to
38f60d5
Compare
There was a problem hiding this comment.
Thanks for addressing the nit — no further issues found. This adds hand-rolled libwebp mux/demux FFI bindings (extern struct layouts, pinned ABI version constants) and cross-allocator ownership in a codec that handles untrusted input, so I'd like a human to give the Zig/FFI side a once-over before merge.
Extended reasoning...
Overview
This PR extends ICC profile carry-through (added for JPEG/PNG in #30201) to WebP. It links src/demux/demux.c and src/mux/mux{edit,internal,read}.c from the existing libwebp checkout, adds ~60 lines of extern declarations in codec_webp.zig (including hand-mirrored WebPData/WebPChunkIterator struct layouts and pinned WEBP_{MUX,DEMUX}_ABI_VERSION constants), wires decode() to demux the ICCP chunk and encode() to mux it back via VP8X, and threads opts.icc_profile through codecs.zig. Remaining changes are comment/doc updates and six new tests in image.test.ts covering lossy/lossless encode, demux on decode, round-trip, resize, and the no-profile fast path.
Security risks
Image codecs process untrusted input by definition. The new surface here is WebPDemuxInternal parsing the input RIFF container a second time after WebPDecodeRGBA already succeeded; libwebp's demuxer is well-fuzzed upstream and the result is only used to copy a bounded chunk into a fresh allocation, so the risk is low. Memory ownership is the more delicate part: the encode path uses copy_data=0 so mux borrows the bitstream/profile until WebPMuxAssemble returns, the intermediate bitstream is freed via defer WebPFree placed after the fast-path returns, and the assembled output is handed to JS with WebPFree as finalizer. On decode, an errdefer frees the duped RGBA if the ICC dupe OOMs. I traced these and they look correct, but the WebPChunkIterator extern struct layout and the ABI constants are hand-pinned to libwebp v1.6.0 — a mismatch would be memory-unsafe rather than a clean error.
Level of scrutiny
Moderate-to-high. This is not a mechanical change: it introduces new FFI bindings with manually transcribed C struct layouts, two ABI version constants that must track the vendored libwebp commit, and ownership transfers between WebPMalloc and bun.default_allocator. The implementation is carefully commented and the test coverage is thorough (including verifying the no-profile fast path stays a bare VP8/VP8L), but FFI struct layout and allocator-boundary correctness in an untrusted-input codec is exactly the kind of thing that benefits from a second pair of eyes.
Other factors
My earlier inline nit (backwards dependency-direction comment for anim_* TUs) was addressed in 9b09a88 — those TUs are now dropped entirely. CI build 50762 is in progress; prior builds passed image.test.ts on every shard with only unrelated pre-existing flakes. No CODEOWNERS cover src/image/. The bug-hunting system found no issues this round.
Closes oven-sh#30197. Follow-up to oven-sh#30201, which added ICC carry-through for JPEG and PNG but left WebP dropping the profile because `libwebpmux`/`libwebpdemux` weren't linked. ## Repro ```js // any JPEG/PNG with an embedded ICC profile — P3, Adobe RGB, Jpegli XYB await Bun.file("p3.png").image().webp().write("out.webp"); // out.webp had no ICCP chunk → viewers reinterpret as sRGB → colours shift ``` And the reverse direction: a WebP carrying an ICCP chunk lost it on decode, so `webp → png/jpeg` also shifted colour. ## Cause WebP stores ICC profiles in an `ICCP` chunk inside a VP8X RIFF container that wraps the VP8/VP8L bitstream. `WebPDecodeRGBA`/`WebPEncodeRGBA` only touch the bitstream chunk; reading or writing sibling chunks needs the separate demux/mux APIs, and Bun only compiled `src/{dec,enc,dsp,utils}`. ## Fix **Build** (`scripts/build/deps/libwebp.ts`): add `src/demux/*.c` and `src/mux/*.c` from the same libwebp checkout. Plain C, no new deps, same include paths. **Decode** (`src/image/codec_webp.zig`): after `WebPDecodeRGBA`, run `WebPDemux` on the original bytes, check `WEBP_FF_FORMAT_FLAGS & ICCP_FLAG`, and `WebPDemuxGetChunk("ICCP")` the profile into `Decoded.icc_profile` (duped into `bun.default_allocator` to match JPEG/PNG ownership). A plain VP8/VP8L WebP with no VP8X wrapper falls through with `null`. **Encode**: `webp.encode` now takes `icc_profile: ?[]const u8`. When `null`/empty, keep the existing one-shot `WebPEncodeRGBA` fast path (bare VP8/VP8L, no VP8X). When set, pass the bitstream through `WebPMuxSetImage` + `WebPMuxSetChunk("ICCP")` + `WebPMuxAssemble` to produce a VP8X-wrapped file and hand the assembled buffer to JS with `WebPFree` as the finaliser. `codecs.zig` / `Image.zig` / `bun.d.ts` comments updated to drop the "WebP loses the profile" caveat. ## Verification New tests in the existing `describe("ICC profile")` block of `test/js/bun/image/image.test.ts` walk the output RIFF container to find the `ICCP` fourcc and compare the payload byte-for-byte: - PNG iCCP → WebP lossy → ICCP chunk present, VP8X flag bit set - PNG iCCP → WebP lossless → ICCP chunk present - WebP ICCP → PNG → iCCP chunk present (exercises demux) - WebP → WebP round-trip preserves profile - resize → WebP preserves profile - source without profile → WebP stays a bare VP8/VP8L (no VP8X wrapper) ``` bun bd test test/js/bun/image/image.test.ts # 93 pass, 2 skip (macOS-only GIF parity), 0 fail ``` Gate: with `src/` + `packages/` stashed and rebuilt, 5 of the 6 new WebP tests fail (`Received: null`); with the fix, all 16 ICC tests pass. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
Closes #30197. Follow-up to #30201, which added ICC carry-through for JPEG and PNG but left WebP dropping the profile because
libwebpmux/libwebpdemuxweren't linked.Repro
And the reverse direction: a WebP carrying an ICCP chunk lost it on decode, so
webp → png/jpegalso shifted colour.Cause
WebP stores ICC profiles in an
ICCPchunk inside a VP8X RIFF container that wraps the VP8/VP8L bitstream.WebPDecodeRGBA/WebPEncodeRGBAonly touch the bitstream chunk; reading or writing sibling chunks needs the separate demux/mux APIs, and Bun only compiledsrc/{dec,enc,dsp,utils}.Fix
Build (
scripts/build/deps/libwebp.ts): addsrc/demux/*.candsrc/mux/*.cfrom the same libwebp checkout. Plain C, no new deps, same include paths.Decode (
src/image/codec_webp.zig): afterWebPDecodeRGBA, runWebPDemuxon the original bytes, checkWEBP_FF_FORMAT_FLAGS & ICCP_FLAG, andWebPDemuxGetChunk("ICCP")the profile intoDecoded.icc_profile(duped intobun.default_allocatorto match JPEG/PNG ownership). A plain VP8/VP8L WebP with no VP8X wrapper falls through withnull.Encode:
webp.encodenow takesicc_profile: ?[]const u8. Whennull/empty, keep the existing one-shotWebPEncodeRGBAfast path (bare VP8/VP8L, no VP8X). When set, pass the bitstream throughWebPMuxSetImage+WebPMuxSetChunk("ICCP")+WebPMuxAssembleto produce a VP8X-wrapped file and hand the assembled buffer to JS withWebPFreeas the finaliser.codecs.zig/Image.zig/bun.d.tscomments updated to drop the "WebP loses the profile" caveat.Verification
New tests in the existing
describe("ICC profile")block oftest/js/bun/image/image.test.tswalk the output RIFF container to find theICCPfourcc and compare the payload byte-for-byte:Gate: with
src/+packages/stashed and rebuilt, 5 of the 6 new WebP tests fail (Received: null); with the fix, all 16 ICC tests pass.