From 43d2c976c31fabbef16ad8c2635ccc6c6fff6cd4 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 30 Jul 2026 17:36:09 +0200 Subject: [PATCH] docs: replace phased plan with a per-requirement one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The phase structure encoded ordering assumptions that stopped being true as the design changed, and its Phase 2 still described retuning constants that are now withdrawn. Ordering is now derived from per-requirement dependencies instead: anything with no unmet dependency is startable. Carries over the TrackRegistry design (now keyed to AR-012/AR-013) and records what was withdrawn from the old plan, including the --presence-mode flag — comparison against old behaviour uses recorded reference output rather than a second live code path. Co-Authored-By: Claude Opus 5 --- docs/IMPLEMENTATION-PLAN.md | 627 ------------------------------------ docs/SPEC.md | 2 +- docs/plan.md | 339 +++++++++++++++++++ 3 files changed, 340 insertions(+), 628 deletions(-) delete mode 100644 docs/IMPLEMENTATION-PLAN.md create mode 100644 docs/plan.md diff --git a/docs/IMPLEMENTATION-PLAN.md b/docs/IMPLEMENTATION-PLAN.md deleted file mode 100644 index 1c01580..0000000 --- a/docs/IMPLEMENTATION-PLAN.md +++ /dev/null @@ -1,627 +0,0 @@ -# Implementation plan - -Companion to [`SPEC.md`](SPEC.md). Sequences the gaps identified there into -phases, with dependencies and acceptance criteria. - -Ordering principle: **land the things that make measurement trustworthy before -the things that require measurement.** The central change (A6) invalidates every -tuned constant in `config.hpp`, so anything that could corrupt a retune must be -fixed first, and the retune itself gates the release. - -| Phase | Contents | Gated on | Parallelisable | -|---|---|---|---| -| 0 | D3 model binding, E1 dump audit | — | with 4 | -| 1 | TrackRegistry + A6 track-extent presence | 0 | — | -| 2 | Retune (A6 constants), E4 | 1 | — | -| 3 | A8 unidentified capture, D2 gallery report | 0 | with 1, 2 | -| 4 | C2 audio signature | — | with everything | - -Phases 3 and 4 touch disjoint code from 1 and 2 and can proceed alongside them. - ---- - -## Phase 0 — De-risk measurement - -Small, self-contained, and prerequisite to trusting any number produced later. - -### 0.1 Gallery/model binding (D3) - -Stamp the embedder identity into the gallery at build time; verify at load. - -- `gallery_builder` writes the embedder model identity (filename + hash of the - ONNX, or an explicit version string) into the gallery file. -- Every consumer — `scene_analyze`, `replay.py`, the optimizer — checks it at - startup against the embedder it is about to use. -- Mismatch is a **hard error naming both sides**, never a warning. - -*Why first:* cross-model cosine similarities are meaningless but look entirely -plausible. A retune against a mismatched gallery would produce numbers that are -wrong and undetectably so. This is the cheapest insurance in the document. - -**Acceptance:** a deliberately mismatched gallery/model pair fails at startup -with a message naming both; a matched pair is unaffected. Covered by a test in -`tests/test_gallery_store.cpp`. - -### 0.2 Dump audit for track-aware replay (E1) - -The dump captures state at `EmbeddedSceneFrame` — *upstream* of tracking. After -A6, presence depends on tracker output, so replay must be able to reconstruct -tracks exactly as the live pipeline would. - -- Confirm `bbox`, `landmarks`, `confidence`, `is_cut`, `is_scene_boundary` and - frame timestamps are sufficient to re-run `FaceTrackerFunc` deterministically. -- Specifically verify the **cross-cut park/revive path** (A4) is reproducible: it - depends on `is_cut` and on last-frame embeddings, both of which should be - present — confirm rather than assume. -- If anything is missing, add it and bump `schema_version` in - [`scripts/optimizer/SCHEMA.md`](../scripts/optimizer/SCHEMA.md). - -**Acceptance:** for one film, tracks reconstructed from the dump are identical -(same `track_id` partitioning of the same faces) to those from a live run. -This equivalence test is the foundation of Phase 2 and should be kept as a -regression test. - ---- - -## Phase 1 — TrackRegistry and track-extent presence (A6) - -The central change. Presence moves from "frames where the actor was recognised" -to "extent of tracks the actor owns". - -### 1.1 Design: TrackRegistry as a shared resource - -**Decision:** track ownership lives in a `TrackRegistry` object that is *external -to the dataflow network*, not in a node and not inside `TrackGallery`. - -This is idiomatic for this codebase rather than a workaround: - -- Node functors are already constructed outside the network and passed in by - reference — `main.cpp:186-207` builds `ftracker_fn`, `tracker_fn`, `sink_fn` as - stack objects and `ObjectNode` wraps them ("the object must outlive the node", - KPN SPEC §371). -- KPN provides `SharedResource` (`external/KPN/shared_resource.hpp`) precisely - for state shared across nodes, and shared resources can be registered with a - network for reporting (KPN SPEC §163, §445). - -Why not the two alternatives: - -- **Not a node.** Ownership is not a stage in the stream — it is state that - several stages read and write, at different points, with the final answer only - known at EOF. Modelling it as a node would force ownership to be decided at a - single point in the flow, which is exactly what it cannot be. -- **Not inside `TrackGallery`.** Ownership is already computed there for - expansion, so putting presence there too would avoid duplication — but it - couples presence semantics to `expand_gallery`, a switchable feature. Turning - expansion off would silently revert A6. - -**Ownership and lifetime:** the registry is created in `main` and handed to every -node that needs it as a `std::shared_ptr`. Nodes hold their own -`shared_ptr`, so lifetime is guaranteed by refcount rather than by the -"object must outlive the node" convention — no ordering assumption between -network teardown and the registry's destruction. - -**Per-track state:** - -``` -Track - first_seen : double set once, at track creation - last_seen : optional UNSET while on screen; set to the last - on-screen timestamp when the face is lost - actor : optional set when a posterior crosses the threshold - belief : {actor_idx -> accumulated_logodds} Bayesian, not a tally (A9) - embedding : Embedding running directional mean, for association -``` - -`last_seen` carries the entire liveness state. Unset means *on screen now*; set -means *went off screen at T*. There is no separate "missing frames" counter and -no expired flag in the registry — the optional is the state machine. - -**Lifecycle:** - -``` -face detected, no match to an existing track - → new track, first_seen = t, last_seen = unset - → begin embedding - -actor identified - → record vote; set actor once the ownership rule fires - -face lost - → last_seen = t_last_on_screen (track stays alive, revivable) - -face seen again, embedding matches a track with last_seen set - → last_seen = unset (same track continues) - -tick(t) where last_seen is set and t - last_seen > timeout - → track is DEAD: emit it to the result aggregator, delete the entry -``` - -**A dead track is passed to the result aggregator.** Reaping is a handoff, not a -deletion into a holding pen: the reaped track — `first_seen`, `last_seen`, owning -actor, vote tally — is pushed downstream the moment it dies, and the registry -drops it. The registry therefore contains **only live tracks**, and its size is -bounded by concurrent on-screen faces rather than growing with the film. - -That gives a clean division: the registry answers "who is on screen now and which -tracks are still revivable"; the aggregator accumulates finished presence. Neither -needs the other's state. - -A face appearing after the timeout starts a genuinely new track with a new -`track_id` — correct, since past the re-acquisition window there are no grounds -to assert continuity. An actor who leaves for half an hour and returns gets two -windows rather than one spanning their absence. - -The registry thus has exactly three states, all implied by `last_seen`: on screen -(unset), revivable (set, within timeout), and gone (emitted and removed). There is -no "expired but retained" state to reason about. - -**Closing a track *is* the presence assertion.** This is the property the design -turns on. There is no later stage where presence gets decided, reconciled or -merged — the act of closing a track emits exactly one complete, immutable claim: -*this actor was on screen from a to b*. It is produced once, never revised, and -carries everything needed to justify it (the vote tally that named the actor). - -Several things follow, which is why it is worth naming explicitly: - -- **Presence is append-only.** The output is a stream of finished claims, so a - partial result is a *prefix* of the full result, not a corrupted version of it. -- **Each claim is independently checkable.** A wrong window can be traced to one - track and its votes, rather than to an emergent interaction between three - timeout constants. -- **Nothing downstream needs to be clever.** The aggregator groups claims by - actor and writes them out. It holds no state machine of its own, which is - precisely why `SceneTrackerFunc` disappears. - -Compare the current design, where presence is inferred at the end from a pile of -per-frame detections via two gap-bridging constants, and no single moment is "the -decision". That indirection is the source of both the late-start bug and the -untunability. - -An actor's presence window is simply `[first_seen, last_seen]` of each track they -own. Nothing else. - -**This removes annealing entirely.** `anneal_sec` (35.5 s) and `extinction_sec` -(57.4 s) exist only because presence is currently assembled from *isolated -accepted frames*, which are full of holes — both constants are gap-bridging -patches over that. Under this model a track survives its own gaps by -construction: a face lost and re-acquired by embedding match is the **same -track**, so there is no second window to merge and nothing to anneal. Presence -continuity is inherited from track continuity rather than reconstructed from it. - -This is also what makes camera cuts fall out for free. A shot/reverse-shot sets -`last_seen` on the cut and unsets it on the next matching detection; the extent -never breaks. The existing cross-cut park/revive machinery -(`face_tracker_node.hpp:154-172`, `cut_revive_sim`) is exactly this mechanism -already, applied only to cuts — the model generalises it to every disappearance, -which is why cuts stop needing a special case. - -**One timeout, not three.** The only surviving tunable is how long a track stays -revivable after the face is lost. It does two jobs at once, and both are wanted: - -- **Gap absorption.** A face lost at `t₁` and re-acquired at `t₂` within the - timeout never closed its track, so the window runs straight through — the actor - **is claimed present across `[t₁, t₂]`**. That is correct: someone who turns - away, is briefly occluded, or is off-camera while the shot cuts to whoever they - are talking to has not left the scene. -- **Identity continuity.** The re-acquisition is only accepted on an embedding - match, so the gap is bridged on evidence that it is the *same person*. - -That second point is the substantive improvement over `anneal_sec`, which merged -windows purely on elapsed time and could therefore stitch together two different -people. Same smoothing, now evidence-gated. - -**The asymmetry that keeps this honest:** interior gaps are claimed, the trailing -cool-down is not. A track that dies ends its window at `last_seen` — the last -frame the face was actually seen — not at the moment of death. So the timeout -buys gap-smoothing without over-claiming the tail. - -**The documented credits overshoot is fixed by this asymmetry, not by scene -detection.** `extinction_sec` kept an actor *active* for 57 s after their last -detection, actively emitting presence into the closing credits (the Downton Abbey -recall collapse, `lvface-deep-dive.md`). A window ending at `last_seen` never -enters the credits at all, because nothing was seen there. - -**Cuts and scene boundaries are association hints, not presence events.** Both -signals say the same thing to the tracker: *spatial continuity is broken — stop -trusting IoU, associate on embedding similarity instead.* Neither closes a window. - -| Signal | Meaning | Effect | -|---|---|---| -| `is_cut` (histogram) | Camera-angle change within a scene | Weight association toward embedding | -| `is_scene_boundary` (TransNetV2) | Different scene | Same, more strongly | - -This is what the tracker's park/revive path already does for cuts -(`face_tracker_node.hpp:154-172`): a post-cut detection is matched on raw cosine -similarity to a parked track's last-frame embedding, with IoU out of the picture. -Generalising it makes `track_alpha` (the spatial/embedding cost weight) -frame-dependent rather than constant — normal frames use the tuned blend, flagged -frames drop toward embedding-only. - -An actor genuinely continuing across a boundary is therefore *kept*, which is -correct; one who does not reappear simply times out and closes at `last_seen`. - -This makes `--scene-detect` **load-bearing for presence correctness**, not the -opt-in extra it is today (`config.hpp`, default off). Two consequences to decide -in Phase 2: whether it becomes default-on despite its dense-decode cost, and what -the degraded behaviour is when it is off — presumably the timeout alone, which is -the current situation and carries the known overshoot. - -It follows that the timeout **is** a presence knob and **cannot** simply be made -generous: it is precisely "how long an absence do we tolerate before calling it a -departure". Too short fragments one continuous appearance into several windows; -too long absorbs a genuine exit-and-return into a single window claiming presence -the actor did not have. It replaces both `anneal_sec` and `extinction_sec` and -inherits their tuning burden — this is the constant Phase 2 must fit. - -`track_max_frames_missing` and `cut_inactive_max_frames` already serve this role -in the tracker and should be reconciled into it. Note the units differ: the -tracker counts *frames* missing while windows are in *seconds*; whichever side -owns the timeout should own the conversion, so `sample_fps` changes cannot -desynchronise them. - -**Ownership rule** (per SPEC A9): each track carries accumulated **log-odds per -candidate actor**, updated as frames arrive. Ownership is "posterior exceeds -threshold", not "≥ N accepted frames" — so `votes` in the state sketch above is a -`{actor_idx → accumulated_logodds}` map, and `on_vote()` is an *update*, not an -increment. - -Consequences for the registry: - -- Every similarity reaching it is already a probability (A9); the registry never - sees a raw cosine. -- Correlated-frame discounting (A9) applies at update time. The registry should - take the already-discounted evidence rather than deciding the discount itself — - that judgement belongs with the diversity buffer that identifies novel poses. -- Ownership is established at **first crossing**, not deferred to track death. - -**Two contradiction rules the registry must enforce** (SPEC A6). Both exist -because the same underlying fault — a missed camera or scene change — shows up in -identity space, and both are detectable *online*: - -| Condition | Meaning | Action | -|---|---|---| -| Belief on one track swaps A → B | `track_id` carried across a viewpoint change onto a different person | Close the track at `last_seen`, open a new one for B at the swap frame | -| Two **live** tracks owned by the same actor | One person split into two tracks, or an identity attached to the wrong one | Treat as a detected cut: reset the affected tracking state and re-associate on embedding | - -The second is the more interesting: it makes identity a **third cut detector**, -independent of the histogram and TransNetV2, firing exactly where those failed. A -cut subtle enough to evade pixel-based detection is not necessarily subtle in -identity space. - -Both must be **counted and reported** (SPEC D2) — the rates are a direct measure -of how often tracking is silently wrong, which nothing currently reveals. - -Implementation note: "two live tracks owned by the same actor" is a cheap check -because the registry already holds every live track and its belief. Maintain a -reverse index `actor_idx → live track_ids` and the condition is detected on the -update that causes it, not by scanning. - -**The registry *is* the tracker's state.** `FaceTrackerFunc` does not keep its own -`tracks_`/`inactive_` maps and mirror them into a registry — it is constructed -with `std::shared_ptr` and operates on it directly. One copy of the -track set, one owner of liveness. - -This matters beyond tidiness. Had the tracker kept private maps and *reported* -into a parallel registry, the two could disagree — the tracker expiring a track -the registry still thinks alive, or vice versa — and every such divergence would -surface as wrong presence windows, silently. Merging them makes that class of bug -unrepresentable rather than merely tested-against. - -It also subsumes the tracker's existing two-pool split: `tracks_` becomes the -tracks with `last_seen` unset, `inactive_` becomes those with it set. Same -structure, one map, and the cross-cut park/revive path becomes the general -re-acquisition path rather than a special case. - -**Interface:** - -``` -TrackRegistry - // tracker-facing: state it owns and mutates - tick(timestamp) ← FaceTrackerFunc, every frame - candidates() -> span → all live tracks; last_seen tells - the caller whether IoU applies - create(timestamp, embedding) -> track_id - mark_seen(track_id, timestamp, embedding) → updates mean, clears last_seen - mark_lost(track_id, last_on_screen_timestamp) - - // matcher-facing - on_vote(track_id, actor_idx, posterior) ← IdentityMatcherFunc (accepted frames only) - - // reader-facing - owner(track_id) -> optional → TrackGallery (expansion confirmation) - - // output: dead tracks are pushed out as they are reaped - on_track_dead : callback(DeadTrack) → ResultSinkFunc - flush() ← at EOF: emit all live tracks, then clear -``` - -`DeadTrack` carries `first_seen`, `last_seen`, the owning `actor_idx` (or none), -and the vote tally — everything the aggregator needs, with no back-reference into -registry state. - -**Every track must be closed at EOF.** `flush()` emits every still-live track -through the same callback, closing each at `last_seen` if set and at the final -tick timestamp otherwise. This is not a tidy-up detail: a film almost always ends -with faces on screen, and those tracks have not timed out, so without an explicit -flush they are simply never emitted — the closing scene's actors disappear from -the output. That failure is silent and looks like a recognition miss rather than -a bookkeeping bug. - -Requirements: - -- `flush()` is idempotent and leaves the registry empty; calling it twice emits - nothing the second time. The sink's existing `written_.exchange(true)` guard - (`result_sink_node.hpp:66`) shows the shape. -- It must run on **every** termination path that produces output, not just clean - EOF — the `eof` sentinel, and the early-exit paths (`--end-sec`, decode error, - user interrupt) if those still write results. -- Finalisation goes through the same code path as a natural death, so a track - closed by EOF is indistinguishable in form from one closed by timeout. -- Deliberately *not* covered: SIGTERM during opportunistic runs (SPEC B4). Those - push no partial result at all, so there is nothing to flush — the item stays - pending and restarts. Flush is for runs that produce output. - -**Acceptance:** a clip ending mid-shot yields a window for the on-screen actor -whose `end` equals the final frame timestamp. This is a specific test, not an -incidental one. - -The tracker's association step reads `candidates()` — **one pool, not two**. A -track with `last_seen` unset was seen last frame, so IoU is meaningful; one with -`last_seen` set is dormant and matched on embedding alone. There is no separate -revival path: matching a dormant track is ordinary inter-frame association, and -the property falls out of the embedding comparison rather than being a mechanism. -The step then calls `create`/`mark_seen`/`mark_lost`. Reaping happens in `tick()`. - -Since the tracker mutates registry state across a frame's association pass, that -pass needs to be atomic as a unit — a `frame_scope()` handle holding the lock for -the duration is cleaner than making each accessor independently locked and hoping -the composite is safe. This is the one place where per-call atomicity is *not* -sufficient. - -`tick()` advances the clock so dead tracks are reaped independently of detection -activity — without it the registry only learns about time when something is -detected, and tracks would only die when some *other* face happened to appear. It -is called once per sampled frame whether or not that frame had detections. - -**Locking.** Nodes run concurrently and do not coordinate, so the registry is -responsible for its own consistency. Two granularities apply: the tracker's -per-frame association pass holds the lock for its whole duration (`frame_scope()` -above), while every other caller's operations must be individually atomic. - -Two cases constrain the API shape: - -- `owner()` is a **read-modify-read** in disguise: `TrackGallery` calls it to - decide promotion while `IdentityMatcher` may be concurrently voting on the same - track. Tally and verdict must be read under one lock as a snapshot — not "read - tally, release, decide" — or a track can be both unowned and owned within a - single promotion decision. -- `on_vote()` for a frame arrives from `IdentityMatcher`, downstream of the - tracker's `tick()`/`mark_seen()` for that same frame. A vote may therefore land - after the clock has moved on. Rule: a vote for a known track always lands on - that track's tally, regardless of clock position. Only reaping is clock-driven. - A vote for a track already reaped is dropped and **counted** — a nonzero count - means the timeout is shorter than the matcher's lag, which is a real - misconfiguration and should not fail silently. -- The `on_track_dead` callback fires from inside `tick()`, which the tracker calls - while holding the frame lock. The callback must therefore not re-enter the - registry, or it self-deadlocks. Keep it to a push onto the aggregator's own - storage; anything heavier belongs downstream of that. - -A single `std::mutex` over the whole registry is the right starting point: -contention is a handful of small updates per frame against per-frame work -measured in GPU milliseconds. Anything finer needs a profile, not an assumption. - -### 1.2 Wiring - -- `FaceTrackerFunc` is constructed with the `shared_ptr` and uses - it as its state — its own `tracks_`/`inactive_` maps go away. Per frame: - `tick()`, then association over `candidates()`, then - `create`/`mark_seen`/`mark_lost`. Its `tracks_`/`inactive_` split and the - cross-cut revival branch both collapse into one pool keyed on `last_seen`. -- `IdentityMatcherFunc` reports accepted-frame votes. It keeps its existing - per-frame acceptance logic unchanged — A6 changes what is *done* with - acceptances, not how they are decided. -- `TrackGallery` replaces its internal confirmation counter with - `registry.owner()`, so ownership is computed **once**. -- `ResultSinkFunc` becomes the result aggregator: it receives dead tracks via the - callback and groups them by actor. The per-frame timestamp collection and - gap-merging at `result_sink_node.hpp:123-147` is deleted. **No annealing pass** - — a dead track already *is* a window. -- `SceneTrackerFunc` (`scene_tracker_node.hpp`) is the extinction-timer state - machine keyed on `actor_idx`. Under this model it has nothing left to do: its - entire job was keeping actors alive across detection gaps. Expect to delete it - from the network rather than adapt it. - -### 1.3 Comparing against current behaviour - -The old path is not worth preserving behind a flag. It is not a variant of the -new one — it is a different pipeline shape (`SceneTrackerFunc` present, annealing -in the sink, two extra constants), so keeping both runnable means maintaining two -sink implementations and a node that otherwise gets deleted. - -Compare against **recorded output** instead: keep the current binary's results for -the validation corpus as reference JSON, and diff the new pipeline against them. -That gives the same A/B for Phase 2 without carrying dead code through it. - -**Acceptance:** - -- `tests/test_face_tracker.cpp` extended: a lost-then-re-acquired face continues - the **same** track and yields one unbroken window; a cut does the same; a face - re-appearing *after* the timeout yields two separate tracks and two windows; - a two-actor conflict resolves to the majority and increments the conflict - counter; a single-frame track yields a zero-length window. -- A registry test: reaping emits exactly once per track; `flush()` at EOF emits - every live track and nothing twice; a vote landing on a reaped track is dropped - and counted. -- Contradiction tests: a belief swap A→B closes one window at `last_seen` and - opens a second starting at the swap frame, with no overlap and no blended - window; two live tracks converging on one actor trigger a re-association and - increment the counter. -- A concurrency test hammering `tick`/`mark_seen`/`mark_lost`/`on_vote` from - multiple threads against `owner()`, under TSan. The vote-tally-plus-verdict read - is the case to target — correct only if atomic as a unit. -- On a known film, every actor's first window starts no later than in the recorded - reference output, and strictly earlier for at least one — the late-start bug - this change exists to fix. - ---- - -## Phase 2 — Delete two constants, tune the rest - -`anneal_sec` (35.5) and `extinction_sec` (57.4) are **replaced, not discredited**. -They exist to answer X-Ray's scene-level question — "is this actor in this scene" -— by holding windows open across cuts, and they answer it with elapsed time -because that was the only signal available at the sink. Phase 1 answers the same -question with better evidence: an embedding-matched re-acquisition, plus a true -scene boundary to stop at. So they are deleted along with `SceneTrackerFunc`, and -their *job* transfers to the re-acquisition timeout rather than disappearing. - -What actually needs tuning: - -- **The ownership posterior threshold** — replaces both `prob_threshold` (0.754) - as a presence decision and `expand_min_anchor_frames` (3) as a vote count. A - track is owned when its accumulated posterior for an actor crosses this (A9). - A single false accept can no longer create a window on its own, so the - operating point should sit lower than the old per-frame threshold. -- **The correlated-frame discount** (A9) — whatever form it takes, it is a fitted - quantity and belongs in the sweep. It directly controls how fast belief - accumulates along a track, so it trades against the ownership threshold and - cannot be tuned separately. -- **The re-acquisition timeout** — reconciled from `track_max_frames_missing` (5) - and `cut_inactive_max_frames` (5). **This is the successor to both `anneal_sec` - and `extinction_sec` and carries their tuning burden.** It decides how long an - absence is absorbed into a presence window versus treated as a departure, so it - trades recall (bridging real gaps) against precision (claiming presence during - a genuine exit) directly. Current values are 5 *frames*, inherited from a - tracker-continuity role; as a presence constant it is likely to want a much - larger value, and should be swept over seconds rather than nudged. - -- **`scene_detect`** — now load-bearing for presence (§1.1), so the sweep must - cover *with* and *without*, and `scene_threshold` (0.60) becomes a presence - constant rather than a diagnostic one. A missed boundary reintroduces the - overshoot; a spurious one truncates a scene's cast early. - -Method per SPEC E3: DE over the validation corpus, objective micro-F1, but -precision and recall logged at every evaluation and printed at the optimum. X-Ray -recall is a face-vs-cast-in-scene ceiling, so unconstrained F1 pushes thresholds -down chasing unreachable recall and trades away real precision. Pick the operating -point deliberately from the trajectory. - -**This resolves SPEC Open Question 1 by construction rather than by measurement.** -The question was whether `extinction_sec` survives alongside track extents; the -answer is that the mechanism it patched no longer exists. - -One thing to watch: the search space is now 3 knobs instead of 3 gap-constants, but -they are *not* independent — a longer re-acquisition timeout means longer tracks, -which means more frames to clear `expand_min_anchor_frames`. Sweep jointly. - -**Acceptance:** `anneal_sec` and `extinction_sec` removed from `config.hpp` and -`Config`; new constants committed with the trajectory and the precision/recall -trade-off documented in the manner of `rep4-optimizer-results.md`; results -compared against the recorded reference output from 1.3. - ---- - -## Phase 3 — Diagnostics - -Independent of Phases 1–2; can run in parallel. - -### 3.1 Unidentified-track capture (A8) - -Today only *promoted* mugshots are dumped (`expand_debug_dir`) — the successes. -This captures the failures. - -- Flag `--dump-unidentified `. -- For every track never identified: all embeddings, track metadata (`track_id`, - first/last timestamp, frame count, per-frame bbox and confidence), and **the - best similarity achieved and which actor it was against**. -- That last field is the point of the feature: it separates "actor missing from - the gallery" from "actor present but scored below threshold" — a gallery - coverage problem versus a threshold problem. -- Crops are **opt-in** (`--dump-unidentified-crops`); embeddings + metadata are - the default. Crops for every unidentified track across a library is a lot of - disk. -- When crops are enabled, store both the 112×112 aligned crop *and* a wider - **context crop** for a bounded number of representative frames per track. The - aligned crop serves diagnostics; the context crop serves the human-in-the-loop - association capability ([`../../SPEC.md`](../../SPEC.md) §4), where someone has - to actually recognise the person — which a tightly-cropped, geometrically - normalised face often makes impossible. -- Naturally expressed against `TrackRegistry`: unidentified = tracks with no - owner at EOF. - -**Acceptance:** on a film with a known out-of-gallery face, that track appears in -the dump with its near-miss actor and similarity. - -### 3.2 Gallery build report (D2) - -Surface what the calibration already computes internally but discards: - -- actors with zero usable images (a silent recall ceiling); -- actors below the 5-embedding threshold for positive pairs; -- near-duplicate references removed; -- the fitted calibration **and the intra/inter similarity distributions behind - it** (`gallery_calibration.hpp` histograms these at `kHistBins = 200` and - throws them away — persist them). - -**Acceptance:** a build report written alongside the gallery; the intra/inter -PDFs are recoverable for inspection. - -### 3.3 Resolve the prior (SPEC Open Question 4) - -SPEC A9 asks for a prior of `intra/(intra+inter)`; the shipped default is -`match_prior = 0.5` (use the calibrated sigmoid directly). These disagree. With -3.2 landed the real value is known, so: either adopt it, or document 0.5 as a -deliberate override with the reason. Cheap once the distributions are persisted. - ---- - -## Phase 4 — Audio signature (C2) - -Largest self-contained chunk; no dependency on any other phase. - -- Implement [`JRay-public-server/SPEC.md` §3](../../JRay-public-server/SPEC.md) - **exactly**: 120 s centred on the midpoint, mono 11025 Hz, 4096/1024 Hann STFT, - 300–3000 Hz, 32 log bins, peak bin + 2-bit energy class, one byte per frame, - base64. -- Audio decode is a second stream from the FFmpeg dependency already linked for - video (`ffmpeg_decoder.hpp`) — not a new dependency. -- Emit in the truth file → **`schema_version` bump**, coordinated with - `jRay/SPEC.md` and the plugin. Neither can be changed unilaterally. -- **Golden-vector cross-check is a hard requirement**, not a nicety: two - independent implementations of the same DSP chain will drift. Fixture: a short - audio file with its expected signature, checked into both repos and asserted in - both test suites (SPEC Open Question 3, resolved this way — a shared fixture - rather than a shared implementation, since the coupling cost of the latter - exceeds the benefit). -- This pipeline **produces only**. Matching and offset recovery stay consumer-side. - -**Acceptance:** pipeline and plugin produce identical signatures for the same -file; the golden-vector test passes in both repos. - ---- - -## Deferred - -Deployment (SPEC Part B) is deliberately excluded from this plan. B3 (on-demand -service) and B4 (opportunistic worker) are packaging concerns over a stable core, -and the core is about to change under A6. `service-conversion.md` remains the -design of record; it should be executed once Phase 2 fixes the constants, so the -installer is not shipping values that are about to be replaced. - -One exception worth pulling forward if convenient: the **temp-file cleanup fix** -in `run_from_jellyfin.py` (SPEC B4) is small, independent, and a named -prerequisite for the worker. - ---- - -## Risks - -| Risk | Mitigation | -|---|---| -| A6 widens presence and precision drops more than recall gains | Phase 2 leaves the operating point explicit; both modes runnable via flag for direct comparison | -| Retune produces a worse optimum than the current constants | Trajectory is logged; the old operating point stays available. A6 is behaviourally correct even if the metric disagrees — decide deliberately, do not let the metric silently veto it | -| Track-ID collisions merge two people into one extent | Conflict counter (1.1) makes the rate visible; `expand_track_spread_max` already guards the expansion side | -| Registry races produce non-deterministic presence between identical runs | Every operation atomic as a unit (1.1); TSan test in Phase 1 acceptance. A race here is especially costly — it would surface as irreproducible optimizer scores in Phase 2, where it would look like metric noise rather than a bug | -| X-Ray metric blindness | `methodology.md` documents the known failure (scene-union hid out-of-cast FPs). Any metric change gets checked for the same class of blindness | -| Two audio implementations drift | Golden vectors in both repos (Phase 4) | diff --git a/docs/SPEC.md b/docs/SPEC.md index 4488f84..65742a2 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -314,7 +314,7 @@ actor was first *recognised*, not when their track began. **Gap — this is the main behavioural change in this spec.** -Design is settled in [`IMPLEMENTATION-PLAN.md`](IMPLEMENTATION-PLAN.md) §1.1: a +Design is settled in [`plan.md`](plan.md) under AR-012/AR-013: a `TrackRegistry`, held by `shared_ptr` and used by `FaceTrackerFunc` as its state, where each track carries `first_seen` plus an **optional `last_seen`** — unset while on screen, set to the last on-screen timestamp when the face is lost, unset diff --git a/docs/plan.md b/docs/plan.md new file mode 100644 index 0000000..de9dadc --- /dev/null +++ b/docs/plan.md @@ -0,0 +1,339 @@ +# Implementation plan — per requirement + +One entry per requirement that needs work. Requirements marked `Done` in +[`requirements.md`](requirements.md) are omitted. + +**Ordering is derived from dependencies, not assigned to phases.** Each entry +lists what it depends on; anything with no unmet dependency is startable. This +replaces the earlier phase-based plan, which encoded ordering assumptions that +stopped being true as the design changed. + +Verification for each requirement is specified in +[`requirements.md`](requirements.md) — this document covers *how to build it*, +not how to prove it. + +--- + +## Startable now (no unmet dependencies) + +`GR-004` · `IR-004` · `IR-005` · `IR-007` · `IR-008` · `VR-005` · `AR-011` · +`AR-023` extension · tooling port + +These touch disjoint files and can proceed concurrently. + +## Blocked on the registry + +Everything in `AR-007` … `AR-022` depends on `AR-012`/`AR-013` landing first, +because they all read or write track state. **This group is one coherent +refactor, not parallel work** — splitting it across concurrent efforts produces +incompatible designs in the same files. + +--- + +# Algorithm + +## AR-012, AR-013 — TrackRegistry (the spine) + +**Depends on:** nothing. **Blocks:** AR-007, AR-008, AR-014 … AR-022. + +Everything else in Part A waits on this, so it goes first. + +### Ownership: a shared resource, not a node + +The registry is **external to the dataflow network**, created in `main` and +handed to each node that needs it as `std::shared_ptr`. Lifetime +is guaranteed by refcount rather than by the "object must outlive the node" +convention, so no ordering assumption exists between network teardown and +registry destruction. + +This is idiomatic here: node functors are already constructed outside the network +and passed by reference (`main.cpp:186-207`), and KPN provides `SharedResource` +for state shared across nodes (KPN SPEC §163, §445). + +Not a node, because ownership is not a stage in the stream — it is state several +stages read and write, whose final answer is only known when a track dies. +Not inside `TrackGallery`, because that would couple presence to `expand_gallery`, +a switchable feature. + +**The registry *is* the tracker's state.** `FaceTrackerFunc` does not keep its own +`tracks_`/`inactive_` maps and mirror them in — it operates on the registry +directly. Two parallel copies could disagree, and every divergence would surface +as wrong presence windows, silently. + +### Per-track state + +``` +Track + first_seen : double set once, at creation + last_seen : optional UNSET while on screen; set to the last + on-screen timestamp when the face is lost + actor : optional set when a posterior crosses the threshold + belief : {actor_idx -> accumulated_logodds} Bayesian, not a tally + embedding : Embedding running directional mean, for association +``` + +`last_seen` carries the entire liveness state. Unset = on screen; set = went off +at T. No separate missing-frames counter, no expired flag — the optional *is* the +state machine, and it subsumes the current two-pool split (`tracks_` = unset, +`inactive_` = set). + +### Lifecycle + +``` +face detected, no match → new track, first_seen = t, last_seen = unset +actor identified → update belief; set actor when threshold crossed +face lost → last_seen = t_last_on_screen (stays revivable) +face seen again, embedding match → last_seen = unset (same track continues) +tick(t), t - last_seen > timeout → emit to aggregator, DELETE the entry +``` + +A presence window is `[first_seen, last_seen]`. Nothing else. + +**Interior gaps are claimed; the trailing cool-down is not.** A face lost at t₁ +and re-acquired at t₂ within the timeout never closed its track, so the actor is +present across `[t₁, t₂]` — correct, since someone briefly occluded or off-camera +has not left the scene. But a track that dies ends at `last_seen`, not at the +moment of death. That asymmetry is what removes the old `extinction_sec` +over-claim. + +**Reaping is a handoff, not a deletion into a holding pen.** The dead track goes +to the result aggregator immediately and the registry drops it, so the registry +holds only live tracks and its size is bounded by concurrent on-screen faces. + +### Interface + +``` +TrackRegistry + tick(timestamp) ← FaceTrackerFunc, every frame + candidates() -> span → all live tracks + create(timestamp, embedding) -> track_id + mark_seen(track_id, timestamp, embedding) → updates mean, clears last_seen + mark_lost(track_id, last_on_screen_timestamp) + on_vote(track_id, actor_idx, posterior) ← IdentityMatcherFunc + owner(track_id) -> optional → TrackGallery + on_track_dead : callback(DeadTrack) → ResultSinkFunc + flush() ← at EOF +``` + +`candidates()` returns **one pool**; `last_seen` tells the caller whether IoU +applies. There is no separate revival path — matching a dormant track is ordinary +inter-frame association. + +`tick()` advances the clock so dead tracks are reaped independently of detection +activity; without it a track only dies when some *other* face happens to appear. + +### Locking + +The tracker mutates registry state across a frame's association pass, so that +pass holds the lock for its duration (a `frame_scope()` handle). Every other +caller's operations must be individually atomic. A single `std::mutex` over the +whole registry is the right start — contention is a few small updates per frame +against per-frame work measured in GPU milliseconds. + +Two cases constrain the API: + +- `owner()` is a **read-modify-read** in disguise: `TrackGallery` calls it while + `IdentityMatcher` may be voting on the same track. Tally and verdict must be + read under one lock as a snapshot, or a track can be both unowned and owned + within a single promotion decision. +- `on_vote()` arrives downstream of the tracker's `tick()` for the same frame, so + a vote may land after the clock moved on. **Rule: a vote for a known track + always lands on its tally, regardless of clock.** Only reaping is clock-driven. + A vote for an already-reaped track is dropped and **counted** — a nonzero count + means the timeout is shorter than the matcher's lag. + +`on_track_dead` fires from inside `tick()` while the frame lock is held, so the +callback must not re-enter the registry. Keep it to a push onto the aggregator's +storage. + +## AR-016 — EOF flush + +**Depends on:** AR-012. + +`flush()` emits every still-live track through the same callback, closing at +`last_seen` if set and the final tick timestamp otherwise. Idempotent, leaving the +registry empty; the sink's `written_.exchange(true)` guard +(`result_sink_node.hpp:66`) shows the shape. + +Must run on **every** termination path that produces output. Not SIGTERM during +opportunistic runs (DP-004) — those push no partial result, so there is nothing +to flush. + +Without it a film ending mid-shot silently drops its closing cast, which looks +like a recognition miss rather than a bookkeeping bug. + +## AR-014, AR-015 — Contradiction rules + +**Depends on:** AR-012, AR-025. + +| Condition | Meaning | Action | +|---|---|---| +| Belief on one track swaps A → B | `track_id` carried across a viewpoint change onto a different person | Close at `last_seen`, open a new track for B at the swap frame | +| Two **live** tracks owned by one actor | One person split in two, or an identity attached to the wrong track | Treat as a detected cut: reset affected state, re-associate on embedding | + +The second makes identity a **third cut detector**, independent of histogram and +TransNetV2, firing where those failed. Detect it via a reverse index +`actor_idx → live track_ids`, so the condition is caught on the update that +causes it rather than by scanning. + +Both counted and reported — the rates measure how often tracking is silently +wrong, which nothing currently reveals. + +## AR-007, AR-008 — Tracker on one pool + +**Depends on:** AR-012, AR-024. + +`FaceTrackerFunc` is constructed with the registry and uses it as state; its +`tracks_`/`inactive_` maps and the cross-cut revival branch collapse into one +pool keyed on `last_seen`. Per frame: `tick()`, association over `candidates()`, +then `create`/`mark_seen`/`mark_lost`. + +`track_alpha` becomes **frame-dependent** — normal frames use the tuned blend, +frames flagged `is_cut`/`is_scene_boundary` drop toward embedding-only. + +## AR-024 — Probability space everywhere + +**Depends on:** AR-023. **Blocks:** AR-007, AR-018, AR-021, AR-025. + +Cuts across tracker, matcher and expansion, so it lands with the registry work +rather than after it. Retires `track_max_embed_dist`, `cut_revive_sim`, +`expand_novelty_sim`, `expand_track_spread_max`. + +Enforcement is a **static grep check** for bare cosine outside a tagged +`EXCEPTION` — a unit test cannot prove absence across a codebase. + +## AR-025 — Bayesian accumulation + +**Depends on:** AR-023, AR-024. + +Log-odds per candidate actor, added per frame. `on_vote()` is an *update*, not an +increment. + +**The independence problem must be handled explicitly.** Consecutive frames are +highly correlated; naive accumulation drives the posterior to certainty on what is +effectively one observation. Preferred mitigation: update only on sufficiently +novel observations, reusing the diversity buffer's existing judgement rather than +inventing a second one. The registry should receive already-discounted evidence. + +## AR-017 — Claims carry belief and route + +**Depends on:** AR-012, AR-025. `DeadTrack` carries posterior plus how it was +identified (live / deferred / pooled). + +## AR-018 … AR-021 — Expansion, deferred pass, clustering + +**Depends on:** AR-012, AR-024, AR-026. + +Ordering within the group: AR-018 (banded store) → AR-019 (annex) → AR-020 (TBI +queue + deferred pass) → AR-021 (clustering). + +AR-021 needs the temporal cannot-link constraint from track extents, so it cannot +start before AR-012. The annex must be a **contiguous matrix** with promotions +appended (AR-026), not a list. + +**Output timing changes:** the sink can no longer finalise at EOF — the deferred +pass runs after and may add windows (IR-003). + +## AR-022 — Unidentified capture + +**Depends on:** AR-020. Unidentified = TBI entries surviving the deferred pass. +Context crops opt-in behind `--dump-unidentified-crops`. + +## AR-001 … AR-004 — Detection and backpressure + +**Depends on:** nothing (AR-002, AR-011); AR-004 blocks AR-003. + +- **AR-002** — `min_face_px` → 66, expressed in original resolution. +- **AR-011** — feed TransNetV2 at native rate; derive the dedup window from + source fps rather than the hardcoded `0.04 s`. +- **AR-004** — backpressure. `kMaxFaces` (`identity_matcher_node.hpp:133`) + currently **throws**; channel capacities of 16 (`main.cpp:204-207`) were sized + against ≤10 faces/frame. Must block on bytes in flight, not item counts. +- **AR-003** — remove `max_faces`. **Gated on AR-004**, not a follow-up to it. + +## AR-026, AR-027 — GEMM and scale + +**Depends on:** nothing to start. The annex CPU loop +(`identity_matcher_node.hpp:159-162`) moves into the GEMM path. + +--- + +# Gallery + +## GR-004 — Model binding + +**Depends on:** nothing. **Startable immediately, highest value per line.** + +Stamp embedder identity into the gallery at build; verify at load in +`scene_analyze`, `replay.py` and the optimizer. Mismatch is a hard error naming +both sides. + +Cross-model similarities are meaningless but *look* plausible — this fails +silently and expensively, and it would corrupt every measurement taken during the +rest of this work. + +## GR-003 — Coverage reporting + +**Depends on:** nothing. Surface what calibration already computes and discards +(`kHistBins = 200`): zero-image actors, under-referenced actors, dedup counts, +and the intra/inter PDFs. + +## GR-006 … GR-008 — Provenance tiers + +**Depends on:** AR-019. Tier per embedding (baked / harvested / confirmed); +harvested persisted but flagged; bell-curve outlier check +(`EXCEPTION: AR-024`). + +--- + +# Integration + +## IR-004, IR-005, IR-007, IR-008 — Audio signature + +**Depends on:** nothing. **Fully independent — no existing pipeline file is +touched.** Best candidate for concurrent work. + +Implement server spec §3 exactly. Audio decode is a second stream from the +already-linked FFmpeg. Media < 120 s: no signature, no offset. Emit and honour +the `v1:` prefix. + +The golden-vector fixture is shared with the plugin repo and runs on CPU, so the +one place two implementations must agree bit-for-bit is verifiable in CI. + +## IR-001 … IR-003 — Truth file + +**Depends on:** AR-017 (belief), AR-020 (output timing). + +Windows carry belief and route; `extraction.*` gains `extinction_sec` and +`gallery_scope`; `anneal_sec` removed. All breaking → **one** coordinated +`schema_version` bump with IR-004 (SR-003). + +--- + +# Validation + +## VR-005 — Minimum face size study + +**Depends on:** nothing. Standalone Python, no C++ contact. Produces the measured +value replacing AR-002's 66 px estimate. + +## VR-001 — Dump audit + +**Depends on:** nothing. Read-only investigation: confirm the HDF5 dump preserves +everything needed to reconstruct tracks deterministically, including the +park/revive path. **Prerequisite for the CI strategy**, since T2 replay is how +most of AR-007 … AR-022 is verified. + +## VR-006 … VR-009 + +**Depends on:** their subjects landing. VR-009 (posterior calibration holds) +depends on AR-025 and is what stops the Bayesian accumulation being decoration. + +--- + +# Withdrawn from the old plan + +The phase structure, the `--presence-mode {frame,track}` flag, and "Phase 2 — +retune `anneal_sec`/`extinction_sec`". Those constants are withdrawn rather than +retuned; comparison against old behaviour uses recorded reference output instead +of a second live code path.