diff --git a/backend/__tests__/services/imageProcessorOrientation.test.js b/backend/__tests__/services/imageProcessorOrientation.test.js new file mode 100644 index 00000000..30d0ef5a --- /dev/null +++ b/backend/__tests__/services/imageProcessorOrientation.test.js @@ -0,0 +1,159 @@ +/** + * EXIF orientation in the gallery-facing generators (#1185). + * + * sharp decodes the pixels as stored, not as displayed. A photo whose + * Orientation tag is not 1 — routine for portrait shots on bodies that tag + * rather than rotate the sensor data — therefore resizes from the raw frame + * and comes out sideways. The generators then call `.withMetadata(false)`, + * which strips the tag from the output, so the browser has no hint left to + * correct it either: nothing downstream can recover it. + * + * The download path (`resizeToBox`) always got this right. These three did + * not, which is why the same photo looked correct on download and rotated in + * the gallery. + * + * Every assertion here fails on the unfixed generators. + */ + +const fs = require('fs').promises; +const path = require('path'); +const os = require('os'); +const sharp = require('sharp'); + +// imageProcessor reads its thumbnail settings from app_settings, so without a +// db the require() alone hangs the run. Same shape the other generator tests +// use (ensureHeroImage.external.test.js). +jest.mock('../../src/database/db', () => { + const api = (table) => { + if (table === 'app_settings') { + return { where: () => ({ whereIn: () => [], first: async () => null }), whereIn: async () => [] }; + } + if (table === 'events') return { where: () => ({ first: async () => null }) }; + if (table === 'photos') return { where: () => ({ update: async () => 1, first: async () => null }) }; + return { where: () => ({ first: async () => null }) }; + }; + return { db: api }; +}); + +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); +const storageModule = require('../../src/services/storage'); + +describe('EXIF orientation in thumbnails, heroes and previews (#1185)', () => { + let tmpDir; + let imageProcessor; + let landscapeTaggedPortrait; + + // 400x200 as stored, Orientation 6 (90° CW) — so it DISPLAYS as 200x400. + // This is exactly the shape the reporter's Sony bodies produce: the sensor + // data is landscape and the tag carries the rotation. + const W = 400; + const H = 200; + + let storageRoot; + + beforeAll(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-orient-')); + storageRoot = path.join(tmpDir, 'storage'); + await fs.mkdir(storageRoot, { recursive: true }); + process.env.STORAGE_PATH = storageRoot; + + const storage = new LocalFsStorage({ root: storageRoot }); + await storage.init(); + storageModule.setStorageForTesting(storage); + + landscapeTaggedPortrait = path.join(tmpDir, 'portrait-tagged.jpg'); + await sharp({ + create: { width: W, height: H, channels: 3, background: { r: 120, g: 80, b: 40 } }, + }) + .withMetadata({ orientation: 6 }) + .jpeg() + .toFile(landscapeTaggedPortrait); + + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + }, 60000); + + afterAll(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + test('the fixture really is stored landscape with a rotation tag', async () => { + const m = await sharp(landscapeTaggedPortrait).metadata(); + expect(m.width).toBe(W); + expect(m.height).toBe(H); + expect(m.orientation).toBe(6); + }); + + test('orientedDimensions reports what a viewer sees, not what is stored', async () => { + const m = await sharp(landscapeTaggedPortrait).metadata(); + // Swapped: this is what the grid needs to size a tile, and what the + // database should hold. + expect(imageProcessor.orientedDimensions(m)).toEqual({ width: H, height: W }); + }); + + test('orientedDimensions leaves an untagged image alone', async () => { + const plain = path.join(tmpDir, 'plain.jpg'); + await sharp({ create: { width: W, height: H, channels: 3, background: { r: 1, g: 2, b: 3 } } }) + .jpeg().toFile(plain); + const m = await sharp(plain).metadata(); + expect(imageProcessor.orientedDimensions(m)).toEqual({ width: W, height: H }); + }); + + test('orientedDimensions survives metadata it cannot use', () => { + expect(imageProcessor.orientedDimensions(null)).toEqual({ width: null, height: null }); + expect(imageProcessor.orientedDimensions({})).toEqual({ width: null, height: null }); + }); + + test('the thumbnail comes out portrait, not sideways', async () => { + const rel = await imageProcessor.generateThumbnail(landscapeTaggedPortrait, { + outputBasename: 'orient-thumb.jpg', + regenerate: true, + }); + expect(rel).toBeTruthy(); + + const out = await sharp(path.join(storageRoot, rel)).metadata(); + // Unfixed, this came back wider than tall — the raw frame, resized. + expect(out.height).toBeGreaterThan(out.width); + }); + + test('the preview tier comes out portrait too', async () => { + const rel = await imageProcessor.generatePreviewImage(landscapeTaggedPortrait, { + outputBasename: 'orient-preview.jpg', + }); + expect(rel).toBeTruthy(); + + const out = await sharp(path.join(storageRoot, rel)).metadata(); + expect(out.height).toBeGreaterThan(out.width); + }); + + test('the watermarked rendition is oriented too', async () => { + // gallery.js serves photos.watermark_path ahead of the original when + // branding watermarking is on, so this is the rendition a guest actually + // sees — and it went through its own sharp pipeline that nobody had + // rotated. + const watermarkService = require('../../src/services/watermarkService'); + const buf = await watermarkService.applyWatermark(landscapeTaggedPortrait, { + enabled: true, position: 'bottom-right', opacity: 50, size: 15, + // companyName, not text — the SVG branch reads this one, and without it + // the service falls through without compositing anything. + companyName: 'PicPeak', + }); + expect(Buffer.isBuffer(buf)).toBe(true); + + const out = await sharp(buf).metadata(); + // 400x200 stored, tagged 6 — so the watermarked output must be portrait. + expect(out.height).toBeGreaterThan(out.width); + }); + + test('the orientation tag is gone from the output, so nothing double-rotates', async () => { + // The pixels are corrected now, so a surviving tag would make a viewer + // rotate an already-rotated image. withMetadata(false) strips it; this + // pins that the two changes agree. + const rel = await imageProcessor.generateThumbnail(landscapeTaggedPortrait, { + outputBasename: 'orient-thumb-tag.jpg', + regenerate: true, + }); + const out = await sharp(path.join(storageRoot, rel)).metadata(); + expect(out.orientation === undefined || out.orientation === 1).toBe(true); + }); +}); diff --git a/backend/__tests__/services/photoProcessor.processPhoto.test.js b/backend/__tests__/services/photoProcessor.processPhoto.test.js index 763b213e..a68ce6bf 100644 --- a/backend/__tests__/services/photoProcessor.processPhoto.test.js +++ b/backend/__tests__/services/photoProcessor.processPhoto.test.js @@ -72,6 +72,15 @@ jest.mock('../../src/services/imageProcessor', () => { return { generateThumbnail: mockGenerateThumbnail, extractCaptureDate: mockExtractCaptureDate, + // processPhoto routes stored dimensions through this to get the displayed + // ones (#1185). Mirrored rather than requireActual'd, because pulling the + // real module in here would drag its database dependency into the mock + // factory. Kept faithful to imageProcessor.orientedDimensions. + orientedDimensions: jest.fn((m) => { + if (!m || !m.width || !m.height) return { width: null, height: null }; + const swap = m.orientation >= 5 && m.orientation <= 8; + return { width: swap ? m.height : m.width, height: swap ? m.width : m.height }; + }), withLocalCopy: jest.fn(async (key, fn) => fn(`/tmp/local-copy-${require('path').basename(key)}`) ), diff --git a/backend/src/routes/adminExternalMedia.js b/backend/src/routes/adminExternalMedia.js index 024e33b4..6d1a90d6 100644 --- a/backend/src/routes/adminExternalMedia.js +++ b/backend/src/routes/adminExternalMedia.js @@ -156,8 +156,8 @@ router.post('/events/:id/import-external', adminAuth, requirePermission('photos. let height = null; try { const metadata = await sharp(f.full).metadata(); - width = metadata.width || null; - height = metadata.height || null; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + ({ width, height } = require('../services/imageProcessor').orientedDimensions(metadata)); } catch (dimErr) { logger.warn(`Could not extract dimensions for ${f.rel}: ${dimErr.message}`); } diff --git a/backend/src/routes/adminPhotoDimensions.js b/backend/src/routes/adminPhotoDimensions.js index 00c797e0..7a949c7c 100644 --- a/backend/src/routes/adminPhotoDimensions.js +++ b/backend/src/routes/adminPhotoDimensions.js @@ -163,13 +163,15 @@ router.post('/repair-dimensions', adminAuth, requirePermission('photos.edit'), a } const metadata = await sharp(fullPath).metadata(); + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + const dims = require('../services/imageProcessor').orientedDimensions(metadata); - if (metadata.width && metadata.height) { + if (dims.width && dims.height) { await db('photos') .where({ id: photo.id }) .update({ - width: metadata.width, - height: metadata.height + width: dims.width, + height: dims.height }); successCount++; diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 6887d8c9..6474743e 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -609,8 +609,8 @@ router.post( let height = null; try { const meta = await sharp(tempPath).metadata(); - width = meta.width || null; - height = meta.height || null; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + ({ width, height } = require('../../services/imageProcessor').orientedDimensions(meta)); } catch { /* non-fatal */ } let thumbRel = null; diff --git a/backend/src/services/fileWatcher.js b/backend/src/services/fileWatcher.js index c1774e56..832bfe60 100644 --- a/backend/src/services/fileWatcher.js +++ b/backend/src/services/fileWatcher.js @@ -101,8 +101,10 @@ async function processNewPhoto(filePath) { if (!isVideo) { try { const metadata = await sharp(filePath).metadata(); - if (metadata.width && metadata.height) { - dimensions = { width: metadata.width, height: metadata.height }; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + const dims = require('./imageProcessor').orientedDimensions(metadata); + if (dims.width && dims.height) { + dimensions = { width: dims.width, height: dims.height }; } } catch (err) { logger.debug(`Could not read image dimensions for ${filename}: ${err.message}`); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 94cb35d3..9ba0b554 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -123,6 +123,31 @@ const contentTypeFor = (format) => { * import path passes a per-photo unique basename so two events both * referencing `IMG_0001.jpg` don't clobber each other's thumbnail. */ +/** + * The dimensions a viewer actually sees, given EXIF orientation (#1185). + * + * sharp reports `metadata.width`/`height` as the pixels are stored, not as + * they are displayed. Orientation values 5-8 carry a 90° rotation, so for + * those the two are swapped — which is why a portrait photo from a body that + * tags rather than rotates was landing in the database as landscape, and why + * masonry and justified layouts sized its tile with the wrong aspect ratio on + * top of the image itself being unrotated. + * + * Everything that renders these photos now applies `.rotate()`, so the stored + * numbers have to describe the rotated result to match. + * + * @param {Object} metadata - a sharp metadata object + * @returns {{ width: number|null, height: number|null }} + */ +function orientedDimensions(metadata) { + if (!metadata || !metadata.width || !metadata.height) return { width: null, height: null }; + const swap = metadata.orientation >= 5 && metadata.orientation <= 8; + return { + width: swap ? metadata.height : metadata.width, + height: swap ? metadata.width : metadata.height, + }; +} + async function generateThumbnail(imagePath, options = {}) { const sourceBasename = path.basename(imagePath); const outputBasename = options.outputBasename || sourceBasename; @@ -160,6 +185,18 @@ async function generateThumbnail(imagePath, options = {}) { failOn: 'none' }); + + // Apply EXIF orientation before resizing (#1185). Without this a photo + // whose Orientation tag is not 1 — routine for portrait shots on bodies + // that tag rather than rotate the sensor data — is resized from the raw + // pixels and comes out sideways. `.withMetadata(false)` below then strips + // the tag, so the browser has no hint left to correct it either. + // + // Unconditional: this pipeline never passes `animated: true`, so it + // already flattens a multi-frame source. Guarding on `pages` would protect + // an animation that was being discarded anyway while leaving the output in + // raw orientation against corrected dimensions. + sharpInstance = sharpInstance.rotate(); // Strip EXIF/metadata from thumbnails (privacy: prevent GPS leak etc.) sharpInstance = sharpInstance.withMetadata(false); @@ -406,6 +443,10 @@ async function generateHeroImage(imagePath, options = {}) { failOn: 'none' }); + + // EXIF orientation, same reasoning as generateThumbnail (#1185) — and + // unconditional for the same reason: no `animated: true` on the input. + sharpInstance = sharpInstance.rotate(); // Strip EXIF/metadata from hero images (privacy: prevent GPS leak etc.) sharpInstance = sharpInstance.withMetadata(false); @@ -619,6 +660,19 @@ async function generatePreviewImage(imagePath, options = {}) { animated: isAnimated, }); + + // EXIF orientation (#1185). Guarded here and not in the thumbnail/hero + // generators because this one DOES open multi-frame sources with + // `animated: true` above, and `.rotate()` would flatten them to a single + // frame — trading an animation for an orientation. + // + // Which leaves one corner unsolved: a multi-frame source that also carries + // an orientation tag keeps its raw orientation here while the thumbnail + // and the stored dimensions describe the rotated one. GIF has no EXIF at + // all and animated WebP effectively never sets it. + if (!isAnimated) { + sharpInstance = sharpInstance.rotate(); + } // Strip EXIF — same privacy reasoning as thumbnails/heroes. sharpInstance = sharpInstance.withMetadata(false); @@ -797,6 +851,7 @@ async function extractCaptureDate(imagePath) { } module.exports = { + orientedDimensions, generateThumbnail, isThumbnailValid, ensureThumbnail, diff --git a/backend/src/services/photoProcessor.js b/backend/src/services/photoProcessor.js index 83d69f8c..a18889f9 100644 --- a/backend/src/services/photoProcessor.js +++ b/backend/src/services/photoProcessor.js @@ -149,11 +149,10 @@ async function processUploadedPhotos(files, eventId, uploadedBy = 'admin', categ try { const sharp = require('sharp'); const metadata = await sharp(tempPath).metadata(); - if (metadata.width && metadata.height) { - imageMetadata = { - width: metadata.width, - height: metadata.height - }; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + const dims = require('./imageProcessor').orientedDimensions(metadata); + if (dims.width && dims.height) { + imageMetadata = { width: dims.width, height: dims.height }; } } catch (metadataError) { logger.warn(`Could not extract image dimensions for ${file.originalname}:`, metadataError.message); @@ -468,9 +467,11 @@ async function processPhoto(photoId) { try { const sharp = require('sharp'); const metadata = await sharp(localPath).metadata(); - if (metadata.width && metadata.height) { - updateData.width = metadata.width; - updateData.height = metadata.height; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + const dims = require('./imageProcessor').orientedDimensions(metadata); + if (dims.width && dims.height) { + updateData.width = dims.width; + updateData.height = dims.height; } } catch (e) { logger.warn(`processPhoto: dimensions extraction failed for ${photoId}`, { error: e.message }); diff --git a/backend/src/services/photoReplacementService.js b/backend/src/services/photoReplacementService.js index 3680cbbd..4ba9be3b 100644 --- a/backend/src/services/photoReplacementService.js +++ b/backend/src/services/photoReplacementService.js @@ -65,8 +65,8 @@ async function replacePhoto(existingPhoto, newFileTempPath, { originalFilename, let height = null; try { const metadata = await sharp(newFileTempPath).metadata(); - width = metadata.width || null; - height = metadata.height || null; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + ({ width, height } = require('./imageProcessor').orientedDimensions(metadata)); } catch { // Non-image or corrupt } diff --git a/backend/src/services/s3AutoImporter.js b/backend/src/services/s3AutoImporter.js index f841059e..eceb23f5 100644 --- a/backend/src/services/s3AutoImporter.js +++ b/backend/src/services/s3AutoImporter.js @@ -110,8 +110,10 @@ async function processEvent(event, storage) { try { dimensions = await withLocalCopy(entry.key, async (localPath) => { const metadata = await sharp(localPath).metadata(); - if (metadata.width && metadata.height) { - return { width: metadata.width, height: metadata.height }; + // Oriented, not raw — see imageProcessor.orientedDimensions (#1185). + const dims = require('./imageProcessor').orientedDimensions(metadata); + if (dims.width && dims.height) { + return { width: dims.width, height: dims.height }; } return null; }); diff --git a/backend/src/services/watermarkService.js b/backend/src/services/watermarkService.js index ccb9c6ee..5f859eb3 100644 --- a/backend/src/services/watermarkService.js +++ b/backend/src/services/watermarkService.js @@ -1,4 +1,5 @@ const sharp = require('sharp'); +const { orientedDimensions } = require('./imageProcessor'); const path = require('path'); const fs = require('fs').promises; const { db } = require('../database/db'); @@ -105,8 +106,21 @@ class WatermarkService { } // Load the main image - const image = sharp(imagePath); - const metadata = await image.metadata(); + // .rotate() for the same reason as the other generators (#1185): sharp + // decodes pixels as stored, so an orientation-tagged photo would be + // composited and re-encoded sideways — and gallery.js serves + // watermark_path ahead of the original, so this is exactly what a guest + // sees on a watermarked gallery. + const image = sharp(imagePath).rotate(); + + // Deliberately NOT `await image.metadata()`: .rotate() does not change + // what metadata() reports — a 400x200 source tagged orientation 6 still + // reads 400x200 there, even though the pipeline now emits 200x400. Every + // use below is positioning, so it has to be the DISPLAYED size or the + // mark lands against the wrong axis. + const rawMetadata = await sharp(imagePath).metadata(); + const oriented = orientedDimensions(rawMetadata); + const metadata = { ...rawMetadata, width: oriented.width, height: oriented.height }; let watermarkBuffer; let watermarkMetadata; @@ -180,10 +194,15 @@ class WatermarkService { ); // Apply watermark with high quality output to preserve original image quality + // Floored: getPositionCoordinates derives from the SVG's estimated text + // extent, which is fractional, and sharp rejects a non-integer offset + // outright — applyWatermark then catches its own error and silently + // returns the unwatermarked original. Whether it landed on a whole pixel + // was previously luck. let watermarkedImage = image.composite([{ input: watermarkBuffer, - top: position.top, - left: position.left + top: Math.max(0, Math.floor(position.top)), + left: Math.max(0, Math.floor(position.left)) }]); // Preserve original format with high quality settings