fix(faces): face avatars were cropped against a cropped rendition (#1100)
* fix(faces): face avatars were cropped against a cropped rendition Found while triaging #1096, which reported the People manager showing unusable cluster covers — a bare shoulder, the back of a head, a patch of background — and asked for more sample faces to compensate. Most of that is not a detector problem and not a UI limitation. It is a bug. faceCropStyle positions an avatar by scaling the WHOLE frame and offsetting so the face lands centre. That holds only while the rendition shown is the entire image at a uniform scale. Thumbnails are not: imageProcessor.js:93 DEFAULT_THUMBNAIL_FIT = 'inside' migration 040:6 thumbnail_fit seeded to 'cover' imageProcessor.js:229 fit: settings.fit The 'inside' constant is only a fallback for a missing settings row, and the row is seeded on every install — so thumbnails are centre-cropped essentially everywhere, and every face avatar rendered against one is silently offset on any non-square photo. The reporter read the setting as safe because of that constant, and the code comment at :87-92 says the same thing; all three places disagree with what is actually stored. It presents as a bad detector, which is why it survived: the boxes are right, the frame they are drawn against is not. All three surfaces — the admin manager and the guest-facing strip and sheet — now read a preview, which uses fit: 'inside' and is therefore the whole frame. At w=640: plenty for a 64px avatar at DPR 3, and small enough that a strip of a dozen people does not pull a dozen 1920px renditions. Face scanning already calls ensurePreviewImage for anything it scans, so a preview exists for every photo that has a face. Adds the admin preview route the manager needed; the gallery already had one. Both whitelist ?w= the same way. The first version of the call-site test passed with every surface still reading thumbnail_url, because an import alone satisfied it. It now matches inside the src={...} expression, and each of the three surfaces was individually reverted to confirm the test fails. * fix(faces): size the face tier by bbox, and keep admin_preview auth The face half of the external review; the tier-key and long-edge fixes live on the #1099 branch this is stacked on. Face avatars used one fixed 640 tier. In a 6000px group shot a 200px face is ~21px there, and faceCropStyle then blows that up ~9x to fill a 64px avatar at DPR 3 — mush, and indistinguishable from the mis-positioning bug this PR exists to fix. The tier is now derived from the bbox's share of the frame, so a face across a hall gets 1920 and a close-up still gets 640. The synthesized face URL also dropped admin_preview. verifyGalleryAccess only accepts the admin cookie when admin_preview=1 is on the request (middleware/gallery.js:28), and the preview flow deliberately mints no gallery JWT — so every avatar 401'd in exactly the mode an admin uses to check a gallery before sending it to a client. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
56034af0f0
commit
b3a7ab27ea
@@ -22,6 +22,7 @@ import { X, Check, Merge, Scissors, EyeOff, Ban, Loader2 } from 'lucide-react';
|
||||
import { Button, Loading } from '../common';
|
||||
import { api } from '../../config/api';
|
||||
import { faceCropStyle } from '../gallery/faceCrop';
|
||||
import { adminFacePreviewUrl } from '../gallery/imageTiers';
|
||||
|
||||
interface AdminPerson {
|
||||
id: number;
|
||||
@@ -95,7 +96,7 @@ const FaceThumb: React.FC<{
|
||||
style={{ width: size, height: size, opacity: dim ? 0.4 : 1 }}
|
||||
>
|
||||
<img
|
||||
src={`/api/admin/photos/${eventId}/thumbnail/${photoId}`}
|
||||
src={adminFacePreviewUrl(eventId, photoId)}
|
||||
alt=""
|
||||
loading="lazy"
|
||||
style={style || { width: '100%', height: '100%', objectFit: 'cover' }}
|
||||
|
||||
@@ -5,6 +5,7 @@ import { useTranslation } from 'react-i18next';
|
||||
import { AuthenticatedImage } from '../common/AuthenticatedImage';
|
||||
import type { GalleryPerson, Photo } from '../../types';
|
||||
import { faceCropStyle } from './faceCrop';
|
||||
import { facePreviewUrl } from './imageTiers';
|
||||
|
||||
/**
|
||||
* "Show all" people (#1074).
|
||||
@@ -113,7 +114,7 @@ export const PeopleSheet: React.FC<PeopleSheetProps> = ({
|
||||
>
|
||||
{photo && (
|
||||
<AuthenticatedImage
|
||||
src={photo.thumbnail_url || photo.url}
|
||||
src={facePreviewUrl(slug, photo, person.cover) || photo.thumbnail_url || photo.url}
|
||||
alt=""
|
||||
isGallery
|
||||
slug={slug}
|
||||
|
||||
@@ -5,6 +5,7 @@ import { useTranslation } from 'react-i18next';
|
||||
import { AuthenticatedImage } from '../common/AuthenticatedImage';
|
||||
import type { GalleryPerson, Photo } from '../../types';
|
||||
import { faceCropStyle } from './faceCrop';
|
||||
import { facePreviewUrl } from './imageTiers';
|
||||
|
||||
/**
|
||||
* "People in this gallery" (#1074).
|
||||
@@ -72,7 +73,7 @@ const PersonAvatar: React.FC<PersonAvatarProps> = ({
|
||||
>
|
||||
{photo && person.cover ? (
|
||||
<AuthenticatedImage
|
||||
src={photo.thumbnail_url || photo.url}
|
||||
src={facePreviewUrl(slug, photo, person.cover) || photo.thumbnail_url || photo.url}
|
||||
alt=""
|
||||
isGallery
|
||||
slug={slug}
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
/**
|
||||
* Face avatars must come from a whole-frame rendition.
|
||||
*
|
||||
* faceCropStyle positions a crop by scaling the ENTIRE frame and offsetting so
|
||||
* the face lands centre. That is only valid while the rendition shown is the
|
||||
* whole image at a uniform scale.
|
||||
*
|
||||
* Thumbnails are not. `thumbnail_fit` is seeded to 'cover' by migration
|
||||
* 040_add_thumbnail_settings, and generateThumbnail passes it straight to
|
||||
* sharp — so thumbnails are centre-cropped on essentially every install. The
|
||||
* DEFAULT_THUMBNAIL_FIT = 'inside' constant only applies when the setting row
|
||||
* is absent, and it never is.
|
||||
*
|
||||
* The result was that every face avatar — admin manager and the guest-facing
|
||||
* strip and sheet — was silently offset on any non-square photo, which reads
|
||||
* as a bad detector rather than a positioning bug. That is what made clusters
|
||||
* show a shoulder, the back of a head, or a patch of background.
|
||||
*
|
||||
* Previews use fit: 'inside', so they are the whole frame.
|
||||
*/
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import fs from 'fs';
|
||||
import path from 'path';
|
||||
|
||||
import { facePreviewUrl, adminFacePreviewUrl, FACE_CROP_WIDTH } from '../imageTiers';
|
||||
|
||||
describe('face avatars use a whole-frame rendition', () => {
|
||||
const photo = { id: 42, preview_url: '/api/gallery/wed/preview/42' };
|
||||
|
||||
it('never points a face crop at a thumbnail', () => {
|
||||
// The regression, stated directly: any URL containing /thumbnail/ is
|
||||
// cropped by fit:'cover' and will mis-place the face.
|
||||
expect(facePreviewUrl('wed', photo)).not.toContain('/thumbnail/');
|
||||
expect(adminFacePreviewUrl(7, 42)).not.toContain('/thumbnail/');
|
||||
});
|
||||
|
||||
it('requests the small preview tier rather than the full 1920', () => {
|
||||
// A 64px avatar does not need 1920px, and the strip renders one per
|
||||
// person — pulling the full tier for each would be its own problem.
|
||||
expect(facePreviewUrl('wed', photo)).toBe(`/api/gallery/wed/preview/42?w=${FACE_CROP_WIDTH}`);
|
||||
expect(adminFacePreviewUrl(7, 42)).toBe(`/api/admin/photos/7/preview/42?w=${FACE_CROP_WIDTH}`);
|
||||
});
|
||||
|
||||
it('prefers the served preview_url so a watermark query survives', () => {
|
||||
const wm = { id: 9, preview_url: '/api/gallery/wed/preview/9?wm=1' };
|
||||
expect(facePreviewUrl('wed', wm)).toBe(`/api/gallery/wed/preview/9?wm=1&w=${FACE_CROP_WIDTH}`);
|
||||
});
|
||||
|
||||
it('still resolves when preview_url is absent', () => {
|
||||
// preview_url is only emitted when lightbox previews are enabled, but face
|
||||
// scanning calls ensurePreviewImage for everything it scans — so a preview
|
||||
// exists on disk for any photo that has a face, and the route can serve it.
|
||||
expect(facePreviewUrl('wed', { id: 5 }))
|
||||
.toBe(`/api/gallery/wed/preview/5?w=${FACE_CROP_WIDTH}`);
|
||||
});
|
||||
|
||||
it('returns null rather than a wrong URL when it cannot build one', () => {
|
||||
// Callers fall back to thumbnail_url — still mis-positioned, but visible,
|
||||
// which beats a broken image.
|
||||
expect(facePreviewUrl(undefined, { id: 5 })).toBeNull();
|
||||
expect(facePreviewUrl('wed', null)).toBeNull();
|
||||
expect(facePreviewUrl('wed', undefined)).toBeNull();
|
||||
});
|
||||
|
||||
it('sizes the tier from how much of the frame the face fills', () => {
|
||||
Object.defineProperty(window, 'devicePixelRatio', { value: 3, configurable: true });
|
||||
const frame = { id: 1, width: 6000, height: 4000 };
|
||||
|
||||
// A face across a hall: 200px in a 6000px frame is ~21px at the 640 tier,
|
||||
// which faceCropStyle then blows up ~9x. Indistinguishable from the
|
||||
// mis-positioning bug this whole change is about.
|
||||
expect(facePreviewUrl('wed', frame, { bbox: [0, 0, 200, 200] })).toContain('w=1920');
|
||||
|
||||
// A close-up needs nothing like that.
|
||||
expect(facePreviewUrl('wed', frame, { bbox: [0, 0, 3000, 3000] })).toContain('w=640');
|
||||
});
|
||||
|
||||
it('falls back to the fixed tier when the bbox is unknown', () => {
|
||||
expect(facePreviewUrl('wed', { id: 1, width: 6000, height: 4000 }, null))
|
||||
.toContain(`w=${FACE_CROP_WIDTH}`);
|
||||
});
|
||||
|
||||
it('carries admin_preview so avatars do not 401 in preview mode', () => {
|
||||
// verifyGalleryAccess only accepts the admin cookie when admin_preview=1 is
|
||||
// on the request (middleware/gallery.js:28), and the preview flow mints no
|
||||
// gallery JWT — so without this every avatar breaks in exactly the mode an
|
||||
// admin uses to check a gallery before sending it.
|
||||
const orig = window.location.search;
|
||||
Object.defineProperty(window, 'location', {
|
||||
value: { search: '?admin_preview=1' }, configurable: true,
|
||||
});
|
||||
expect(facePreviewUrl('wed', { id: 5 })).toContain('admin_preview=1');
|
||||
Object.defineProperty(window, 'location', { value: { search: orig }, configurable: true });
|
||||
});
|
||||
|
||||
// The helper being correct is not the contract — the call sites using it is.
|
||||
// Every assertion above passes with all three surfaces still reading
|
||||
// thumbnail_url, which is exactly the bug. So pin the call sites.
|
||||
describe('the three face surfaces actually use it', () => {
|
||||
const surfaces = [
|
||||
['PeopleStrip', '../PeopleStrip.tsx', 'facePreviewUrl'],
|
||||
['PeopleSheet', '../PeopleSheet.tsx', 'facePreviewUrl'],
|
||||
['PeopleManagerModal', '../../admin/PeopleManagerModal.tsx', 'adminFacePreviewUrl'],
|
||||
] as const;
|
||||
|
||||
it.each(surfaces)('%s builds its avatar src from %s', (_name, rel, helper) => {
|
||||
const src = fs.readFileSync(path.join(__dirname, rel), 'utf8');
|
||||
|
||||
// Matched inside the src={...} expression, not merely present in the
|
||||
// file: an import alone satisfies toContain(helper) while the avatar
|
||||
// still reads thumbnail_url, which is the bug wearing the fix's clothes.
|
||||
expect(src).toMatch(new RegExp(`src=\\{[^}]*${helper}\\(`));
|
||||
|
||||
// And no face surface may reintroduce a hardcoded thumbnail path.
|
||||
expect(src).not.toContain('/thumbnail/${photoId}');
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -15,6 +15,12 @@
|
||||
|
||||
export const PREVIEW_WIDTHS = [640, 1280, 1920] as const;
|
||||
|
||||
/** Fallback tier for face avatars when the face's size in frame is unknown. */
|
||||
export const FACE_CROP_WIDTH = 640;
|
||||
|
||||
/** CSS px of the avatar the crop has to fill; used to size the tier. */
|
||||
const FACE_AVATAR_PX = 64;
|
||||
|
||||
// Thumbnail tiers are NOT here yet. Emitting a srcset whose candidates the
|
||||
// server ignores is worse than emitting none: the browser would pick the
|
||||
// "600w" candidate, receive the 300px image, and upscale it — the exact
|
||||
@@ -41,7 +47,8 @@ function smallestCovering(needed: number, tiers: readonly number[]): number {
|
||||
* and lands back on the desktop rendition, which is the thing being fixed.
|
||||
*
|
||||
* Without photo dimensions there is nothing to reason about, so it falls back
|
||||
* to the largest edge the viewport could demand — i.e. today's behaviour.
|
||||
* to the largest edge the viewport could possibly demand — which resolves to
|
||||
* the top tier, i.e. exactly today's behaviour.
|
||||
*/
|
||||
export function viewportPreviewWidth(photo?: { width?: number | null; height?: number | null }): number {
|
||||
if (typeof window === 'undefined') return PREVIEW_WIDTHS[PREVIEW_WIDTHS.length - 1];
|
||||
@@ -58,7 +65,8 @@ export function viewportPreviewWidth(photo?: { width?: number | null; height?: n
|
||||
// Contained in the viewport, so one axis binds; the rendered long edge is
|
||||
// the source long edge times that scale.
|
||||
const scale = Math.min(vw / pw, vh / ph);
|
||||
return smallestCovering(Math.round(Math.max(pw, ph) * scale * dpr), PREVIEW_WIDTHS);
|
||||
const renderedLongEdge = Math.max(pw, ph) * scale * dpr;
|
||||
return smallestCovering(Math.round(renderedLongEdge), PREVIEW_WIDTHS);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -103,3 +111,87 @@ export function previewUrlForViewport(
|
||||
if (width === PREVIEW_WIDTHS[PREVIEW_WIDTHS.length - 1]) return previewUrl;
|
||||
return withWidth(previewUrl, width);
|
||||
}
|
||||
|
||||
/**
|
||||
* An aspect-preserved rendition for a face avatar (#1096).
|
||||
*
|
||||
* NOT the thumbnail. faceCropStyle positions the crop by scaling the whole
|
||||
* frame and offsetting so the face lands centre — which holds only while the
|
||||
* rendition IS the whole frame. thumbnail_fit is seeded to 'cover' (migration
|
||||
* 040_add_thumbnail_settings), so thumbnails are centre-cropped on essentially
|
||||
* every install and every avatar rendered against one is silently offset. It
|
||||
* presents as a bad detector: a shoulder, the back of a head, a patch of
|
||||
* background.
|
||||
*
|
||||
* Previews use fit: 'inside', so they are the whole frame. 640 is plenty for a
|
||||
* 64px avatar even at DPR 3, and face scanning has already generated a preview
|
||||
* for any photo that has a face — faceProcessor calls ensurePreviewImage to
|
||||
* get something to scan — so this asks for a rendition that is already there.
|
||||
*/
|
||||
export function facePreviewUrl(
|
||||
slug: string | undefined,
|
||||
photo: {
|
||||
id: number | string;
|
||||
preview_url?: string | null;
|
||||
width?: number | null;
|
||||
height?: number | null;
|
||||
} | null | undefined,
|
||||
cover?: { bbox: number[] } | null,
|
||||
): string | null {
|
||||
if (!photo) return null;
|
||||
const width = faceTierWidth(photo, cover);
|
||||
|
||||
// preview_url carries the watermark query when the server emitted one, so
|
||||
// prefer it; it is only absent when lightbox previews are off.
|
||||
if (photo.preview_url) return withWidth(photo.preview_url, width);
|
||||
if (!slug) return null;
|
||||
|
||||
// Carry admin_preview through. verifyGalleryAccess only accepts the admin
|
||||
// cookie when admin_preview=1 is on the request (middleware/gallery.js:28),
|
||||
// and the preview flow deliberately mints no gallery JWT — so a synthesized
|
||||
// URL without it 401s, and every avatar breaks in exactly the mode an admin
|
||||
// uses to check their gallery before sending it.
|
||||
const adminPreview = typeof window !== 'undefined'
|
||||
&& new URLSearchParams(window.location.search).get('admin_preview') === '1'
|
||||
? '&admin_preview=1'
|
||||
: '';
|
||||
return `/api/gallery/${slug}/preview/${photo.id}?w=${width}${adminPreview}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Tier for a face crop, sized by how much of the frame the face occupies.
|
||||
*
|
||||
* A fixed small tier is wrong for the case that matters most: in a 6000px
|
||||
* group shot a 200px face is only ~21px at the 640 tier, and faceCropStyle
|
||||
* then blows that up ~9x to fill a 64px avatar at DPR 3 — visibly mush, and
|
||||
* indistinguishable from the mis-positioning bug this was meant to fix.
|
||||
*
|
||||
* Working back from the avatar: the frame must be large enough that the
|
||||
* bbox's share of it still covers the avatar's device pixels. A close-up
|
||||
* lands on 640, a face across a hall lands on 1920.
|
||||
*/
|
||||
function faceTierWidth(
|
||||
photo: { width?: number | null; height?: number | null },
|
||||
cover?: { bbox: number[] } | null,
|
||||
): number {
|
||||
const avatarDevicePx = FACE_AVATAR_PX
|
||||
* (typeof window !== 'undefined' ? Math.min(window.devicePixelRatio || 1, 3) : 2);
|
||||
|
||||
const frameLongEdge = Math.max(photo.width || 0, photo.height || 0);
|
||||
const bboxLongEdge = cover?.bbox ? Math.max(cover.bbox[2] || 0, cover.bbox[3] || 0) : 0;
|
||||
if (!frameLongEdge || !bboxLongEdge) return FACE_CROP_WIDTH;
|
||||
|
||||
const faceShareOfFrame = bboxLongEdge / frameLongEdge;
|
||||
return smallestCovering(Math.round(avatarDevicePx / faceShareOfFrame), PREVIEW_WIDTHS);
|
||||
}
|
||||
|
||||
/** Admin equivalent — the admin API has its own preview route. */
|
||||
export function adminFacePreviewUrl(
|
||||
eventId: number | string,
|
||||
photoId: number | string,
|
||||
photo?: { width?: number | null; height?: number | null },
|
||||
cover?: { bbox: number[] } | null,
|
||||
): string {
|
||||
const width = photo ? faceTierWidth(photo, cover) : FACE_CROP_WIDTH;
|
||||
return `/api/admin/photos/${eventId}/preview/${photoId}?w=${width}`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user