[repro] Fix resolveFit contain/scale-down padding regression (upstream #14714)#2
Open
allocsys wants to merge 61 commits into
Open
[repro] Fix resolveFit contain/scale-down padding regression (upstream #14714)#2allocsys wants to merge 61 commits into
allocsys wants to merge 61 commits into
Conversation
…path Previously runTransform() hardcoded sharp's fit: "contain" (which letterboxes with black bars) for every resize, ignoring any fit, gravity, or background the caller passed on the transform. Wire up the same resolveFit/resolveGravity helpers already used for the cf.image fetch path, and forward background (defaulting to white for "pad", matching production). Fixes cloudflare#14695
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…dflare configuration (cloudflare#14534)
…loudflare/src/frameworks (cloudflare#14759) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…ace paths (Windows) (cloudflare#14713) Co-authored-by: dumbCodesOnly <225476909+dumbCodesOnly@users.noreply.github.com> Co-authored-by: Pete Bacon Darwin <pbacondarwin@cloudflare.com> Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…lity inputs in `unstable_startWorker` (cloudflare#14617)
…flare#14448) Co-authored-by: Dario Piotrowicz <dario@cloudflare.com>
…/frameworks (cloudflare#14760) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…lare#14688) Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…andlers (cloudflare#14763) Co-authored-by: Edmund Hung <edmund@cloudflare.com>
…ing (cloudflare#14746) Signed-off-by: Sam Agarwal <samarthagrawal526@gmail.com>
Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com> Co-authored-by: Ben Dixon <ben@nuro.dev>
…cloudflare#14773) Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com>
Co-authored-by: James Opstad <13586373+jamesopstad@users.noreply.github.com>
…-cloudflare/src/frameworks (cloudflare#14659) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…dates (cloudflare#14715) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…contain"), not "inside" Fixes image snapshot regression (expected 128x100, got 56x100) in get-platform-proxy Image bindings test. Cloudflare Images "contain" and the default "scale-down" fit both pad to the exact requested box with letterboxing, matching Sharp's "contain" fit - not "inside", which only constrains within the box while preserving aspect ratio without padding.
The previous fix conflated two different APIs that share this file: - Images binding (env.IMAGES.transform()): explicit fit:"contain" pads to the exact box (letterbox), but default/unspecified fit and "scale-down" must NOT pad - confirmed by transform.spec.ts. - cf.image (fetch cf.image option): fit:"contain" does NOT pad, per cf-image.spec.ts "fit:contain preserves aspect ratio". Introduces resolveImagesBindingFit() (contain -> pad, default/scale-down -> inside/no pad) used only by runTransform(), and keeps the original resolveFit() (contain -> inside, default/scale-down -> inside) for applyCfImageTransforms(), instead of one shared function.
…one line (verified with oxfmt locally against .oxfmtrc.jsonc)
…m-proxy Image bindings snapshot and upload it as an artifact for manual review
, c7b3219) to this branch's copy: (1) run through turbo/test:ci instead of calling vitest directly so wrangler's dist gets built first, (2) add --update flag so jest-image-snapshot actually writes a new baseline despite CI detection via is-ci/GITHUB_ACTIONS.
…ded, matches production-correct default/scale-down fit behavior after resolveFit/resolveImagesBindingFit split)
… runTransform() passed the raw Sharp Duplex stream directly into `new Response(transformer, ...)`, unlike cfImageLocalFetcher which explicitly buffers via .toBuffer() first. This pre-existing pattern (present before this PR's changes) apparently produced incomplete/corrupted output in some environments, only exposed by this PR's own new pixel-level regression tests (transform.spec.ts), which failed with Sharp "Input buffer contains unsupported image format" when re-parsing the response. Buffering explicitly, matching the already-working cf.image code path, fixes it.
Snapshot has been regenerated and committed; this throwaway workflow_dispatch job is no longer needed and was blocking Validate PR Description (new workflow files require justification).
All 6 tests were failing with "Input buffer contains unsupported image format" when decoding the transform response - the response body was actually a plain-text binding error, not image bytes. Passing the raw request.body ReadableStream into input() inside a dispatchFetch-invoked worker doesn't round-trip correctly through the local Images binding loopback; buffering to an ArrayBuffer first (as done implicitly by the passing getPlatformProxy-based fixture test, which reads via RPC rather than a live stream) avoids this.
Previous buffering fix (5fd60db) did not resolve the failures - still "Input buffer contains unsupported image format" on all 6 tests. Adding a status/text dump so we can see what's actually coming back instead of guessing again.
Found it via debug logging: the transform response was a 500 with "Error: To use the new ReadableStream() constructor, enable the streams_enable_constructors compatibility flag" - not corrupted image bytes as previously assumed. The Miniflare instance in this test never set compatibilityDate, so it defaulted to a date before that flag is implied. Sibling test images/index.spec.ts already uses compatibilityDate: "2025-04-01" for the same binding - match it here. Reverts the unnecessary request.arrayBuffer() buffering from 5fd60db (request.body streaming was never the problem) and removes the debug logging from cb65af1, now that the actual cause is confirmed.
…ensions (50x25, 2:1) had matching aspect ratios, so fit:cover needed zero cropping and gravity was never actually exercised - gravity:top passed by coincidence, gravity:bottom correctly failed. Use width:100 height:25 (4:1) to force vertical cropping so gravity has an observable effect.
…tom-crop's first row exactly on the white/red seam (original row 50), so Sharp's resampling interpolation blended the two colors ([255,14,14] instead of pure red) - not a logic bug, just sampling too close to the boundary. Use width:200 height:20 (scale=1, pure crop with no resampling) so the kept rows sit solidly inside the white (top) or red (bottom) half.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repro/root-cause PR for cloudflare#14714 on our fork, to verify the resolveFit() fix (commit a06fc78) resolves the failing get-platform-proxy Image bindings snapshot test before proposing it upstream.
Not intended to be merged upstream from here - just running our own CI for verification.