fix(uploads): apply the EXIF orientation before discarding it (#300)
Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them. A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not. The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis. Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail. The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time. This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301. Closes #300 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -89,7 +89,27 @@ export async function reencodeInPlace(filePath: string, mimetype: string): Promi
|
||||
// uploader's image while reporting success. It is not set unconditionally
|
||||
// because it changes how `resize` interprets height (the full frame
|
||||
// strip, not one frame), which would be wrong for the other two.
|
||||
sharp(filePath, mimetype === 'image/webp' ? { animated: true } : {}).resize({
|
||||
sharp(filePath, mimetype === 'image/webp' ? { animated: true } : {})
|
||||
// Applies the EXIF orientation to the pixels, and must come before
|
||||
// resize (#300).
|
||||
//
|
||||
// A camera does not turn its sensor data round. It writes the pixels as
|
||||
// the sensor read them and sets an Orientation tag saying which way up
|
||||
// they go, and every viewer honours that — which is why a portrait
|
||||
// photograph looks upright to the person who took it and to the person
|
||||
// who attached it. Stripping the tag without applying it does not leave
|
||||
// the photo alone: it leaves the pixels sideways with nothing left to
|
||||
// explain them, and the sender's upright photo arrives on its side.
|
||||
//
|
||||
// Before resize because the resize bounds are width and height, and for
|
||||
// a portrait photo those are the wrong way round until the rotation has
|
||||
// happened. A 3000x4000 photograph stored as 4000x3000 would otherwise
|
||||
// be bounded on the wrong axis.
|
||||
//
|
||||
// No argument: that is what makes it read the tag rather than turn the
|
||||
// image by a fixed amount.
|
||||
.rotate()
|
||||
.resize({
|
||||
width: MAX_DIMENSION,
|
||||
height: MAX_DIMENSION,
|
||||
fit: 'inside',
|
||||
|
||||
@@ -42,6 +42,30 @@ async function photoWithLocation(): Promise<Buffer> {
|
||||
.toBuffer();
|
||||
}
|
||||
|
||||
/**
|
||||
* A photo the way a phone actually writes one: pixels in the sensor's own
|
||||
* orientation, and an EXIF tag saying which way up to display them.
|
||||
*
|
||||
* Orientation 6 means "rotate 90° clockwise to show this". So these 400x200
|
||||
* landscape pixels are what a portrait photograph looks like on disk, and every
|
||||
* viewer that honours the tag — the camera roll, the mail client, the browser
|
||||
* the sender attached it from — shows it as 200x400. That is why the sender
|
||||
* sees an upright photo and has no idea the file is sideways.
|
||||
*/
|
||||
async function portraitPhoto(): Promise<Buffer> {
|
||||
return sharp({
|
||||
create: { width: 400, height: 200, channels: 3, background: { r: 20, g: 60, b: 120 } }
|
||||
})
|
||||
// withMetadata rather than withExif, and the difference is not cosmetic:
|
||||
// withExif({ IFD0: { Orientation: '6' } }) writes a tag that sharp reads
|
||||
// back as orientation 1, so a fixture built that way carries no orientation
|
||||
// at all and would let this test pass against the unfixed code. Checked
|
||||
// rather than assumed — the first version of this test did exactly that.
|
||||
.withMetadata({ orientation: 6 })
|
||||
.jpeg()
|
||||
.toBuffer();
|
||||
}
|
||||
|
||||
async function storedPathFor(itemId: number): Promise<string> {
|
||||
const { rows } = await pool.query<{ image_path: string }>(
|
||||
`SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`,
|
||||
@@ -108,6 +132,37 @@ describe('an uploaded photo does not keep where it was taken', () => {
|
||||
expect(stored.height).toBe(200);
|
||||
});
|
||||
|
||||
/**
|
||||
* The other half of stripping metadata, and the half #226 missed (#300).
|
||||
*
|
||||
* Dropping EXIF is right — a product photo should not publish where it was
|
||||
* taken. But orientation lives in EXIF too, and deleting it without first
|
||||
* applying it to the pixels does not leave the photo alone: it leaves the
|
||||
* pixels sideways with nothing left to explain them. The sender sees an
|
||||
* upright photo, uploads it, and finds it rotated.
|
||||
*/
|
||||
it('applies the orientation before throwing the tag away', async () => {
|
||||
const itemId = await createItemWith(await portraitPhoto(), 'portrait.jpg');
|
||||
|
||||
const stored = await sharp(await storedPathFor(itemId)).metadata();
|
||||
|
||||
// 400x200 on disk with Orientation 6 is a 200x400 photograph. Stored
|
||||
// upright, the dimensions swap.
|
||||
expect(stored.width).toBe(200);
|
||||
expect(stored.height).toBe(400);
|
||||
});
|
||||
|
||||
// And the tag itself still goes, so nothing rotates it a second time.
|
||||
it('does not leave the orientation tag behind after applying it', async () => {
|
||||
const itemId = await createItemWith(await portraitPhoto(), 'portrait.jpg');
|
||||
|
||||
const stored = await sharp(await storedPathFor(itemId)).metadata();
|
||||
|
||||
expect(stored.exif).toBeUndefined();
|
||||
// sharp reports 1 — the identity — for a file with nothing to say about it.
|
||||
expect(stored.orientation ?? 1).toBe(1);
|
||||
});
|
||||
|
||||
it('leaves no temporary re-encoding files on the volume', async () => {
|
||||
await createItemWith(await photoWithLocation(), 'vase.jpg');
|
||||
|
||||
|
||||
Reference in New Issue
Block a user