Bun.Image: preserve 16-bit-per-channel PNG through decode/encode - #30463
Bun.Image: preserve 16-bit-per-channel PNG through decode/encode#30463robobun wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds 16-bit-per-channel PNG support to Bun.Image. The PNG codec now decodes and encodes 16-bpc data with proper bit_depth tracking. The image pipeline conditionally downconverts to 8-bit only when operations or non-PNG encoding require it, preserving 16-bit precision for PNG pass-through. Tests validate 16-bpc round-trip and downconversion behavior. Changes16-bit PNG preservation and conditional downconversion
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/image/codecs.zig`:
- Around line 295-305: The probe() PNG IHDR check multiplies width*height*2
which can overflow; instead compute an effective_max_pixels = max_pixels / 2
when bytes[24] == 16 and use the existing guard(...) or comparison that checks w
and h against effective_max_pixels (rather than multiplying by 2). Locate the
IHDR branch that reads w/h from bytes and the bytes[24] == 16 condition, replace
the direct multiplication `@as`(u64, w) * `@as`(u64, h) * 2 with a call to the
existing guard(effective_max_pixels) or a comparison against
effective_max_pixels (where effective_max_pixels = max_pixels / 2) so you avoid
overflow and keep probe() consistent with codec_png.decode()'s 16-bpc behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ddc740c-073e-46d2-81de-6c1d64f9669e
📒 Files selected for processing (4)
src/runtime/image/Image.zigsrc/runtime/image/codec_png.zigsrc/runtime/image/codecs.zigtest/js/bun/image/image.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/image/image.test.ts`:
- Around line 646-673: The test currently only checks for the presence of an
iCCP chunk (hasIccp) but must validate the actual payload bytes; locate the iCCP
chunk in the produced out buffer (using dv/DataView and the existing
chunk-scanning loop), extract the chunk body bytes (the body length `len` at
that chunk), skip the keyword + 0x00 separator and the one compression-method
byte to get the compressed profile portion, inflate it with zlib.inflateSync and
compare the result to the original profile Uint8Array using an equality
assertion instead of just checking hasIccp; update the test around the while
loop/expect lines to perform this extraction, decompression, and expect equality
to `profile` (replace or augment the hasIccp check).
- Around line 641-644: The test currently asserts pngBitDepth(out) === 8 which
conflates indexed-PNG IHDR.bit_depth (palette index width) with per-channel
precision; instead modify the assertion in the "PNG 16 → png({palette})
downconverts to 8-bpc (quantise is u8-only)" test to verify the output is an
indexed PNG and not 16-bit per-sample: call the existing pngColorType(out)
helper and expect it to equal 3 (indexed palette), and assert pngBitDepth(out)
!== 16 (or assert it is one of 1,2,4,8) to ensure the encoder did not emit
16-bit samples while allowing 4-bit indices for 16 colors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffcdb655-57a6-4ce3-b997-0dccafc87dfe
📒 Files selected for processing (1)
test/js/bun/image/image.test.ts
There was a problem hiding this comment.
Thanks for addressing all four rounds of feedback — I have nothing further. Deferring to a human for final sign-off since this threads a new bit-depth contract through the codec pipeline and touches the decompression-bomb guard.
Extended reasoning...
Overview
This PR adds 16-bit-per-channel PNG preservation to Bun.Image. It introduces a bit_depth: u8 = 8 field on codecs.Decoded, a downconvertTo8() in-place narrowing helper, conditional SPNG_FMT_RGBA16 decode/encode in codec_png.zig, four downconvertTo8() call sites in Image.zig (placeholder, non-PNG encode, pipeline ops, EXIF orientation), a bit_depth parameter on codecs.encode/png.encode, and a halved maxPixels budget for 16-bpc sources in both codec_png.decode() and probe(). ~220 lines of new tests cover round-trip, downconversion on ops/encoders, iCCP interaction, the halving guard, and the hostile-IHDR overflow case.
Security risks
The PR modifies the decompression-bomb defence: 16-bpc PNGs now allocate 8 bytes/pixel, so max_pixels is halved for 16-bpc sources to keep the ~1 GiB byte cap. An earlier revision had a u64 overflow in probe() (w*h*2 on a hostile 25-byte IHDR → safety-checked panic on the JS thread); this was fixed in 4560487 to the overflow-safe w*h > max_pixels/2 form, with a regression test. The downconvertTo8 in-place narrowing reads u16 at byte offset 2i and writes u8 at offset i, so writes never overtake reads. No new injection/auth/data-exposure surface.
Level of scrutiny
This is a non-trivial feature change to a codec pipeline that processes untrusted input. It alters the internal buffer-format contract (RGBA8 → conditionally RGBA16), threads that through four call sites that must each downconvert before u8-only consumers, and modifies a security guard. The PR went through four rounds of review feedback (infallible-signature cleanup, 2× allocation guard, overflow fix, test-value tightening, comment corrections), all addressed. That iteration history itself argues for a maintainer's eye on the final state rather than bot-only approval.
Other factors
Test coverage is thorough (13 new cases including fail-before/pass-after verification and an 8-bpc control for the halving test). All my prior inline comments are resolved and no new issues were found on dbc60cb. CodeRabbit also reports clean. The remaining reason to defer is scope/criticality, not any outstanding concern.
|
CI summary for maintainers (build 53275 @ ✅ Diff is green. ❌ Remaining red — all unrelated to this diff (
All review threads resolved. I've already spent the one |
|
does this work for high bit depth HEIC and AVIF and TIFF too? |
|
@190n No — this PR is PNG-only. The other high-bit-depth-capable formats all hit a forced 8-bpc conversion before we own the pixels:
Preserving 10/12-bit HEIC + AVIF would need new OS-codec entry points that return RGBA16 (or RGBA10/12 packed), plus a third |
|
@robobun can you fix it properly? |
|
Pushed 0469f17 — extended 16-bpc decode through the CoreGraphics (macOS) and WIC (Windows) backends so HEIC 10/12-bit, AVIF 10/12-bit, and TIFF 16-bit all keep their precision through to PNG 16-bpc encode. CG (macOS): Added WIC (Windows): Retyped the Everything downstream is unchanged — Linux AVIF via libavif+dav1d (PR #30204) is still on a separate branch and not in main, so nothing to do there yet; when that lands the same pattern applies (ask libavif for Three new tests: build a minimal in-process 16-bpc TIFF and assert (1) it round-trips to 16-bpc PNG preserving low bytes, (2) an 8-bpc TIFF control stays 8-bpc — classifier doesn't upgrade unconditionally, (3)
|
|
CI update at sha 1be2728: ✅ The Windows ❌ Remaining red: one Windows 2019 x64-baseline shard fails on Diff is green across every lane that reports on it. Needs a maintainer to re-roll CI or merge. |
There was a problem hiding this comment.
All prior review comments are addressed and I found nothing further, but given the cross-platform native-code scope (CoreGraphics shim ABI change, WIC vtable/GUID classifier, pipeline bit-depth invariant) and the mid-review expansion you requested, this is one for a human to sign off on.
Extended reasoning...
Overview
This PR changes the Bun.Image pipeline to preserve 16-bit-per-channel precision through decode → encode when no u8-only operation intervenes. It touches 8 files: the libspng PNG codec (codec_png.zig), the shared codec layer (codecs.zig — new Decoded.bit_depth field, downconvertTo8() in-place narrowing, probe() and default_max_pixels guard adjustments), the pipeline driver (Image.zig — four new downconvertTo8() call sites gating u8-only consumers), the macOS CoreGraphics C++ shim (image_coregraphics_shim.cpp — new dlsym'd CFNumberGetValue/CFDictionaryGetValue/CGImageSourceCopyPropertiesAtIndex/kCGImagePropertyDepth, an extra ABI parameter on bun_coregraphics_decode, and a new 16-bpc vImage VFmt path), the macOS Zig wrapper (backend_coregraphics.zig), the Windows WIC backend (backend_wic.zig — GetPixelFormat vtable slot retyped, a 15-entry high-bpc GUID allowlist, 64bppRGBA convert target), the contributor README, and ~400 lines of tests including hand-rolled 16-bpc PNG and baseline-TIFF binary fixtures.
Security risks
The change is security-adjacent: it modifies the decompression-bomb maxPixels guard in three places (codec_png.decode, codecs.probe, and both system backends) to halve the effective pixel budget for 16-bpc sources. Earlier review rounds caught and fixed a u64 overflow in the probe() formulation that would have panicked Debug/ReleaseSafe builds on a 25-byte hostile IHDR, and a 2× regression in the worst-case allocation cap. Those are now fixed with regression tests, and the current formulation (w*h > max_pixels/2) is overflow-safe. downconvertTo8() does an align(1) u16 pointer-cast over a heap buffer and an in-place shrinking realloc; the buffer is always allocator-owned and even-length, so this looks correct, but it is the kind of pointer-reinterpretation a human should glance at. No injection, auth, or data-exposure surface.
Level of scrutiny
High. This is not a mechanical change: it rewrites a documented pipeline invariant ("RGBA8 everywhere" → "RGBA8 except the 16-bpc carry-through"), adds an ABI parameter to a C++↔Zig boundary, retypes a COM vtable slot, and depends on undocumented-in-tests behaviour of OS codecs (vImage 16-bpc widening, WIC's HEIF extension pixel-format reporting, WIC TIFF strictness on Server 2019). The scope was expanded mid-review at Jarred-Sumner's explicit request ("can you fix it properly?") from PNG-only to HEIC/AVIF/TIFF across CG and WIC, and that expansion briefly broke all three Windows CI lanes before being fixed. The README now publicly promises "iPhone HEIC 10-bit → PNG 16" on Windows, which is untested against a real HEIC fixture (only synthetic TIFF is exercised).
Other factors
Test coverage is thorough for the PNG path (11 cases including round-trip byte-equality, downconvert-on-op, iCCP payload survival, the maxPixels halving with an 8-bpc control, and the hostile-IHDR overflow regression). System-backend coverage is weaker by necessity — the TIFF tests pass-vacuously on hosts where WIC rejects the hand-rolled fixture, and HEIC/AVIF 10-bit is untested. CI at 1be2728 shows image.test.ts green on all lanes; remaining failures (s3, http-abort, hot.test) are unrelated. All ~12 inline review comments from me and CodeRabbit across six rounds have been addressed and resolved. Given a maintainer was actively shaping scope here, they should review the final result rather than have it bot-approved.
|
CI at sha d6440bd: same WTF I already used my one Final state of the diff:
Needs a maintainer to re-roll the ASAN lane or merge. |
d6440bd to
6699303
Compare
|
Rebased onto main, which migrated the repo from Zig to Rust since this PR branched. The conflict resolution was effectively a rewrite:
Verification on the rebase: full image suite green on Linux (106 pass / 5 skip), adversarial + kernels + sharp-comparison suites green (127 pass), |
|
CI triage for build 60945 (sha 6699303, the Rust port), final state now that the build has finished:
No retrigger pushed: the bunx failure is deterministic registry state and will reproduce on every run until #31818 or #31820 lands. This PR is ready for review; CI should go green once one of those merges. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/image_coregraphics_shim.cpp:208-209— These two newly-added comments cross-referencesrc/runtime/image/codecs.zig(line 208) andsrc/runtime/image/codec_png.zig(line 353), but both files were deleted in the Zig→Rust migration this PR was rebased across — the referencedDecodeddoc and the PNG halving now live incodecs.rsandcodec_png.rs. Should be.rsin both places.Extended reasoning...
What the issue is
Two comments this PR adds to
src/jsc/bindings/image_coregraphics_shim.cppreference Zig source files that no longer exist in the tree:- Line 208 (comment on
kBunCGBitmapByteOrder16Host): "See src/runtime/image/codecs.zig's Decoded doc." - Line 353 (comment on
effective_max_pixelshalving): "same as the PNG halving in src/runtime/image/codec_png.zig."
ls src/runtime/image/*.zigreturns nothing — the .zig files were deleted during the repo-wide Zig→Rust migration that landed on main between this PR's original May branch point and its June 6 rebase. TheDecodedstruct doc-comment and the max_pixels-halving logic these comments point at now live insrc/runtime/image/codecs.rs(thebit_depthfield doc at ~line 220) andsrc/runtime/image/codec_png.rs(theeffective_max_pixelsblock at ~line 140) respectively.Why this happened
The PR's own timeline explains the mismatch. Per the 2026-06-06 rebase comment:
The 9 Zig-era commits patched
src/runtime/image/*.zig, which are now dead porting-reference files on main. Those edits are dropped; the.zigfiles are untouched relative to main. The whole fix is ported 1:1 to the Rust pipeline instead …image_coregraphics_shim.cppchanges … re-applied; this file survived the migration with only minor edits.So the C++ shim's edits were carried over from the Zig-era commits with the comment text unchanged, while the Rust files they now describe were written fresh — leaving the cross-references pointing at the pre-migration paths. Since then main has removed the .zig porting-reference files entirely, so the paths now resolve to nothing at all.
Step-by-step proof
grep -n '\.zig' src/jsc/bindings/image_coregraphics_shim.cppyields exactly two hits: line 208 (codecs.zig) and line 353 (codec_png.zig). Both are in+hunks of this PR's diff — they are new lines, not pre-existing.ls src/runtime/image/*.zig→ no matches. The referenced files do not exist on the current tree.- The content the comments describe does exist, in the .rs equivalents:
src/runtime/image/codecs.rsdefinesstruct Decodedwith thebit_depth: u8field and its host-endian-u16 doc-comment — exactly what line 208 wants the reader to look at.src/runtime/image/codec_png.rs::decode()computeseffective_max_pixels = if ihdr.bit_depth == 16 { max_pixels / 2 } else { max_pixels }— the "PNG halving" line 353 refers to.
- The PR's own June-6 rebase comment states the .zig files are "dead porting-reference files" and that the fix was ported to
codecs.rs/codec_png.rs, confirming the .rs paths are the intended targets.
Why existing code doesn't prevent it
Nothing enforces that comment cross-references resolve to real files; the C++ compiler doesn't see them and there's no lint for it. The shim was one of the few files that survived the migration mostly-unchanged, so its comments were re-applied verbatim from the pre-rebase commits without the
.zig → .rssubstitution the rest of the port received.Impact
Documentation only — zero runtime impact. A reader following either "See …" reference lands on a nonexistent path and has to grep for the .rs equivalent themselves. Flagging because REVIEW.md's "Only comment what the code cannot say" section covers comment accuracy, and this PR has consistently accepted and fixed comment-accuracy nits of exactly this kind (resolved threads on the codecs.zig doc wording, the WIC comment tightening, and the TIFF inline-value prose).
How to fix
- // without a byte swap. See src/runtime/image/codecs.zig's Decoded doc. + // without a byte swap. See src/runtime/image/codecs.rs's Decoded doc.
- // halving in src/runtime/image/codec_png.zig. + // halving in src/runtime/image/codec_png.rs.
- Line 208 (comment on
|
Both stale .zig cross-references were already fixed in f4fd484 (the comment-trim commit): the shim now points at src/runtime/image/codecs.rs (line 197) and src/runtime/image/codec_png.rs (line 327), and |
|
CI state at f4fd484 (build 96265): the only failure is |
There was a problem hiding this comment.
I reviewed this PR after 49fbe79 and didn't find any new bugs — the grayscale WIC GUIDs and test were added as suggested, and all earlier threads are resolved. Given the scope (three-backend FFI/ABI change to bun_coregraphics_decode, new WIC vtable slot + 20-GUID classifier, a new bit_depth invariant threaded through every decode/encode/op site), a human sign-off is still worthwhile.
What was reviewed:
downconvert_to_8call-site coverage — every u8-only consumer (ops, apply_orientation, ThumbHash, non-PNG/indexed encode) narrows before use.- The halved
max_pixelsguard is applied consistently acrosscodec_png::decode,probe(), the CG shim, and WIC, using the overflow-safew*h > cap/2form. - CG shim: TOCTOU guard extended to
bit_depth;kCGBitmapByteOrder16Hostmatches libspng's host-endian RGBA16; non-Apple stub arity updated. - WIC:
GUID_WICPixelFormat64bppRGBAis the straight-alpha0x16variant;GetPixelFormatvtable slot position matchesIWICBitmapSource.
Extended reasoning...
Overview
This PR adds 16-bit-per-channel preservation to Bun.Image: libspng decode/encode now honours SPNG_FMT_RGBA16 for 16-bpc IHDRs, codecs::Decoded gains a bit_depth field with a downconvert_to_8() narrowing helper, and both system backends (macOS CoreGraphics via the C++ shim, Windows WIC) were extended to emit RGBA16 for high-bit-depth TIFF/HEIC/AVIF sources. The pipeline in Image.rs narrows to 8 bpc before any u8-only op (rotate/flip/resize/modulate), before EXIF auto-orient, before ThumbHash, and before any non-PNG-truecolour encode. ~400 lines of new tests cover the PNG round-trip, every downconvert site, the halved max-pixels guard, and hand-rolled TIFF fixtures for the CG/WIC paths. Twelve files touched.
Security risks
The change is security-adjacent because it doubles bytes-per-pixel for a class of inputs: an unchanged pixel-count guard would let a hostile 16-bpc IHDR trigger ~2× the previous worst-case allocation. This was raised in an earlier round and is now addressed uniformly — codec_png::decode, codecs::probe(), the CG shim, and WIC all halve the effective pixel budget for 16-bpc sources, using w*h > max_pixels/2 (two u32 factors, no overflow) rather than w*h*2 > max_pixels. The CG shim's phase-1/phase-2 TOCTOU guard was extended to compare bit_depth as well as dimensions, so a mid-decode swap from 8→16 bpc can't overflow the caller-sized buffer. No new attack surface beyond that.
Level of scrutiny
High. This is a cross-cutting invariant change with platform-specific FFI on all three OSes: a new parameter on the bun_coregraphics_decode C ABI (with the non-Apple stub updated in lockstep), a newly-typed COM vtable slot (GetPixelFormat), a 20-entry hand-transcribed WIC GUID allowlist, and dlsym'd CoreFoundation/ImageIO symbols. Every existing decode/encode/op path now has an implicit bit_depth precondition. The failure modes for a missed downconvert_to_8() call site are buffer-size mismatches into C image kernels. That warrants a maintainer's eyes even with no automated findings.
Other factors
The PR has been through many review rounds — I raised roughly a dozen issues over prior iterations (unreachable error paths, the 2× allocation guard, a u64 overflow in probe(), vacuous test constants, stale README/doc comments, WIC GUID label/value mismatches, missing R10G10B10A2 and grayscale GUIDs, Windows CI failures, the runTiffTest vacuous-pass reporting) and all are resolved in the current diff. Two maintainers (190n, Jarred-Sumner) have engaged on direction but neither has approved. CI at f4fd484 was green on the image suite across all lanes; the sole failure was an unrelated bake/deinitialization.test.ts crash on Windows also present on main. One comment-cop bot flag on backend_wic.rs:833 (comment length) remains open but is cosmetic. Given the size and platform reach, deferring for human approval rather than auto-approving.
|
CI at 49fbe79 (build 96345): 177 jobs passed, zero test failures. The build is marked failed only because the darwin 14 aarch64 test jobs expired in the runner queue three times (original plus two auto-retries) without ever starting; no code ran on that lane. The image suite, including the new 16-bpc grayscale TIFF test, is green on every lane that executed it, and the same suite passed on darwin 14 x64 in the previous build. Ready for review. |
3 participants
Closes #30462.
Repro
Expected 16-bit sRGB; got 8-bit. The low byte of every channel is lost even though no user op runs.
Cause
src/runtime/image/codec_png.zighard-codes the round-trip at 8 bpc:spng_decode_image(..., SPNG_FMT_RGBA8, ...)— libspng down-converts 16-bpc samples during decode..bit_depth = 8in the IHDR.codecs.Decoded.rgba: []u8is the only intermediate container, so every codec and every op is built around 8 bpc. The pass-through path had no hole in it to let a 16-bpc buffer survive.Fix
Three small changes keep the PNG → PNG pass-through at full precision and narrow at the latest possible moment elsewhere:
codecs.Decodedgains abit_depth: u8 = 8field plus adownconvertTo8helper that narrows the u16 channels to their high byte in place (same convention as libpngpng_set_strip_16, libvips). Every other decoder / backend constructsDecodedwithout the field and gets 8 by default — no call-site churn.codec_png.decodereads the source IHDR and asks libspng forSPNG_FMT_RGBA16whenihdr.bit_depth == 16. The buffer is 8 bytes/pixel, host-endian — libspng does the wire-format byte swap in both directions.codec_png.encodetakesbit_depthand writes it to the IHDR.SPNG_FMT_PNGwithbit_depth == 16sets libspng'sto_bigendianflag so the host-endian u16 buffer round-trips cleanly.Image.zigcallsdownconvertTo8before any u8-only consumer touches the buffer: any pipeline op (resize/rotate/flip/flop/modulate),applyOrientation, the.placeholder()ThumbHash path, and any non-PNG or indexed-PNG encode. That keeps the single lossless path — PNG in, PNG out, no ops — at 16 bpc and makes every other path explicit about where precision is lost.Tests
test/js/bun/image/image.test.tsgets a newdescribe("16-bit-per-channel PNG (issue #30462)", ...)block with 11 cases covering:.png()and bare.bytes(), plus.write(path)on disk)Fail-before/pass-after verified by stashing
src/:Full image suite remains green on Linux (104 pass, 2 system-backend skips).
no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/image/image.test.ts