feat(faces): let the photographer choose which photo represents a person (#1119)
Phase 1 of #1096. Clustering picks the cover, and its idea of a good one and a human's do not always agree. A cluster whose avatar is turned away or softer than the rest stays that way in the guest-facing people strip too, and nothing in the UI could change it. A picker reachable from each person row, reusing the face list the split dialog already loads — same query, same grid, different action on a click. Making the choice actually stick took four changes --------------------------------------------------------------------------- event_people.cover_face_id has existed since migration 177 and the PATCH already accepted it, so the first version of this was frontend-only. It was also a no-op: - facePeopleService.listPeople SELECTED cover_face_id and then discarded it, recomputing the cover as the best-scoring VISIBLE face on every read. The picker saved, said so, and the avatar reverted immediately. It now prefers the stored pick whenever this audience can see it, and falls back to the score-ordered choice otherwise — so visibility scoping still wins, and a guest is never handed a crop of a photo they cannot open. - recomputeCentroid overwrote cover_face_id unconditionally. It runs on rescan and on photo replacement, so any reprocessing silently undid a deliberate choice. It now keeps the chosen face while it is still a member of the cluster. - The face list is cached per person, and split/merge move faces between people. Until now the only reader closed itself after acting, so nobody saw the stale copy; the picker is a second reader of the same key. - cover_face_id meant two things. assignFaces seeded it with whichever face opened the cluster and recomputeCentroid overwrote it with the highest scoring one, so an automatic guess was indistinguishable from a deliberate choice — and honouring it would have pinned every UNCURATED person to that guess, which is worse than the fallback it replaced (the fallback is computed per audience and skips photos a guest cannot open). Both writers are gone, migration 179 clears the stored guesses, and the column now means one thing. That also removes the need to defend the choice against rescans: nothing overwrites it, and a dangling id self-heals to the derived cover. Clearing existing values is safe rather than destructive: no install has ever been able to SET a cover, so every stored value is an automatic guess by construction. Also fixes a PostgreSQL-only 500 --------------------------------------------------------------------------- GET /admin/events/:id/people/:personId/faces joined `photos` but did not table-qualify its WHERE, and photo_faces and photos BOTH have an event_id: column reference "event_id" is ambiguous Postgres refuses it, so the endpoint 500s and the Split dialog — its only consumer until now — has been broken on every PostgreSQL install since the join was added. SQLite resolves the ambiguity silently, which is why the suite stayed green. Reproduced against a real Postgres before and after. The query is now a named builder the route calls and the test imports, rather than a copy: an earlier version of that test re-declared the query, so the route could regress to the bare form while the assertions kept passing. Merge and recluster preserve the choice as well. Both already carried labels and privacy flags across; the chosen cover is human state of the same kind, so it now rides along — through a merge when the target has none, and through a recluster by following its FACE into whichever cluster ends up holding it, rather than the majority-descendant rule the label uses. The picker and the endpoint disagree past 500 faces, so the picker now says when it is showing a capped list rather than presenting it as exhaustive. Frontend suite 178 passing, backend 23 across the touched suites, build clean, no new type errors. Mutation-checked twice: dropping the cover preference fails the new listPeople test while the visibility-scoping test still passes, and restoring the auto-seed in assignFaces fails it too.
This commit is contained in:
Paul Nothaft
@@ -25,6 +25,40 @@ const faceClient = require('../../services/faceClient');
|
||||
|
||||
const requireFaces = requireFeatureFlag('faces', 'FACES_DISABLED');
|
||||
|
||||
/** Cap on one person's face list. Surfaced so the UI can say when it bites. */
|
||||
const PERSON_FACES_LIMIT = 500;
|
||||
|
||||
/**
|
||||
* Every face of one person, newest-confidence first, with the ORIGINAL photo
|
||||
* dimensions the bbox was measured against.
|
||||
*
|
||||
* Exported so a test can assert the shape of the SQL this actually emits. It
|
||||
* has to stay TABLE-QUALIFIED: `photos` also has an event_id, so the bare form
|
||||
* became ambiguous the moment the join was added — Postgres refuses it
|
||||
* ("column reference event_id is ambiguous") and the endpoint 500s, which took
|
||||
* the Split dialog down with it on every PG install. SQLite resolves the
|
||||
* ambiguity silently, which is why the suite stayed green and it shipped.
|
||||
*/
|
||||
function buildPersonFacesQuery(dbi, eventId, personId) {
|
||||
return dbi('photo_faces')
|
||||
.where({
|
||||
'photo_faces.event_id': eventId,
|
||||
'photo_faces.person_id': personId,
|
||||
})
|
||||
.orderBy('det_score', 'desc')
|
||||
.limit(PERSON_FACES_LIMIT)
|
||||
.join('photos', 'photos.id', 'photo_faces.photo_id')
|
||||
.select(
|
||||
'photo_faces.id', 'photo_faces.photo_id',
|
||||
'photo_faces.bbox_x', 'photo_faces.bbox_y',
|
||||
'photo_faces.bbox_w', 'photo_faces.bbox_h',
|
||||
'photo_faces.det_score', 'photo_faces.blur',
|
||||
// Needed to crop the box — it is in original-image pixels.
|
||||
'photos.width as photo_width', 'photos.height as photo_height'
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
async function loadOwnedEvent(req) {
|
||||
let q = db('events').where('id', req.params.id);
|
||||
if (req.admin.roleName === 'editor') {
|
||||
@@ -274,19 +308,7 @@ module.exports = (router) => {
|
||||
const event = await loadOwnedEvent(req);
|
||||
if (!event) return res.status(404).json({ error: 'Event not found' });
|
||||
|
||||
const faces = await db('photo_faces')
|
||||
.where({ event_id: event.id, person_id: req.params.personId })
|
||||
.orderBy('det_score', 'desc')
|
||||
.limit(500)
|
||||
.join('photos', 'photos.id', 'photo_faces.photo_id')
|
||||
.select(
|
||||
'photo_faces.id', 'photo_faces.photo_id',
|
||||
'photo_faces.bbox_x', 'photo_faces.bbox_y',
|
||||
'photo_faces.bbox_w', 'photo_faces.bbox_h',
|
||||
'photo_faces.det_score', 'photo_faces.blur',
|
||||
// Needed to crop the box — it is in original-image pixels.
|
||||
'photos.width as photo_width', 'photos.height as photo_height'
|
||||
);
|
||||
const faces = await buildPersonFacesQuery(db, event.id, req.params.personId);
|
||||
|
||||
res.json({
|
||||
faces: faces.map((f) => ({
|
||||
@@ -473,3 +495,6 @@ module.exports = (router) => {
|
||||
});
|
||||
|
||||
};
|
||||
|
||||
module.exports.buildPersonFacesQuery = buildPersonFacesQuery;
|
||||
module.exports.PERSON_FACES_LIMIT = PERSON_FACES_LIMIT;
|
||||
|
||||
@@ -208,7 +208,13 @@ async function assignFacesLocked(eventId, faceRows, thresholds, trx) {
|
||||
centroid: packEmbedding(embedding),
|
||||
face_count_total: 1,
|
||||
model_version: face.model_version,
|
||||
cover_face_id: face.id,
|
||||
// cover_face_id is deliberately NOT set (#1096). It means one thing
|
||||
// now — the photographer picked this face — and seeding it with
|
||||
// whichever face happened to start the cluster made that
|
||||
// indistinguishable from a real choice. listPeople derives the
|
||||
// automatic cover at read time instead, which it has to anyway: the
|
||||
// best face for an ADMIN may sit in a photo a guest cannot open.
|
||||
cover_face_id: null,
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
@@ -309,7 +315,8 @@ async function mergePeople(eventId, sourceIds, targetId) {
|
||||
// person they had suppressed — the target keeps its own values where it
|
||||
// has them, and inherits from a source only where it does not.
|
||||
const target = await trx('event_people').where({ id: targetId }).first();
|
||||
const sources = await trx('event_people').whereIn('id', ids).select('label', 'is_hidden', 'is_ignored');
|
||||
const sources = await trx('event_people').whereIn('id', ids)
|
||||
.select('label', 'is_hidden', 'is_ignored', 'cover_face_id');
|
||||
|
||||
const inherited = {};
|
||||
if (target && !target.label) {
|
||||
@@ -319,6 +326,13 @@ async function mergePeople(eventId, sourceIds, targetId) {
|
||||
// Suppression is one-way on merge: if ANY party was hidden or ignored,
|
||||
// the survivor stays that way. Re-exposing someone by merging is the
|
||||
// failure that matters; leaving them hidden is trivially reversible.
|
||||
// A chosen cover is human state like the label, and the merged face is
|
||||
// still in the cluster afterwards — so it survives the merge rather than
|
||||
// reverting to the automatic pick (#1096). The target's own choice wins.
|
||||
if (target && !target.cover_face_id) {
|
||||
const curated = sources.find((p) => p.cover_face_id);
|
||||
if (curated) inherited.cover_face_id = curated.cover_face_id;
|
||||
}
|
||||
if (sources.some((p) => p.is_hidden) || target?.is_hidden) inherited.is_hidden = true;
|
||||
if (sources.some((p) => p.is_ignored) || target?.is_ignored) inherited.is_ignored = true;
|
||||
|
||||
@@ -395,15 +409,13 @@ async function recomputeCentroid(personId, trx = db) {
|
||||
}
|
||||
for (let i = 0; i < mean.length; i++) mean[i] /= vectors.length;
|
||||
|
||||
// Cover face: the highest-scoring detection in the cluster, so the avatar
|
||||
// is the sharpest available crop of that person rather than whichever face
|
||||
// happened to arrive first.
|
||||
const cover = faces.reduce((a, b) => ((b.det_score ?? 0) > (a.det_score ?? 0) ? b : a));
|
||||
|
||||
// cover_face_id is left alone (#1096): it holds the photographer's pick and
|
||||
// nothing else, so a rescan or a merge must not touch it. If the chosen face
|
||||
// is gone the id simply dangles, and listPeople falls back to the derived
|
||||
// cover on the next read — self-healing, no reassociation needed.
|
||||
await trx('event_people').where({ id: personId }).update({
|
||||
centroid: packEmbedding(normalize(mean)),
|
||||
face_count_total: faces.length,
|
||||
cover_face_id: cover.id,
|
||||
updated_at: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
@@ -427,9 +439,15 @@ async function recluster(eventId) {
|
||||
const previousLabels = await db('event_people')
|
||||
.where({ event_id: eventId })
|
||||
.where(function () {
|
||||
this.whereNotNull('label').orWhere('is_hidden', true).orWhere('is_ignored', true);
|
||||
this.whereNotNull('label')
|
||||
.orWhere('is_hidden', true)
|
||||
.orWhere('is_ignored', true)
|
||||
// A chosen cover is human state too, and re-grouping used to drop it
|
||||
// (#1096). Faces keep their ids across a recluster, so the choice can
|
||||
// follow its FACE into whichever new cluster ends up holding it.
|
||||
.orWhereNotNull('cover_face_id');
|
||||
})
|
||||
.select('id', 'label', 'is_hidden', 'is_ignored');
|
||||
.select('id', 'label', 'is_hidden', 'is_ignored', 'cover_face_id');
|
||||
|
||||
const priorMembership = new Map();
|
||||
if (previousLabels.length) {
|
||||
@@ -501,8 +519,21 @@ async function recluster(eventId) {
|
||||
if (old?.label && !labelFor.has(newId)) labelFor.set(newId, old.label);
|
||||
}
|
||||
|
||||
for (const [newId, flags] of suppression) {
|
||||
// The chosen COVER follows its face, not the majority: the point of the
|
||||
// choice is that specific photo, so it belongs to whichever cluster now
|
||||
// holds it — which is not always the one that inherited the most faces.
|
||||
const coverFor = new Map(); // newId -> faceId
|
||||
const newPersonOfFace = new Map(assignments.map((a) => [a.faceId, a.personId]));
|
||||
for (const old of previousLabels) {
|
||||
if (!old.cover_face_id) continue;
|
||||
const newId = newPersonOfFace.get(old.cover_face_id);
|
||||
if (newId != null && !coverFor.has(newId)) coverFor.set(newId, old.cover_face_id);
|
||||
}
|
||||
|
||||
for (const newId of new Set([...suppression.keys(), ...coverFor.keys()])) {
|
||||
const flags = suppression.get(newId) || {};
|
||||
const update = { ...flags, updated_at: new Date().toISOString() };
|
||||
if (coverFor.has(newId)) update.cover_face_id = coverFor.get(newId);
|
||||
if (labelFor.has(newId)) update.label = labelFor.get(newId);
|
||||
await db('event_people').where({ id: newId }).update(update);
|
||||
}
|
||||
|
||||
@@ -100,11 +100,28 @@ async function listPeople(eventId, { isClient = false, forAdmin = false, minClus
|
||||
'photos.height as photo_height'
|
||||
);
|
||||
|
||||
// The photographer's explicit pick wins whenever this audience can see it
|
||||
// (#1096). Without this the stored cover_face_id was loaded and discarded —
|
||||
// the picker saved, said so, and the avatar reverted to the best-scoring
|
||||
// face on the very next read, making the whole feature a no-op.
|
||||
//
|
||||
// The score-ordered fallback still stands for everything it always covered:
|
||||
// a person who has never been curated, and a chosen cover that sits in a
|
||||
// photo this audience is not allowed to see.
|
||||
const chosenByPerson = new Map(
|
||||
people.filter((p) => p.cover_face_id).map((p) => [p.id, p.cover_face_id])
|
||||
);
|
||||
const visibleById = new Map(covers.map((row) => [row.id, row]));
|
||||
|
||||
const coverByPerson = new Map();
|
||||
for (const row of covers) {
|
||||
// Rows arrive best-score-first, so the first hit per person wins.
|
||||
if (!coverByPerson.has(row.person_id)) coverByPerson.set(row.person_id, row);
|
||||
}
|
||||
for (const [personId, faceId] of chosenByPerson) {
|
||||
const chosen = visibleById.get(faceId);
|
||||
if (chosen && chosen.person_id === personId) coverByPerson.set(personId, chosen);
|
||||
}
|
||||
|
||||
const out = [];
|
||||
for (const person of people) {
|
||||
|
||||
Reference in New Issue
Block a user