fix(export): apply EXIF orientation in the keepsake too
Round 1 fixed EXIF orientation in the compression worker, which corrected the live app — feed preview and diashow display. The export worker was missed, and it does not reuse those derivatives: it re-decodes the originals itself with `image::open`, which ignores the orientation tag, then re-encodes to JPEG, which drops the tag — so the viewer has no way to recover it. The damage was oddly shaped, which is exactly why it reads as a viewer bug: Gallery.zip originals correct (byte-copied, EXIF intact) Memories viewer grid thumbnails SIDEWAYS (always) Memories viewer full image >5 MB SIDEWAYS (re-encoded at 2000px) Memories viewer full image ≤5 MB correct (streamed byte-for-byte) So in the keepsake people actually keep, every portrait photo in the grid was on its side, and clicking through silently "fixed" small photos but not large ones. Rather than paste the decoder dance a third time, extract `services::imaging:: decode_oriented` and route both workers through it, so there is exactly one way to turn a file on disk into a DynamicImage. It carries a second invariant that had also drifted: `image::open` applies NO decode limits, so the export path was decoding arbitrary user-supplied images unbounded — the decompression-bomb cap existed only in the compression worker. Both now come as a pair, which is the point of having one function. Not done: switching export to consume the existing `display` derivative. It would fix orientation and drop a redundant full-resolution decode per photo, but it would also replace the pristine ≤5 MB originals in the keepsake with 2048px re-encodes — a real quality regression in the one artefact people keep forever. Test uploads the round-1 fixture (40x20 landscape tagged Orientation=6), runs a real export, pulls the thumbnail out of Memories.zip and asserts it came back portrait — with a sanity check that the source really is stored landscape, so the test can't pass against a pipeline that does nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -3,7 +3,6 @@ use std::sync::Arc;
|
|||||||
use std::sync::atomic::{AtomicU64, Ordering};
|
use std::sync::atomic::{AtomicU64, Ordering};
|
||||||
|
|
||||||
use anyhow::{Context, Result};
|
use anyhow::{Context, Result};
|
||||||
use image::ImageDecoder;
|
|
||||||
use sqlx::PgPool;
|
use sqlx::PgPool;
|
||||||
use tokio::sync::{Semaphore, broadcast};
|
use tokio::sync::{Semaphore, broadcast};
|
||||||
use uuid::Uuid;
|
use uuid::Uuid;
|
||||||
@@ -205,38 +204,9 @@ impl CompressionWorker {
|
|||||||
|
|
||||||
// Run blocking image operations in a spawn_blocking task
|
// Run blocking image operations in a spawn_blocking task
|
||||||
tokio::task::spawn_blocking(move || -> Result<()> {
|
tokio::task::spawn_blocking(move || -> Result<()> {
|
||||||
// Reject decompression bombs *before* fully decoding: the upload body
|
// Decompression-bomb limits + EXIF orientation, both in one place — see
|
||||||
// cap bounds the file size on disk, but a small file can still decode to
|
// services::imaging for why neither may be skipped.
|
||||||
// enormous dimensions (e.g. a ~1 MB image expanding to 50k×50k px →
|
let img = crate::services::imaging::decode_oriented(&original)?;
|
||||||
// gigabytes), OOM-ing the box during decode/resize. 12000×12000 covers
|
|
||||||
// any real phone photo; max_alloc hard-caps the decode allocation.
|
|
||||||
let mut reader = image::ImageReader::open(&original)
|
|
||||||
.context("failed to open image")?
|
|
||||||
.with_guessed_format()
|
|
||||||
.context("failed to read image header")?;
|
|
||||||
let mut limits = image::Limits::default();
|
|
||||||
limits.max_image_width = Some(12_000);
|
|
||||||
limits.max_image_height = Some(12_000);
|
|
||||||
limits.max_alloc = Some(256 * 1024 * 1024);
|
|
||||||
reader.limits(limits);
|
|
||||||
|
|
||||||
// Apply the EXIF orientation. Phones do not rotate the sensor data — they record
|
|
||||||
// the physical camera orientation in a tag and store the pixels as shot. `decode()`
|
|
||||||
// hands back those raw pixels, and the JPEG re-encode below writes no EXIF at all,
|
|
||||||
// so skipping this stores EVERY portrait photo sideways in the feed preview, the
|
|
||||||
// 2048px diashow display and the keepsake — while the untouched original still
|
|
||||||
// renders upright, which is why it looks like a viewer bug rather than a pipeline
|
|
||||||
// one. `into_decoder` carries the limits set above through to the decoder, so the
|
|
||||||
// decompression-bomb guard is unaffected.
|
|
||||||
let mut decoder = reader.into_decoder().context("failed to decode image")?;
|
|
||||||
// A missing or malformed tag is not a failure: most images simply have none.
|
|
||||||
let orientation = decoder
|
|
||||||
.orientation()
|
|
||||||
.unwrap_or(image::metadata::Orientation::NoTransforms);
|
|
||||||
let mut img =
|
|
||||||
image::DynamicImage::from_decoder(decoder).context("failed to decode image")?;
|
|
||||||
img.apply_orientation(orientation);
|
|
||||||
let img = img;
|
|
||||||
|
|
||||||
// Preview: max 800px, preserving aspect ratio (data-saver feed).
|
// Preview: max 800px, preserving aspect ratio (data-saver feed).
|
||||||
img.resize(
|
img.resize(
|
||||||
|
|||||||
@@ -791,7 +791,12 @@ async fn run_html_export_inner(
|
|||||||
let thumb_path_clone = thumb_path.clone();
|
let thumb_path_clone = thumb_path.clone();
|
||||||
|
|
||||||
let thumb_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
let thumb_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
||||||
let img = image::open(&src_clone).context("failed to open image for thumbnail")?;
|
// `decode_oriented`, not `image::open`: the latter ignores the EXIF
|
||||||
|
// orientation tag AND applies no decode limits. Using it here is why every
|
||||||
|
// portrait photo came out sideways in the keepsake's HTML viewer grid — the
|
||||||
|
// re-encode below drops the tag, so the viewer cannot recover it.
|
||||||
|
let img = crate::services::imaging::decode_oriented(&src_clone)
|
||||||
|
.context("failed to open image for thumbnail")?;
|
||||||
let resized = img.resize(400, 400, image::imageops::FilterType::Lanczos3);
|
let resized = img.resize(400, 400, image::imageops::FilterType::Lanczos3);
|
||||||
resized
|
resized
|
||||||
.save_with_format(&thumb_path_clone, image::ImageFormat::Jpeg)
|
.save_with_format(&thumb_path_clone, image::ImageFormat::Jpeg)
|
||||||
@@ -812,8 +817,12 @@ async fn run_html_export_inner(
|
|||||||
let full_path_clone = full_path.clone();
|
let full_path_clone = full_path.clone();
|
||||||
|
|
||||||
let compress_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
let compress_result = tokio::task::spawn_blocking(move || -> Result<()> {
|
||||||
let img =
|
// Same reason as the thumbnail above. This branch only runs for originals
|
||||||
image::open(&src_clone).context("failed to open image for compression")?;
|
// over 5 MB, which is why the viewer's full image looked correct for small
|
||||||
|
// photos and sideways for large ones — an inconsistency that reads as a
|
||||||
|
// viewer bug rather than an export one.
|
||||||
|
let img = crate::services::imaging::decode_oriented(&src_clone)
|
||||||
|
.context("failed to open image for compression")?;
|
||||||
let resized = img.resize(2000, 2000, image::imageops::FilterType::Lanczos3);
|
let resized = img.resize(2000, 2000, image::imageops::FilterType::Lanczos3);
|
||||||
resized
|
resized
|
||||||
.save_with_format(&full_path_clone, image::ImageFormat::Jpeg)
|
.save_with_format(&full_path_clone, image::ImageFormat::Jpeg)
|
||||||
|
|||||||
52
backend/src/services/imaging.rs
Normal file
52
backend/src/services/imaging.rs
Normal file
@@ -0,0 +1,52 @@
|
|||||||
|
//! Shared image decoding.
|
||||||
|
//!
|
||||||
|
//! Exists so there is exactly ONE way to turn a file on disk into a `DynamicImage` in this
|
||||||
|
//! codebase. Two properties have to hold everywhere an image is decoded, and both were
|
||||||
|
//! previously re-derived per call site — which is how they drifted apart:
|
||||||
|
//!
|
||||||
|
//! - **EXIF orientation must be applied.** Phones do not rotate sensor data; they record how
|
||||||
|
//! the camera was held in a tag and store the pixels as shot. `image::open` and
|
||||||
|
//! `ImageReader::decode` both hand back the raw pixels and ignore that tag, and re-encoding
|
||||||
|
//! to JPEG writes no EXIF, so the derivative is permanently sideways while the untouched
|
||||||
|
//! original still renders upright. The compression worker was fixed; the export worker was
|
||||||
|
//! not, so every portrait photo came out sideways in the keepsake's HTML viewer.
|
||||||
|
//! - **Decode limits must be set.** The upload body cap bounds the file on disk, but a small
|
||||||
|
//! file can decode to enormous dimensions (a ~1 MB image expanding to 50k×50k px), OOM-ing
|
||||||
|
//! the box. `image::open` applies NO limits at all, so the export path was also decoding
|
||||||
|
//! arbitrary user-supplied images unbounded.
|
||||||
|
|
||||||
|
use anyhow::{Context, Result};
|
||||||
|
use image::{DynamicImage, ImageDecoder};
|
||||||
|
use std::path::Path;
|
||||||
|
|
||||||
|
/// Bounds for any decode of user-supplied image data. 12000×12000 covers any real phone
|
||||||
|
/// photo; `max_alloc` hard-caps the decode allocation.
|
||||||
|
fn decode_limits() -> image::Limits {
|
||||||
|
let mut limits = image::Limits::default();
|
||||||
|
limits.max_image_width = Some(12_000);
|
||||||
|
limits.max_image_height = Some(12_000);
|
||||||
|
limits.max_alloc = Some(256 * 1024 * 1024);
|
||||||
|
limits
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Decode an image from disk with decompression-bomb limits applied and its EXIF
|
||||||
|
/// orientation baked into the pixels.
|
||||||
|
///
|
||||||
|
/// Blocking — call inside `spawn_blocking`.
|
||||||
|
pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
|
||||||
|
let mut reader = image::ImageReader::open(path)
|
||||||
|
.context("failed to open image")?
|
||||||
|
.with_guessed_format()
|
||||||
|
.context("failed to read image header")?;
|
||||||
|
reader.limits(decode_limits());
|
||||||
|
|
||||||
|
// `into_decoder` carries the limits above through, so reading the tag costs nothing in
|
||||||
|
// safety. A missing or malformed tag is not an error — most images simply have none.
|
||||||
|
let mut decoder = reader.into_decoder().context("failed to decode image")?;
|
||||||
|
let orientation = decoder
|
||||||
|
.orientation()
|
||||||
|
.unwrap_or(image::metadata::Orientation::NoTransforms);
|
||||||
|
let mut img = DynamicImage::from_decoder(decoder).context("failed to decode image")?;
|
||||||
|
img.apply_orientation(orientation);
|
||||||
|
Ok(img)
|
||||||
|
}
|
||||||
@@ -2,6 +2,7 @@ pub mod compression;
|
|||||||
pub mod config;
|
pub mod config;
|
||||||
pub mod disk;
|
pub mod disk;
|
||||||
pub mod export;
|
pub mod export;
|
||||||
|
pub mod imaging;
|
||||||
pub mod maintenance;
|
pub mod maintenance;
|
||||||
pub mod rate_limiter;
|
pub mod rate_limiter;
|
||||||
pub mod sse_tickets;
|
pub mod sse_tickets;
|
||||||
|
|||||||
117
e2e/specs/06-export/exif-orientation.spec.ts
Normal file
117
e2e/specs/06-export/exif-orientation.spec.ts
Normal file
@@ -0,0 +1,117 @@
|
|||||||
|
/**
|
||||||
|
* Regression guard — the keepsake must not be sideways.
|
||||||
|
*
|
||||||
|
* Round 1 taught the compression worker to apply EXIF orientation, which fixed the live app
|
||||||
|
* (feed preview + diashow display). The export worker was missed: it does NOT reuse those
|
||||||
|
* derivatives — it re-decodes the originals itself with `image::open`, which ignores the
|
||||||
|
* orientation tag — and then re-encodes to JPEG, which drops the tag, so the viewer has no
|
||||||
|
* way to recover it.
|
||||||
|
*
|
||||||
|
* The resulting damage was oddly shaped, which is what made it read as a viewer bug:
|
||||||
|
* - Gallery.zip originals → correct (byte-copied, EXIF intact)
|
||||||
|
* - Memories viewer grid thumbnails → ALWAYS sideways
|
||||||
|
* - Memories viewer full image >5 MB → sideways (re-encoded at 2000px)
|
||||||
|
* - Memories viewer full image ≤5 MB → correct (streamed byte-for-byte)
|
||||||
|
*
|
||||||
|
* So clicking a small photo silently "fixed" it and a large one didn't. This pins the
|
||||||
|
* thumbnail, which is the path every photo takes.
|
||||||
|
*/
|
||||||
|
import { test, expect } from '../../fixtures/test';
|
||||||
|
import { uploadRaw } from '../../helpers/upload-client';
|
||||||
|
import { BASE } from '../../helpers/env';
|
||||||
|
import { execFileSync } from 'node:child_process';
|
||||||
|
import { mkdtempSync, readFileSync, writeFileSync, rmSync } from 'node:fs';
|
||||||
|
import { tmpdir } from 'node:os';
|
||||||
|
import { join } from 'node:path';
|
||||||
|
|
||||||
|
// 40x20 landscape pixels tagged Orientation=6 ("rotate 90° CW to display"), so anything
|
||||||
|
// that honours the tag emits a PORTRAIT derivative.
|
||||||
|
const EXIF_FIXTURE = join(process.cwd(), 'fixtures', 'media', 'portrait-exif6.jpg');
|
||||||
|
|
||||||
|
/** Pixel dimensions from a JPEG's SOF marker — avoids an image dep for one assertion. */
|
||||||
|
function jpegSize(buf: Buffer): { width: number; height: number } {
|
||||||
|
let i = 2;
|
||||||
|
while (i < buf.length) {
|
||||||
|
if (buf[i] !== 0xff) {
|
||||||
|
i++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
const marker = buf[i + 1];
|
||||||
|
if (marker >= 0xc0 && marker <= 0xcf && marker !== 0xc4 && marker !== 0xc8 && marker !== 0xcc) {
|
||||||
|
return { height: buf.readUInt16BE(i + 5), width: buf.readUInt16BE(i + 7) };
|
||||||
|
}
|
||||||
|
i += 2 + buf.readUInt16BE(i + 2);
|
||||||
|
}
|
||||||
|
throw new Error('no SOF marker found — not a JPEG?');
|
||||||
|
}
|
||||||
|
|
||||||
|
test.describe('Export — EXIF orientation in the keepsake', () => {
|
||||||
|
test('the Memories viewer thumbnail of a rotated photo is upright', async ({ host, db }) => {
|
||||||
|
test.setTimeout(90_000);
|
||||||
|
const bearer = { Authorization: `Bearer ${host.jwt}` };
|
||||||
|
|
||||||
|
const src = readFileSync(EXIF_FIXTURE);
|
||||||
|
// Sanity: the SOURCE really is stored landscape, or this test proves nothing.
|
||||||
|
const srcSize = jpegSize(src);
|
||||||
|
expect(srcSize.width).toBeGreaterThan(srcSize.height);
|
||||||
|
|
||||||
|
const up = await uploadRaw(host.jwt, src, {
|
||||||
|
filename: 'hochkant.jpg',
|
||||||
|
contentType: 'image/jpeg',
|
||||||
|
caption: 'hochkant',
|
||||||
|
});
|
||||||
|
expect(up.status).toBe(201);
|
||||||
|
const { id } = (await up.json()) as { id: string };
|
||||||
|
await expect.poll(() => db.compressionStatus(id), { timeout: 30_000 }).toBe('done');
|
||||||
|
|
||||||
|
const rel = await fetch(`${BASE}/api/v1/host/gallery/release`, {
|
||||||
|
method: 'POST',
|
||||||
|
headers: bearer,
|
||||||
|
});
|
||||||
|
expect(rel.status).toBe(204);
|
||||||
|
|
||||||
|
await expect
|
||||||
|
.poll(
|
||||||
|
async () => {
|
||||||
|
const res = await fetch(`${BASE}/api/v1/export/status`, { headers: bearer });
|
||||||
|
return (await res.json()).html?.status;
|
||||||
|
},
|
||||||
|
{ timeout: 60_000, intervals: [500] }
|
||||||
|
)
|
||||||
|
.toBe('done');
|
||||||
|
|
||||||
|
const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, {
|
||||||
|
method: 'POST',
|
||||||
|
headers: bearer,
|
||||||
|
});
|
||||||
|
const { ticket } = (await ticketRes.json()) as { ticket: string };
|
||||||
|
const dl = await fetch(`${BASE}/api/v1/export/html?ticket=${encodeURIComponent(ticket)}`);
|
||||||
|
expect(dl.status).toBe(200);
|
||||||
|
|
||||||
|
const dir = mkdtempSync(join(tmpdir(), 'eventsnap-exif-'));
|
||||||
|
try {
|
||||||
|
const zipPath = join(dir, 'Memories.zip');
|
||||||
|
writeFileSync(zipPath, Buffer.from(await dl.arrayBuffer()));
|
||||||
|
|
||||||
|
const entries = execFileSync('unzip', ['-Z1', zipPath], { encoding: 'utf8' })
|
||||||
|
.split('\n')
|
||||||
|
.filter(Boolean);
|
||||||
|
const thumbEntry = entries.find((e) => e.includes(`${id}_thumb`));
|
||||||
|
expect(thumbEntry, `no thumbnail for ${id} in Memories.zip`).toBeTruthy();
|
||||||
|
|
||||||
|
// `-p` streams the entry to stdout. Extracting to disk instead fails with EACCES:
|
||||||
|
// the archive preserves the container's file mode, which the test user can't read.
|
||||||
|
const thumb = execFileSync('unzip', ['-p', zipPath, thumbEntry!], {
|
||||||
|
maxBuffer: 64 * 1024 * 1024,
|
||||||
|
});
|
||||||
|
const { width, height } = jpegSize(thumb);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
height,
|
||||||
|
`the keepsake grid thumbnail is ${width}x${height} — EXIF orientation was not applied`
|
||||||
|
).toBeGreaterThan(width);
|
||||||
|
} finally {
|
||||||
|
rmSync(dir, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user