-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(miniflare): apply EXIF orientation in local Images binding and cf.image transforms #15031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "miniflare": patch | ||
| --- | ||
|
|
||
| Fix the local Images binding and `cf.image` transforms ignoring EXIF orientation. Previously, photos stored with an EXIF orientation flag (e.g. phone portrait photos, stored as landscape pixels plus a rotation flag) came back sideways from local transforms, while the production Images binding auto-orients them. Local dev now bakes the EXIF rotation into the pixels before applying transforms, matching production behavior. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,12 +61,12 @@ export async function imagesLocalFetcher(request: Request): Promise<Response> { | |
| ); | ||
| } | ||
|
|
||
| const transformer = sharp(await body.arrayBuffer(), {}); | ||
| const source = await body.arrayBuffer(); | ||
|
|
||
| const url = new URL(request.url); | ||
|
|
||
| if (url.pathname == "/info") { | ||
| return runInfo(transformer); | ||
| return runInfo(sharp(source, {})); | ||
| } else { | ||
| const badTransformsResponse = errorResponse( | ||
| 400, | ||
|
|
@@ -96,7 +96,14 @@ export async function imagesLocalFetcher(request: Request): Promise<Response> { | |
| ); | ||
| } | ||
|
|
||
| return runTransform(transformer, transforms, outputFormat); | ||
| // Production applies EXIF orientation before any transforms; | ||
| // autoOrient bakes the rotation into the pixels at decode so | ||
| // EXIF-rotated photos (e.g. phone portraits) come out upright. | ||
| return runTransform( | ||
| sharp(source, { autoOrient: true }), | ||
| transforms, | ||
| outputFormat | ||
| ); | ||
| } catch { | ||
| return badTransformsResponse; | ||
| } | ||
|
|
@@ -401,7 +408,7 @@ export async function cfImageLocalFetcher(request: Request): Promise<Response> { | |
| } | ||
|
|
||
| if (options.format === "json") { | ||
| const jsonTransformer = sharp(source); | ||
| const jsonTransformer = sharp(source, { autoOrient: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 cf.image format=json reports stored (non-oriented) original dimensions alongside oriented output dimensions The Was this helpful? React with 👍 or 👎 to provide feedback.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vdanielb could you update this too? 😄 |
||
| applyCfImageTransforms(jsonTransformer, options); | ||
| const { info } = await jsonTransformer.toBuffer({ | ||
| resolveWithObject: true, | ||
|
|
@@ -418,7 +425,9 @@ export async function cfImageLocalFetcher(request: Request): Promise<Response> { | |
| }); | ||
| } | ||
|
|
||
| const transformer = sharp(source); | ||
| // Production applies EXIF orientation before any transforms; | ||
| // autoOrient bakes the rotation into the pixels at decode. | ||
| const transformer = sharp(source, { autoOrient: true }); | ||
| applyCfImageTransforms(transformer, options); | ||
|
|
||
| const quality = resolveQuality(options.quality); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,20 +70,33 @@ describe("Images binding local transforms", () => { | |
|
|
||
| async function transform( | ||
| transformOpts: Record<string, unknown>, | ||
| format = "image/png" | ||
| format = "image/png", | ||
| source: Buffer = sourcePng | ||
| ) { | ||
| const params = new URLSearchParams({ | ||
| transform: JSON.stringify(transformOpts), | ||
| format, | ||
| }); | ||
| const res = await mf.dispatchFetch(`http://localhost/?${params}`, { | ||
| method: "POST", | ||
| body: sourcePng, | ||
| body: source, | ||
| }); | ||
| const body = Buffer.from(await res.arrayBuffer()); | ||
| return { res, body }; | ||
| } | ||
|
|
||
| // The 200x100 white-top/red-bottom source as a JPEG tagged with EXIF | ||
| // orientation 6 ("rotate 90° CW to display") - the layout phone cameras | ||
| // use for portrait photos. A production-matching transform bakes that | ||
| // rotation in, producing an upright 100x200 image with red on the left | ||
| // (the source's bottom half) and white on the right. | ||
| async function exifRotatedJpeg() { | ||
| return sharp(sourcePng) | ||
| .jpeg({ quality: 95 }) | ||
| .withMetadata({ orientation: 6 }) | ||
| .toBuffer(); | ||
| } | ||
|
Comment on lines
+93
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: Test fixture relies on withMetadata's orientation option
Was this helpful? React with 👍 or 👎 to provide feedback.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vdanielb could you add this assert? 🙏 |
||
|
|
||
| async function pixelAt(body: Buffer, x: number, y: number) { | ||
| const { data, info } = await sharp(body) | ||
| .raw() | ||
|
|
@@ -196,4 +209,40 @@ describe("Images binding local transforms", () => { | |
| const { r, g, b } = await pixelAt(body, 0, 0); | ||
| expect([r, g, b]).toEqual([255, 255, 255]); | ||
| }); | ||
|
|
||
| test("EXIF orientation is applied before transforms (matches production)", async ({ | ||
| expect, | ||
| }) => { | ||
| // Production auto-orients per EXIF before transforming; without it the | ||
| // 200x100 source would pass through sideways as 200x100. | ||
| const { body } = await transform({}, "image/png", await exifRotatedJpeg()); | ||
| const meta = await sharp(body).metadata(); | ||
| expect(meta.width).toBe(100); | ||
| expect(meta.height).toBe(200); | ||
|
|
||
| // After the 90° CW rotation the source's red bottom half lands on the | ||
| // left and the white top half on the right. JPEG encoding is lossy, so | ||
| // sample deep inside each half and allow small artifacts. | ||
| const left = await pixelAt(body, 25, 100); | ||
| expect(left.r).toBeGreaterThan(240); | ||
| expect(left.g).toBeLessThan(15); | ||
| expect(left.b).toBeLessThan(15); | ||
| const right = await pixelAt(body, 75, 100); | ||
| expect(right.r).toBeGreaterThan(240); | ||
| expect(right.g).toBeGreaterThan(240); | ||
| expect(right.b).toBeGreaterThan(240); | ||
| }); | ||
|
Comment on lines
+213
to
+234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: No regression test added for the cf.image path The PR changes both Was this helpful? React with 👍 or 👎 to provide feedback.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vdanielb could you add tests for |
||
|
|
||
| test("EXIF orientation composes with resize", async ({ expect }) => { | ||
| // width applies to the upright (100x200) image, not the stored | ||
| // sideways (200x100) pixels. | ||
| const { body } = await transform( | ||
| { width: 50 }, | ||
| "image/png", | ||
| await exifRotatedJpeg() | ||
| ); | ||
| const meta = await sharp(body).metadata(); | ||
| expect(meta.width).toBe(50); | ||
| expect(meta.height).toBe(100); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Transform output dimensions now disagree with info() for EXIF-rotated images
/infointentionally keeps the non-auto-orientedsharp(source, {})(packages/miniflare/src/plugins/images/fetcher.ts:69) while the transform path now decodes withautoOrient: true. For a JPEG with EXIF orientation 6,env.IMAGES.info()will report 200x100 while.transform({}).output(...)now yields 100x200. Worth confirming against production: if the productioninfoendpoint reports the display (EXIF-applied) dimensions, local dev will now be inconsistent with production in the opposite direction from the bug this PR fixes. No test covers/infowith an EXIF-tagged image.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've manually tested this and I can confirm that in production
env.IMAGES.info()differ between production and the local simulationin production the original dimensions are shown while locally those after the EXIF
@vdanielb could you address fix and also add test to cover
/info? 🙏