Add Bun.Image clone() and stats() - #32123
Conversation
clone() snapshots an Image into a new independent pipeline sharing the same input bytes (Arc for owned sources, the same ArrayBuffer for JS buffer sources) with a copy of the recorded operations, so one upload can fan out into several variants. Concurrent pipelines in a clone family whose decode is full resolution (stats, placeholder, transcodes, any non-JPEG source) dedupe the decode through a keyed weak cache; hinted JPEG decodes keep the per-pipeline IDCT shrink-on-load path, which benchmarks faster than sharing a full-resolution decode. stats() computes Sharp-shaped pixel statistics of the source image on the worker: per-channel min/max/sum/squaresSum/mean/stdev with min/max positions, isOpaque, greyscale entropy, laplacian sharpness, and the dominant sRGB colour from a 4096-bin histogram. Also gates the JPEG decode hint to encode pipelines: a recorded resize previously made placeholder() (and would have made stats()) run on an IDCT-downscaled decode instead of the source.
|
Updated 11:41 AM PT - Jun 11th, 2026
✅ @robobun, your commit c72c6781660d1ee799a0b963b05ff315f649770d passed in 🧪 To try this PR locally: bunx bun-pr 32123That installs a local version of the PR into your bun-32123 --bun |
|
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 Bun.Image.clone() to snapshot pipelines sharing input/decodes, and Bun.Image.stats() to decode the source image and return per-channel aggregates, opacity, entropy, sharpness, and dominant sRGB. Shared-decode uses an atomic family counter and guarded keyed cache; stats() is a new worker terminal that ignores recorded ops. ChangesBun.Image.clone() and Bun.Image.stats() with decode sharing
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/image/Image.rs (1)
363-372:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't charge shared
Arcinput bytes to every clone.
Source::Ownedis nowArc<Vec<u8>>, butestimated_size()still addsb.len()for every wrapper. A fan-out clone family will therefore report the same encoded input N times to the GC/native-memory accounting even though only one buffer exists, which can artificially increase memory pressure on the exact clone-heavy path this PR adds.🤖 Prompt for 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. In `@src/runtime/image/Image.rs` around lines 363 - 372, estimated_size() currently charges the full Vec<u8> length for every Image clone because Source::Owned is now an Arc<Vec<u8>>; change the accounting in the Source::Owned arm to avoid double-counting shared Arc buffers by using Arc::strong_count to either (a) only charge the bytes when the Arc is unique (if Arc::strong_count(&b) == 1 then b.len() else 0) or (b) split the charge across clones (b.len() / Arc::strong_count(&b) as usize) depending on desired semantics; update the Source::Owned branch in the estimated_size() function to use Arc::strong_count(&b) and compute the adjusted byte charge instead of always adding b.len().
🤖 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/Image.rs`:
- Around line 686-696: When cloning Image you must not copy the runtime cache
last_width/last_height into the new instance; change the Box::new(Image { ...
last_width: Cell::new(self.last_width.get()), last_height:
Cell::new(self.last_height.get()), ... }) initialization to reset those caches
(initialize to an empty/default state rather than copying the parent's values)
and additionally ensure any code paths that mutate the pipeline (places that
call pipeline.set(...) or otherwise change self.pipeline.get())
clear/invalidates the Image cache fields (last_width, last_height) on the
affected instance(s) so cached terminal dimensions are not reused after pipeline
changes.
---
Outside diff comments:
In `@src/runtime/image/Image.rs`:
- Around line 363-372: estimated_size() currently charges the full Vec<u8>
length for every Image clone because Source::Owned is now an Arc<Vec<u8>>;
change the accounting in the Source::Owned arm to avoid double-counting shared
Arc buffers by using Arc::strong_count to either (a) only charge the bytes when
the Arc is unique (if Arc::strong_count(&b) == 1 then b.len() else 0) or (b)
split the charge across clones (b.len() / Arc::strong_count(&b) as usize)
depending on desired semantics; update the Source::Owned branch in the
estimated_size() function to use Arc::strong_count(&b) and compute the adjusted
byte charge instead of always adding b.len().
🪄 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: d19fb116-96e6-4cde-a765-51ee84f04e8b
📒 Files selected for processing (5)
docs/runtime/image.mdxpackages/bun-types/bun.d.tssrc/runtime/image/Image.classes.tssrc/runtime/image/Image.rstest/js/bun/image/image.test.ts
width/height are documented as -1 until the instance's first awaited terminal; a clone starts fresh instead of inheriting the parent's last terminal output. estimated_size now divides the Arc-shared input charge by the family size so N clones report ~one buffer, not N.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/image/Image.rs (2)
1378-1403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnwind
pending_tasksand reclaim the handler whenread_bytes_to_handler()fails.This path increments
pending_tasks, upgradesthis_ref, and leakschainintorawbefore the ownership transfer. Ifread_bytes_to_handler()returnsErr, none of that is unwound, so the wrapper stays artificially alive and the handler allocation is leaked. Arm a local guard and only disarm it after the read handler has been successfully registered. As per coding guidelines, pair every acquisition with its release at the acquisition site using Drop/RAII guards.🤖 Prompt for 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. In `@src/runtime/image/Image.rs` around lines 1378 - 1403, Wrap the acquisition sequence (checking/incrementing image.pending_tasks, setting image.this_ref strong, allocating Box<BlobReadChain> and calling bun_core::heap::into_raw) in a small RAII guard that, on Drop, will revert those actions if registration fails: it should decrement image.pending_tasks, clear the strong this_ref if the guard performed the upgrade, and if a raw pointer was produced reconstruct Box::from_raw and drop it to free the handler; only disarm the guard after blob.read_bytes_to_handler(...) returns Ok (i.e. after successful registration). Apply this around the code that touches image.pending_tasks, image.this_ref.with_mut, BlobReadChain construction, and bun_core::heap::into_raw so failure from blob.read_bytes_to_handler does not leak or leave the wrapper strongly held.Source: Coding guidelines
1424-1435:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce
MAX_INPUT_FILE_BYTESat the actual read boundary, not just before it.The size cap is bypassable in both byte-materialization paths here:
Source::Blobaccepts anyReadBytesResult::Ok(bytes)without a length check, soBun.file(...).image()and other blob-backed sources can still materialize arbitrarily large encoded inputs;Source::Pathonly checksst_sizebeforeread_to_end(), so a concurrently-growing regular file can pass the check and still read past the cap. That defeats the “bound encoded bytes before decode” invariant and reopens OOM/DoS on large inputs. Move the cap into the actual read path: use a bounded file read loop for paths and reject oversize blob reads before storing them, or push both through one capped helper. As per coding guidelines, validate untrusted input before allocation/side effects and fix the whole bug class in the same PR.Also applies to: 1769-1796
🤖 Prompt for 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. In `@src/runtime/image/Image.rs` around lines 1424 - 1435, The code allows bypassing MAX_INPUT_FILE_BYTES because ReadBytesResult::Ok(bytes) is accepted unconditionally for Source::Blob and Path reads check st_size before read; fix by enforcing the cap at the actual read boundary: in the match arm handling ReadBytesResult::Ok(bytes) (and the other identical arm around lines 1769-1796) reject or return an error when bytes.len() > MAX_INPUT_FILE_BYTES before calling image.source.set(Source::Owned(...)); for Source::Path replace the single read_to_end() with a bounded read loop (or a shared helper like read_capped(path_or_stream, MAX_INPUT_FILE_BYTES)) that stops and errors if the cap is exceeded, ensuring all materialisation paths validate untrusted input size before allocation or storing in image.source.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/image/Image.rs`:
- Around line 1378-1403: Wrap the acquisition sequence (checking/incrementing
image.pending_tasks, setting image.this_ref strong, allocating
Box<BlobReadChain> and calling bun_core::heap::into_raw) in a small RAII guard
that, on Drop, will revert those actions if registration fails: it should
decrement image.pending_tasks, clear the strong this_ref if the guard performed
the upgrade, and if a raw pointer was produced reconstruct Box::from_raw and
drop it to free the handler; only disarm the guard after
blob.read_bytes_to_handler(...) returns Ok (i.e. after successful registration).
Apply this around the code that touches image.pending_tasks,
image.this_ref.with_mut, BlobReadChain construction, and
bun_core::heap::into_raw so failure from blob.read_bytes_to_handler does not
leak or leave the wrapper strongly held.
- Around line 1424-1435: The code allows bypassing MAX_INPUT_FILE_BYTES because
ReadBytesResult::Ok(bytes) is accepted unconditionally for Source::Blob and Path
reads check st_size before read; fix by enforcing the cap at the actual read
boundary: in the match arm handling ReadBytesResult::Ok(bytes) (and the other
identical arm around lines 1769-1796) reject or return an error when bytes.len()
> MAX_INPUT_FILE_BYTES before calling image.source.set(Source::Owned(...)); for
Source::Path replace the single read_to_end() with a bounded read loop (or a
shared helper like read_capped(path_or_stream, MAX_INPUT_FILE_BYTES)) that stops
and errors if the cap is exceeded, ensuring all materialisation paths validate
untrusted input size before allocation or storing in image.source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28c2796e-c201-4675-b483-3129f36049d4
📒 Files selected for processing (2)
src/runtime/image/Image.rstest/js/bun/image/image.test.ts
encode_for_body wraps its PipelineTask in ManuallyDrop (its Drop would underflow pending_tasks) and releases owned fields manually, but the SharedDecode Arc added for decode sharing was not released there, so every Image-as-Response-body encode leaked one ArcInner (caught by LeakSanitizer on the x64-asan lane). Fold the share flag and the Arc into one Option field: schedule() passes Some only when the family has clones, and the ManuallyDrop path carries None so there is nothing to leak.
|
On the two outside-diff review findings (no inline threads to resolve, so replying here):
The LeakSanitizer failure on the x64-asan lane was real and mine: encode_for_body's ManuallyDrop task never released the new SharedDecode Arc, leaking one ArcInner per Image-used-as-Response-body. Fixed in c9b0a6d by folding the share flag and Arc into one Option field that is None on that path; verified locally with ASAN_OPTIONS=detect_leaks=1 (4 leak reports before, 0 after, full suite green). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/image/Image.rs (2)
2364-2369:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
stats()adds a full extra image-sized allocation just for sharpness.Line 2364 materializes an
n-byte luma plane so Lines 2440-2465 can make a second pass. At the defaultmaxPixelsceiling, that is another ~256 MiB on top of the already-decoded RGBA buffer, sostats()can OOM inputs that the rest of the pipeline still handles. Sharpness only needs a rolling 3-row window; keeping this O(width) instead of O(pixels) would avoid the new high-water mark.As per coding guidelines, "Count the copies and allocations your native code makes."
Also applies to: 2440-2465
🤖 Prompt for 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. In `@src/runtime/image/Image.rs` around lines 2364 - 2369, The stats() function currently allocates a full-image luma Vec<u8> (variable luma) for the sharpness Laplacian pass, which can OOM; change it to a rolling 3-row buffer of size width (O(width)) reused across the pass instead of allocating n bytes. Specifically, replace the full-image allocation and uses of luma in the sharpness computation (the block that reads/constructs luma and the loop in the sharpness Laplacian pass around lines 2440-2465) with three Vec<u8> row buffers (or a single Vec of length 3*width) that are try_reserve_exact'ed to width, rotate/reuse them as you scan rows, and update index arithmetic so the Laplacian reads from the three current row buffers rather than from luma at pixel-offsets. Ensure error handling still returns codecs::Error::OutOfMemory on reserve failures and preserve the same sharpness output semantics.Source: Coding guidelines
2356-2438:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
stats().dominantis not sRGB with non-sRGB inputs.Line 2356 bins the raw decoded RGB values, but this file explicitly preserves the source ICC profile instead of color-converting decoded RGBA first (see Lines 1936-1945). For Display-P3 / AdobeRGB assets,
stats().dominanttherefore comes back in source space, while the new public contract inpackages/bun-types/bun.d.tsdescribes an sRGB color and even shows feeding it into CSSrgb(...). Either convert to sRGB before histogramming or narrow the new runtime/docs contract before merge.
🤖 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.
Outside diff comments:
In `@src/runtime/image/Image.rs`:
- Around line 2364-2369: The stats() function currently allocates a full-image
luma Vec<u8> (variable luma) for the sharpness Laplacian pass, which can OOM;
change it to a rolling 3-row buffer of size width (O(width)) reused across the
pass instead of allocating n bytes. Specifically, replace the full-image
allocation and uses of luma in the sharpness computation (the block that
reads/constructs luma and the loop in the sharpness Laplacian pass around lines
2440-2465) with three Vec<u8> row buffers (or a single Vec of length 3*width)
that are try_reserve_exact'ed to width, rotate/reuse them as you scan rows, and
update index arithmetic so the Laplacian reads from the three current row
buffers rather than from luma at pixel-offsets. Ensure error handling still
returns codecs::Error::OutOfMemory on reserve failures and preserve the same
sharpness output semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 929f7c24-f6e0-4b6e-be75-5cb6bdeab02e
📒 Files selected for processing (1)
src/runtime/image/Image.rs
The sharpness Laplacian previously materialized a full image-sized luma plane, adding 25% to stats()'s peak memory on top of the decoded RGBA. Luma rows are now recomputed into a 3-row rolling window. Also drop the sRGB claim from the dominant-colour docs: statistics are computed over the decoded pixels, which stay in the source colour space because the pipeline preserves the ICC profile instead of converting (Sharp converts via vips' colour engine first; Bun does not ship one). sRGB sources, the overwhelming default, are unaffected.
|
Both outside-diff findings from the latest review round are addressed in 4e8f999: the sharpness pass now uses a 3-row rolling luma window instead of a full image-sized plane (O(width) extra memory), and the dominant-colour contract no longer claims sRGB — stats are documented as computed in the image's own colour space, since the pipeline preserves the ICC profile instead of converting (Bun ships no colour engine; Sharp converts through vips first). Sharpness output is bit-identical, all 243 image tests green. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-types/bun.d.ts (1)
8519-8524:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
Image.stats()docs withImage.Stats.dominantcolor-space contract.Line 8523 still says “
dominantsRGB colour”, butImage.Stats.dominantnow documents source-color-space output. This creates conflicting API semantics in the same.d.ts.Suggested doc fix
- * `isOpaque`, greyscale `entropy` and `sharpness` estimates, and the - * `dominant` sRGB colour — e.g. for placeholder backgrounds. + * `isOpaque`, greyscale `entropy` and `sharpness` estimates, and the + * `dominant` colour in the image's own colour space — e.g. for + * placeholder backgrounds.🤖 Prompt for 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. In `@packages/bun-types/bun.d.ts` around lines 8519 - 8524, Update the JSDoc for Image.stats() to match the Image.Stats.dominant contract: remove or replace the phrase "dominant sRGB colour" and state that the `dominant` property is reported in the image's source color space (same behavior as Image.Stats.dominant). Locate the Image.stats() documentation block and change its wording so it no longer asserts sRGB output and instead explicitly documents that `dominant` uses the source color space, keeping the rest of the stats description unchanged.
🤖 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.
Outside diff comments:
In `@packages/bun-types/bun.d.ts`:
- Around line 8519-8524: Update the JSDoc for Image.stats() to match the
Image.Stats.dominant contract: remove or replace the phrase "dominant sRGB
colour" and state that the `dominant` property is reported in the image's source
color space (same behavior as Image.Stats.dominant). Locate the Image.stats()
documentation block and change its wording so it no longer asserts sRGB output
and instead explicitly documents that `dominant` uses the source color space,
keeping the rest of the stats description unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3ed6fcd-9123-476a-9517-65c92e8b5394
📒 Files selected for processing (3)
docs/runtime/image.mdxpackages/bun-types/bun.d.tssrc/runtime/image/Image.rs
|
The triage reply on #32122 contains two inaccuracies worth correcting while this PR is open:
Of those, |
|
You're right on both points, thanks for the careful read. Corrected the triage comment on #32122 and this PR's scope section: #31899 covers only Keeping this PR scoped to clone() + stats(): it's through several review rounds and CI is close to green, and each of the remaining items is self-contained enough to land as its own focused PR against #32122. The issue now reflects accurately which items remain open. |
|
Agreed on keeping this PR scoped. Since you've sized items 3, 4, and 6 as self-contained, opening those focused PRs against #32122 would close out the issue: |
|
Sizing notes for whoever picks these up (with one correction to my earlier "self-contained" claim):
On opening them now: there are currently 8 open Bun.Image PRs from this account awaiting review (this one plus #31898, #31899, #30616, #30204, #30463, #30237, #30202). The bottleneck is review bandwidth, not PR production, so I'd rather not stack three more onto that queue unprompted. If a maintainer wants any of these next, say the word and it gets a focused PR. |
2 participants
Implements the two top-ranked items from #32122:
Bun.Image#clone()andBun.Image#stats().clone()Sharp-style snapshot: a new independent
Imagesharing the same input with a copy of the operations recorded so far. Input sharing is zero-copy:Source::OwnedbecomesArc<Vec<u8>>, JS-buffer sources point the clone'ssourceJSslot at the same ArrayBuffer, Blob sources take a newStrongon the same Blob.Decode sharing: images in a clone family share a
SharedDecode(Arc), whose keyedWeakcache dedupes concurrent full-resolution decodes. The first task decodes while siblings block on the lock, then everyone reuses the pixels; the cache key is wyhash(input bytes) + backend/maxPixels/autoOrient, so a mutated source buffer or aBun.Image.backendflip re-decodes instead of serving stale pixels. TheWeakmeans an idle family pins no pixel memory.Sharing engages only where it cannot change the decode a task would do anyway:
stats(),placeholder(), transcodes, and every non-JPEG source (their decoders take no hint). JPEG pipelines with aresizekeep today's per-pipeline IDCT shrink-on-load decode, untouched. I first tried sharing one full-resolution decode across all variants and benchmarked it slower for the flagship workload (12 MP JPEG into 7 variants: 12.3s vs 7.8s on a debug build), because resizing every variant from the full frame costs more than the saved decodes; Sharp/libvips also shrink-on-load per pipeline rather than share one full decode. With the final gating, the 7-variant JPEG fan-out is identical with clones vs fresh instances, and a stats+placeholder+transcode cluster saves one full decode per extra member (~1.4s CPU on the same debug build, wall time neutral on an idle pool).Images that never call
clone()take exactly the old path: no hashing, no locking, no copy.stats()Same shape as Sharp's
stats(): per-channelmin/max/sum/squaresSum/mean/stdev(sample stdev, n-1 like vips) with min/max positions,isOpaque, greyscaleentropy(Shannon, 256-bin) andsharpness(stdev of a 3x3 Laplacian) estimates, anddominantfrom the same 4096-bin (16^3) RGB histogram algorithm Sharp uses. Computed in one pass on the worker over the decoded RGBA; stats describe the source image (recorded ops are ignored, likeplaceholder()). Channels are always the 4 of the RGBA pixel model every decode emits; alpha-less sources report a constant-255 alpha channel.Bug fix ridden in on purpose
The JPEG decode hint (IDCT downscale) was computed from
pipeline.resizeregardless of terminal kind, sonew Bun.Image(jpeg).resize(10, 10).placeholder()computed the placeholder from a 1/8-scale decode, andstats()would have inherited the same defect (wrong means, wrong min/max positions,width/heightreporting the downscaled dims). The hint is now only applied to encode pipelines;test("a recorded resize doesn't shrink the decode stats run on")covers it.Not in this PR
Scope stays clone() + stats(). Of the remaining #32122 items:
fit: "cover"/"contain"/"outside"are in #30616 andpositionanchors in #31502; AVIF on Linux in #30199/#30204. #31899 covers only part of item 5 (space/channels/hasAlpha); frame/page count, bit depth, EXIF orientation value, and ICC profile presence have no open PR. Items 3 (explicitcrop()/position: "attention"), 4 (encodereffort/chromaSubsampling), and 6 (timeout()) also have no coverage and remain open on the issue; each is self-contained and suited to a focused follow-up.Verification
test/js/bun/image/image.test.ts(clone independence both directions, fan-out, path/BunFile/detached sources, option snapshotting, exact-value stats math recomputed in the test, dominant/entropy/sharpness edge cases, the IDCT-hint regression). All fail on the unfixed build (img.clone is not a function), all pass with the fix.Bun.gc(true), clean under ASAN.cargo clippy -p bun_runtimeclean; types validated bytest/integration/bun-types.Fixes the
clone()andstats()items of #32122.