From 04e0e8fdf4f8957cea8afd9ead2fa5bc351fe077 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 13 Aug 2026 11:34:23 -0500 Subject: [PATCH] Measure and thumbnail an image from a single decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ingest asked for metadata() then writeThumbnail(), which decoded the file twice. That pairing was free under sharp, whose .metadata() only parses the header, but every decode here is a full one — ~1s for a 12MP photo — so the naive translation doubled the most expensive thing on the ingest path. image-ops.measureAndThumbnail() returns both from one decode. Full ingest of a 12MP photo: 2 decodes/~1.9s -> 1150ms, still with zero event-loop stalls. The subtlety is the failure contract. In the two-call version width and height were assigned BEFORE the thumbnail was attempted, so a failed thumbnail still left usable dimensions on the row — the player needs them to letterbox. Merging naively would have turned any thumbnail failure into total metadata loss. So a WRITE failure is reported ({thumbnailWritten:false, thumbnailError}) with the dimensions intact, and the caller sets thumbnail_path only when the write succeeded, keeping the phantom-path discipline. A DECODE failure still throws — there is nothing to report about an unreadable image. backfill-rotation-dims.js deliberately keeps the separate calls: it probes every image row but regenerates a thumbnail only for the few whose dimensions changed, so pairing them there would decode files it has no reason to thumbnail. Tests count decodes rather than timing them — an exact property, and a wall-clock comparison would be flaky under load. The count filters for reads of the file under test: Node's ESM loader also goes through fs.promises.readFile, so a raw call count picks up jimp's and the WASM codecs' lazy loading and reads 30 instead of 1. 1649/1649 pass. Ingest re-verified across all 7 formats with sharp moved aside. --- server/lib/content-ingest.js | 21 ++++++++++------ server/lib/image-ops-core.js | 45 ++++++++++++++++++++++++++++------ server/lib/image-ops-worker.js | 1 + server/lib/image-ops.js | 11 ++++++++- server/test/image-ops.test.js | 44 ++++++++++++++++++++++++++++++++- 5 files changed, 105 insertions(+), 17 deletions(-) diff --git a/server/lib/content-ingest.js b/server/lib/content-ingest.js index 36c7b2b..291f24e 100644 --- a/server/lib/content-ingest.js +++ b/server/lib/content-ingest.js @@ -46,18 +46,23 @@ async function deriveMediaMetadata(sourcePath, filepath, mime) { thumbnailPath = filepath; } else if (mime.startsWith('image/')) { const imageOps = require('./image-ops'); - const metadata = await imageOps.metadata(sourcePath); + const thumbName = `thumb_${filepath}`; + // Measure and thumbnail from ONE decode. Asking separately costs two, and a decode is the + // single most expensive thing on this path (~1s for a 12MP photo — unlike sharp, whose + // .metadata() only read the header). #170: rotation is implicit, the decoder auto-orients, + // so the recorded dimensions and the thumbnail agree without an explicit rotate. + const metadata = await imageOps.measureAndThumbnail( + sourcePath, path.join(config.contentDir, thumbName), config.thumbnailWidth, 70); // #170: honor EXIF orientation so a portrait photo isn't stored as landscape. The decoder // applies it and reports orientation 1, so this is a no-op pass-through today — kept so the // rule lives in one place regardless of which decoder is underneath. ({ width, height } = imageDisplayDims(metadata)); - // Assign thumbnailPath only AFTER the write succeeds: a decode failure used to - // return the already-assigned name for a file that was never written, storing a - // phantom thumbnail_path that the UI then requests forever as a broken image. - const thumbName = `thumb_${filepath}`; - // #170: rotation is implicit — the decode already auto-oriented, so the thumbnail matches. - await imageOps.writeThumbnail(sourcePath, path.join(config.contentDir, thumbName), config.thumbnailWidth, 70); - thumbnailPath = thumbName; + // Assign thumbnailPath only if the write actually succeeded: naming it unconditionally used + // to store a phantom thumbnail_path for a file that was never created, which the UI then + // requests forever as a broken image. The dimensions above survive that failure on purpose — + // they are independently useful, and losing them would letterbox the asset wrongly. + if (metadata.thumbnailWritten) thumbnailPath = thumbName; + else console.warn(`Thumbnail write failed for ${filepath}: ${metadata.thumbnailError}`); } else if (mime.startsWith('video/')) { try { // execFile, NOT execFileSync. These two spawns each carry a 15s timeout, and run diff --git a/server/lib/image-ops-core.js b/server/lib/image-ops-core.js index 6489c97..ad5372a 100644 --- a/server/lib/image-ops-core.js +++ b/server/lib/image-ops-core.js @@ -97,15 +97,46 @@ async function metadata(src) { } /* - * Write a JPEG thumbnail `width` px wide, aspect preserved — sharp's - * .rotate().resize(width).jpeg({quality}).toFile(). Rotation is implicit in the decode. - * Never upscales: sharp's resize() would enlarge a small source, but a thumbnail bigger than its - * original is pure waste, and the callers only ever shrink. + * Resize-and-encode an ALREADY DECODED image. Never upscales: sharp's resize() would enlarge a + * small source, but a thumbnail bigger than its original is pure waste and callers only shrink. + * Mutates img, so measure before calling. */ -async function writeThumbnail(src, destPath, width, quality = 70) { - const img = await readImage(src); +async function encodeThumbnail(img, destPath, width, quality) { if (img.bitmap.width > width) img.resize({ w: width }); await fs.promises.writeFile(destPath, await img.getBuffer('image/jpeg', { quality })); } -module.exports = { metadata, writeThumbnail, readImage }; +/* + * Write a JPEG thumbnail `width` px wide, aspect preserved — sharp's + * .rotate().resize(width).jpeg({quality}).toFile(). Rotation is implicit in the decode. + */ +async function writeThumbnail(src, destPath, width, quality = 70) { + await encodeThumbnail(await readImage(src), destPath, width, quality); +} + +/* + * Measure AND thumbnail from a SINGLE decode — what ingest actually wants. + * + * Calling metadata() then writeThumbnail() decodes the file twice. That was free under sharp, + * whose .metadata() only parses the header, but here every decode is the full ~1s of a 12MP + * photo, so the naive pairing doubled the most expensive thing the ingest path does. + * + * A thumbnail failure must NOT discard the dimensions: they are independently useful (the player + * needs them to letterbox correctly) and that is how the two-call version behaved, since width and + * height were already assigned before the thumbnail was written. So the write is reported, not + * thrown — and the caller assigns a thumbnail_path only when thumbnailWritten is true, keeping the + * phantom-path discipline that stops the UI requesting a file that was never created. + * A DECODE failure still throws: there is nothing to report about an unreadable image. + */ +async function measureAndThumbnail(src, destPath, width, quality = 70) { + const img = await readImage(src); + const measured = { width: img.bitmap.width, height: img.bitmap.height, orientation: 1 }; + try { + await encodeThumbnail(img, destPath, width, quality); + return { ...measured, thumbnailWritten: true, thumbnailError: null }; + } catch (err) { + return { ...measured, thumbnailWritten: false, thumbnailError: err && err.message ? err.message : String(err) }; + } +} + +module.exports = { metadata, writeThumbnail, measureAndThumbnail, readImage }; diff --git a/server/lib/image-ops-worker.js b/server/lib/image-ops-worker.js index 4964f24..576611d 100644 --- a/server/lib/image-ops-worker.js +++ b/server/lib/image-ops-worker.js @@ -16,6 +16,7 @@ const core = require('./image-ops-core'); const OPS = { metadata: (job) => core.metadata(job.src), writeThumbnail: (job) => core.writeThumbnail(job.src, job.dest, job.width, job.quality), + measureAndThumbnail: (job) => core.measureAndThumbnail(job.src, job.dest, job.width, job.quality), }; parentPort.on('message', async (job) => { diff --git a/server/lib/image-ops.js b/server/lib/image-ops.js index 13f65a3..401cbe2 100644 --- a/server/lib/image-ops.js +++ b/server/lib/image-ops.js @@ -127,6 +127,15 @@ function writeThumbnail(src, dest, width, quality = 70) { return submit({ op: 'writeThumbnail', src, dest, width, quality }); } +/* + * Both of the above from ONE decode -> { width, height, orientation, thumbnailWritten, + * thumbnailError }. Prefer this wherever both are wanted: a decode here is the full ~1s of a 12MP + * photo, not sharp's cheap header parse, so the pair costs double. See image-ops-core. + */ +function measureAndThumbnail(src, dest, width, quality = 70) { + return submit({ op: 'measureAndThumbnail', src, dest, width, quality }); +} + /* Drop the worker now rather than waiting out the idle timer. For shutdown paths and tests. */ async function shutdown() { clearIdleTimer(); @@ -135,4 +144,4 @@ async function shutdown() { if (w) await w.terminate(); } -module.exports = { metadata, writeThumbnail, shutdown }; +module.exports = { metadata, writeThumbnail, measureAndThumbnail, shutdown }; diff --git a/server/test/image-ops.test.js b/server/test/image-ops.test.js index 784100b..c7205a5 100644 --- a/server/test/image-ops.test.js +++ b/server/test/image-ops.test.js @@ -6,7 +6,7 @@ // our own thumbnail backfill. lib/image-ops therefore hosts the work on a worker thread, and // these bites pin the properties that makes it safe, none of which a functional test would catch. -const { test, after } = require('node:test'); +const { test, after, mock } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); @@ -76,6 +76,48 @@ test('concurrent callers are serialized, and each still gets its own answer', as assert.deepEqual(got.map(m => [m.width, m.height]), sizes, 'replies must not be crossed between queued jobs'); }); +test('measureAndThumbnail decodes the file exactly once', async () => { + // Counted, not timed: a wall-clock comparison against metadata()+writeThumbnail() would be + // flaky under load, and this is an exact property. Asserted against image-ops-core directly + // because the decode happens on the worker thread, out of reach of a spy set up here. + // readImage() is the only reader in core, so readFile calls == decodes. + const core = require('../lib/image-ops-core'); + const fsp = require('node:fs/promises'); + const src = path.join(tmp, 'once.png'); + fs.writeFileSync(src, await sharp({ create: { width: 200, height: 80, channels: 3, background: '#246' } }).png().toBuffer()); + + const spy = mock.method(fsp, 'readFile'); + // Count reads OF THIS FILE only. Node's ESM loader also reads through fs.promises.readFile, so + // a raw call count picks up jimp's and the WASM codecs' lazy module loading on first use. + const decodes = () => spy.mock.calls.filter(c => String(c.arguments[0]) === src).length; + try { + const r = await core.measureAndThumbnail(src, path.join(tmp, 'once-thumb.jpg'), 100, 70); + assert.equal(decodes(), 1, 'combined op must decode once, not once per answer'); + assert.deepEqual([r.width, r.height], [200, 80]); + assert.equal(r.thumbnailWritten, true); + + // The pairing it replaces, for contrast — this is the cost being removed. + spy.mock.resetCalls(); + await core.metadata(src); + await core.writeThumbnail(src, path.join(tmp, 'twice-thumb.jpg'), 100, 70); + assert.equal(decodes(), 2, 'the separate calls are what cost two decodes'); + } finally { spy.mock.restore(); } +}); + +test('a thumbnail that cannot be written still yields dimensions', async () => { + // Dimensions are independently useful — the player needs them to letterbox — and the two-call + // version kept them, because width/height were assigned before the thumbnail was attempted. + // Merging the calls must not quietly turn a thumbnail failure into a total metadata failure. + const src = path.join(tmp, 'ok.png'); + fs.writeFileSync(src, await sharp({ create: { width: 150, height: 60, channels: 3, background: '#654' } }).png().toBuffer()); + + const undirectable = path.join(tmp, 'no-such-dir', 'thumb.jpg'); // parent does not exist + const r = await imageOps.measureAndThumbnail(src, undirectable, 100, 70); + assert.deepEqual([r.width, r.height], [150, 60], 'dimensions survive a thumbnail write failure'); + assert.equal(r.thumbnailWritten, false); + assert.match(r.thumbnailError || '', /ENOENT|no such file/i); +}); + test('#170 EXIF orientation is applied by the decoder, so dimensions are as DISPLAYED', async () => { // orientation 6 = "rotate 90° CW to display": a 30x100 stored buffer DISPLAYS as 100x30. const p = path.join(tmp, 'rot6.jpg');