* feat(gallery): sized preview tiers so phones stop pulling 1920px (#1095) A phone can display ~1170px at most, but the preview tier is a single 1920px JPEG with no size parameter — so every lightbox swipe ships roughly twice the bytes it can use, and the slide track preloads neighbours, which multiplies it. On the reporter's all-external install a null preview_url falls back to the untouched NAS original, which makes it worse again. Backend: ?w= on the gallery preview route, whitelisted to 640/1280/1920. A whitelist rather than a free-form width because every distinct value is a permanent rendition on disk — an open parameter is an invitation to fill the volume. Unrecognised or absent values fall through to the canonical 1920 preview, so old clients and hand-typed URLs behave exactly as today. Extra tiers are cache, not state: ensurePreviewImageAtWidth keys them by width, looks them up in storage and generates on miss, and never writes photos.preview_path. That column owns the canonical rendition, and threading a width through it would mean the last size anyone requested silently becomes "the" preview. Requesting 1920 resolves to the existing preview rather than a w1920 duplicate, so no install grows a second copy of every preview it already has. The tier is part of the ETag. Without it a client holding the 1920 rendition gets a 304 for its 640 request and renders the wrong size, which is this feature inverted. Frontend: the lightbox picks a tier from innerWidth x devicePixelRatio, capped at DPR 3 — uncapped, a DPR-10 device asks for 3900px and lands straight back on the desktop rendition. At the top tier the URL is left byte-identical so existing caches and ETags stay valid and desktop sees no change at all. saveData and a 2g/3g effectiveType drop one tier; both are Chromium-only, so they are a bonus rather than the mechanism. Grid thumbnails are NOT tiered here, deliberately. generateThumbnail resolves its width from admin settings rather than an argument, so tiering it is a separate change — and shipping a srcset whose candidates the server ignores would be worse than shipping none: the browser would take the "600w" candidate, receive the 300px image and upscale it, which is the reported softness made slightly worse. That half of #1095 lands separately. * fix(gallery): scope tier keys per photo, size by long edge, clean up tiers External review. Three findings against the tier work, one a cross-gallery leak. The tier cache key was the photo's BASENAME. Managed uploads keep camera basenames, so two events can each hold an IMG_0001.jpg — and a tier is served straight from a cache hit without re-reading the source, so the second gallery gets the first gallery's photo. Keys are now scoped by photo id for every source type. The RAW branch passed proc.outputBasename, which would have dropped that scoping again; it now passes the scoped name. Tier selection used viewport WIDTH, but ?w= bounds the LONG edge (fit:'inside'). On a 390x844 phone at DPR 3 a 2:3 portrait is bound by height and renders ~1755 device px, so width-only picked 1280 and made portraits softer than today; landscape on the same phone needs ~1170. It now computes the rendered long edge from the photo's own dimensions and falls back to the top tier — today's behaviour — when they are unknown. Tiers live outside photos.preview_path, so nothing else knew they existed: delete, bulk-delete and archive left them orphaned in previews/ forever, and regenerate-previews refreshed only the canonical rendition while phones kept the stale copy. previewTierKeys derives them from the same deterministic scheme and all four paths clean up. Deliberately outside the preview_path guard — a tier can exist when the canonical rendition never did, so keying cleanup off preview_path would strand precisely the photos only ever viewed on a phone. The existing tier tests encoded the old width-only semantics and were updated rather than kept; that is a behaviour change, not a test fix. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
co-authored by
Paul Nothaft
parent
0b886ed942
commit
011f6ae7ec
@@ -677,6 +677,11 @@ router.delete('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.
|
||||
if (photo.preview_path) {
|
||||
await storage.delete(photo.preview_path).catch(() => {});
|
||||
}
|
||||
// Outside the preview_path guard on purpose: a responsive tier (#1095) can
|
||||
// exist when the canonical rendition never did — they are generated
|
||||
// independently, on demand — so keying their cleanup off preview_path
|
||||
// would strand exactly the photos that were only ever viewed on a phone.
|
||||
await require('../services/imageProcessor').deletePreviewTiers(photo);
|
||||
|
||||
// Delete pre-generated watermark if exists
|
||||
if (photo.watermark_path) {
|
||||
@@ -838,6 +843,9 @@ router.post('/:eventId/photos/bulk-delete', adminAuth, requirePermission('photos
|
||||
if (photo.preview_path) {
|
||||
await storage.delete(photo.preview_path).catch(() => {});
|
||||
}
|
||||
// Outside the guard: a tier can exist when the canonical rendition never
|
||||
// did, so keying cleanup off preview_path would strand phone-only photos.
|
||||
await require('../services/imageProcessor').deletePreviewTiers(photo);
|
||||
if (photo.watermark_path) {
|
||||
await watermarkGeneratorService.deleteForPhoto(photo.id);
|
||||
}
|
||||
|
||||
@@ -232,6 +232,12 @@ router.post('/regenerate-previews', adminAuth, requirePermission('photos.edit'),
|
||||
// Force regeneration regardless of existing preview state by
|
||||
// nulling the cached path so ensurePreviewImage doesn't
|
||||
// short-circuit on a stale isPreviewValid check.
|
||||
// Drop the responsive tiers first (#1095). They are cached by width
|
||||
// outside preview_path, so regenerating only the canonical rendition
|
||||
// leaves phones served the stale 640/1280 copy indefinitely — which
|
||||
// is precisely the case this endpoint exists for (a replaced
|
||||
// reference source, or a corrupted rendition).
|
||||
await require('../services/imageProcessor').deletePreviewTiers(photo);
|
||||
const newPreviewPath = await ensurePreviewImage({ ...photo, preview_path: null });
|
||||
if (newPreviewPath) {
|
||||
successCount++;
|
||||
|
||||
@@ -2449,10 +2449,20 @@ router.get('/:slug/preview/:photoId',
|
||||
return res.redirect(withPreview(req, `/api/gallery/${req.params.slug}/photo/${photoId}`));
|
||||
}
|
||||
|
||||
// Responsive tier (#1095). Whitelisted only — an open ?w= would let
|
||||
// anyone fill the disk with renditions nobody asked for. An unrecognised
|
||||
// or absent value falls through to the canonical 1920 preview, so old
|
||||
// clients and hand-typed URLs behave exactly as before.
|
||||
const { PREVIEW_WIDTHS, normalizeTierWidth, ensurePreviewImageAtWidth } =
|
||||
require('../services/imageProcessor');
|
||||
const tierWidth = normalizeTierWidth(req.query.w, PREVIEW_WIDTHS);
|
||||
|
||||
// Lazy generation: ensurePreviewImage returns null on any
|
||||
// failure (corrupt source, sharp OOM, storage unavailable, …).
|
||||
// Fall back to the original so the lightbox always renders.
|
||||
const previewPath = await ensurePreviewImage(photo);
|
||||
const previewPath = tierWidth
|
||||
? (await ensurePreviewImageAtWidth(photo, tierWidth)) || (await ensurePreviewImage(photo))
|
||||
: await ensurePreviewImage(photo);
|
||||
if (!previewPath) {
|
||||
logger.warn(`Failed to generate preview for photo ${photoId}, falling back to original`);
|
||||
return res.redirect(withPreview(req, `/api/gallery/${req.params.slug}/photo/${photoId}`));
|
||||
@@ -2472,7 +2482,10 @@ router.get('/:slug/preview/:photoId',
|
||||
const watermarkHash = watermarkSettings?.enabled
|
||||
? `-wm${watermarkSettings.opacity}${watermarkSettings.position}${watermarkSettings.size}`
|
||||
: '-nowm';
|
||||
const etag = `"preview-${photoId}-${mtimeMs}${watermarkHash}"`;
|
||||
// Tier is part of the etag: without it a client that already holds the
|
||||
// 1920 rendition would get a 304 for its 640 request and render the
|
||||
// wrong size, which is the whole point of the feature inverted.
|
||||
const etag = `"preview-${photoId}-${tierWidth || 'def'}-${mtimeMs}${watermarkHash}"`;
|
||||
if (req.headers['if-none-match'] === etag) {
|
||||
return res.status(304).end();
|
||||
}
|
||||
|
||||
@@ -219,6 +219,9 @@ async function archiveEvent(event) {
|
||||
if (photo.preview_path) {
|
||||
await storage.delete(photo.preview_path).catch(() => {});
|
||||
}
|
||||
// Outside the guard: a tier can exist when the canonical rendition never
|
||||
// did, so keying cleanup off preview_path would strand phone-only photos.
|
||||
await require('./imageProcessor').deletePreviewTiers(photo);
|
||||
// Best effort: remove watermarked variants too if a refactor added them.
|
||||
if (photo.watermark_path) {
|
||||
await storage.delete(photo.watermark_path).catch(() => {});
|
||||
|
||||
@@ -108,6 +108,23 @@ const DEFAULT_HERO_QUALITY = 85;
|
||||
const DEFAULT_PREVIEW_LONG_EDGE = 1920;
|
||||
const DEFAULT_PREVIEW_QUALITY = 85;
|
||||
|
||||
// Responsive tiers (#1095). A whitelist, not a free-form ?w=: an open
|
||||
// parameter lets anyone fill the disk with renditions nobody asked for, and
|
||||
// every distinct value is a permanent cache entry.
|
||||
//
|
||||
// 1920 stays the default so existing preview_path rows keep their meaning and
|
||||
// nothing regenerates on upgrade. The smaller tiers exist because a phone can
|
||||
// show ~1170px at most, so the 1920 tier ships roughly twice the bytes it can
|
||||
// use on every lightbox swipe.
|
||||
const PREVIEW_WIDTHS = [640, 1280, 1920];
|
||||
const THUMBNAIL_WIDTHS = [300, 600, 900];
|
||||
|
||||
/** Whitelist a requested width, or null. Callers treat null as "use default". */
|
||||
function normalizeTierWidth(requested, allowed) {
|
||||
const n = parseInt(requested, 10);
|
||||
return Number.isFinite(n) && allowed.includes(n) ? n : null;
|
||||
}
|
||||
|
||||
// Helper to parse setting value (handles both JSON-encoded and plain values)
|
||||
function parseSettingValue(value) {
|
||||
if (value === null || value === undefined) {
|
||||
@@ -578,7 +595,12 @@ async function ensureHeroImage(photo) {
|
||||
*/
|
||||
async function generatePreviewImage(imagePath, options = {}) {
|
||||
const filename = options.outputBasename || path.basename(imagePath);
|
||||
const previewFilename = `preview_${filename}`;
|
||||
// Non-default tiers get their own key so they cannot collide with the
|
||||
// canonical preview the DB column points at.
|
||||
const widthTag = options.longEdge && options.longEdge !== DEFAULT_PREVIEW_LONG_EDGE
|
||||
? `w${options.longEdge}_`
|
||||
: '';
|
||||
const previewFilename = `preview_${widthTag}${filename}`;
|
||||
const previewRelKey = path.posix.join('previews', previewFilename);
|
||||
const storage = getStorage();
|
||||
|
||||
@@ -666,6 +688,113 @@ async function isPreviewValid(previewPath) {
|
||||
* withLocalCopy, and the throw put every lightbox open back on the full-size
|
||||
* original — the exact cost the preview tier (#492) exists to avoid.
|
||||
*/
|
||||
/**
|
||||
* A preview at a specific tier width (#1095).
|
||||
*
|
||||
* Deliberately separate from ensurePreviewImage rather than a parameter on it.
|
||||
* That function owns photos.preview_path — one column, one canonical rendition
|
||||
* — and threading a width through it would either overwrite that column with
|
||||
* whatever size was asked for last, or need a column per tier. Extra tiers are
|
||||
* pure cache instead: keyed by width, looked up in storage, generated on miss,
|
||||
* never written to the row.
|
||||
*
|
||||
* Returns null on anything unexpected so callers fall back to the default
|
||||
* tier, which is always the honest thing to serve.
|
||||
*/
|
||||
/**
|
||||
* Storage keys for every responsive tier of a photo (#1095).
|
||||
*
|
||||
* Tiers live outside photos.preview_path deliberately — that column owns the
|
||||
* canonical rendition — but that also means nothing else knows they exist.
|
||||
* Delete, bulk-delete, archive and regenerate all operate on preview_path
|
||||
* alone, so without this the tiers survive their own photo: orphaned on disk
|
||||
* forever after a delete, and served stale forever after a regenerate.
|
||||
*
|
||||
* Derived rather than tracked: the key scheme is deterministic, so there is
|
||||
* nothing to keep in sync and no migration.
|
||||
*/
|
||||
function previewTierKeys(photo) {
|
||||
if (!photo) return [];
|
||||
const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
|
||||
const sourceBasename = path.basename(
|
||||
(isExternal ? (photo.external_relpath || photo.filename) : photo.path) || `photo-${photo.id}`
|
||||
);
|
||||
const outputBasename = `p${photo.id}_${sourceBasename}`;
|
||||
return PREVIEW_WIDTHS
|
||||
.filter((w) => w !== DEFAULT_PREVIEW_LONG_EDGE)
|
||||
.map((w) => path.posix.join('previews', `preview_w${w}_${outputBasename}`));
|
||||
}
|
||||
|
||||
/** Best-effort removal of every responsive tier for a photo. */
|
||||
async function deletePreviewTiers(photo) {
|
||||
const storage = getStorage();
|
||||
await Promise.all(previewTierKeys(photo).map((k) => storage.delete(k).catch(() => {})));
|
||||
}
|
||||
|
||||
async function ensurePreviewImageAtWidth(photo, width) {
|
||||
if (!width || width === DEFAULT_PREVIEW_LONG_EDGE) return ensurePreviewImage(photo);
|
||||
|
||||
const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver');
|
||||
const storage = getStorage();
|
||||
|
||||
let event;
|
||||
try {
|
||||
event = await db('events').where('id', photo.event_id).first();
|
||||
} catch (e) {
|
||||
return null;
|
||||
}
|
||||
if (!event) return null;
|
||||
|
||||
const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference';
|
||||
const sourceBasename = path.basename(
|
||||
(isExternal ? (photo.external_relpath || photo.filename) : photo.path) || `photo-${photo.id}`
|
||||
);
|
||||
// ALWAYS scoped by photo id, managed rows included. Basenames are not unique
|
||||
// across events — two galleries can each hold an IMG_0001.jpg — and because a
|
||||
// tier is served straight from a cache hit without re-reading the source, a
|
||||
// collision hands one gallery's photo to another. Scoping by id is what makes
|
||||
// the cache safe to trust; it is not a tidiness choice.
|
||||
const outputBasename = `p${photo.id}_${sourceBasename}`;
|
||||
const key = path.posix.join('previews', `preview_w${width}_${outputBasename}`);
|
||||
|
||||
// Cache hit: nothing to do. This is the common path once a gallery has been
|
||||
// browsed at a given size.
|
||||
try {
|
||||
if (await storage.stat(key)) return key;
|
||||
} catch (e) {
|
||||
// fall through and regenerate
|
||||
}
|
||||
|
||||
try {
|
||||
if (isExternal) {
|
||||
const localPath = resolvePhotoFilePath(event, photo);
|
||||
return await generatePreviewImage(localPath, {
|
||||
regenerate: true, outputBasename, longEdge: width,
|
||||
});
|
||||
}
|
||||
const sourceKey = resolvePhotoStorageKey(event, photo);
|
||||
if (!sourceKey) return null;
|
||||
return await withLocalCopy(sourceKey, async (localPath) => {
|
||||
const proc = await withProcessableImage(localPath, sourceKey);
|
||||
try {
|
||||
// outputBasename, not proc.outputBasename: the RAW path returns the
|
||||
// source basename, which would drop the photo-id scoping above and
|
||||
// reintroduce the cross-gallery collision.
|
||||
return await generatePreviewImage(proc.path, {
|
||||
regenerate: true,
|
||||
outputBasename,
|
||||
longEdge: width,
|
||||
});
|
||||
} finally {
|
||||
proc.cleanup();
|
||||
}
|
||||
});
|
||||
} catch (e) {
|
||||
logger.warn(`Preview tier w${width} failed for photo ${photo.id}: ${e.message}`);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
async function ensurePreviewImage(photo) {
|
||||
const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver');
|
||||
|
||||
@@ -851,6 +980,12 @@ async function resizeToBox(inputBuffer, box, options = {}) {
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
ensurePreviewImageAtWidth,
|
||||
previewTierKeys,
|
||||
deletePreviewTiers,
|
||||
PREVIEW_WIDTHS,
|
||||
THUMBNAIL_WIDTHS,
|
||||
normalizeTierWidth,
|
||||
resizeToBox,
|
||||
generateThumbnail,
|
||||
isThumbnailValid,
|
||||
|
||||
Reference in New Issue
Block a user