Skip to content

Architecture Board Review — 2026-06-19

Subject: ADR-0023 — the offline camera/CV vision/ track (realises the deferred Track B sensing path from ADR-0020). Method: seven independent senior experts reviewed the plan in parallel; concerns were adversarially verified against the published model cards, server/src/types.ts, ffmpeg/v360 behaviour, and the repo's own metric/privacy rules; a chair de-duplicated and adjudicated using the project's standing priority — children's-data privacy > correctness/honesty > performance — at hobby scale.

Panel

Computer-vision / ML engineer · Sports-analytics & pitch-geometry · Video / media & GPU-performance · Privacy / data-protection (children's data) · Software architect / systems integration · Delivery lead / pragmatist (YAGNI) · MLOps / reproducibility & testing.

Board verdict: APPROVE WITH CHANGES (unanimous)

The plan is genuinely strong and unusually honest, and the core decisions must not be reopened: the two-tier success criteria (pipeline-correctness checkable now vs accuracy deferred behind fine-tuning on footage that does not yet exist), the fixed-camera compute-H-once geometry, recall-is-the-bottleneck ball honesty with capped interpolation, deterministic team anchoring, the swappable-stage WorldState contract, and the public-adult-footage-only privacy gate. It is not a plain "approve" because three controls are asserted more strongly than they are built: the no-network gate proves something weaker than "incapable of phoning home"; the privacy firewall (vision/.gitignore) is never sequenced to exist before the first clip lands; and there is no model-fetch integrity pinning — for a children's-data project these are the load-bearing controls and must be hardened before any code runs. It is not "rework" because no redesign is needed: the spine is right and every fix below is a bounded doc-or-test edit, cheap now and expensive after a quiet failure. Several metric definitions also quietly contradict the project's own GPS metric-hygiene rules and must be reconciled before they emit numbers, and one documented ffmpeg command cannot run as written.

What the plan gets right — do not reopen

  • Two-tier success criteria (pipeline-correctness now vs accuracy deferred): the single best decision in the doc. It lets a no-target-footage project make falsifiable progress without quoting broadcast-trained accuracy as if it transfers.
  • Fixed-camera ⇒ compute homography once from hand-picked landmarks, project feet not centre, undistort first — sidesteps the central-view per-frame keypoint jitter problem entirely; auto-calibration correctly deferred to v4.
  • Ball honesty: recall (not precision) named as the bottleneck; mandatory ≤0.5 s capped-gap interpolation with an interpolated=True flag and ball_known_fraction; over-cap gaps left empty rather than fabricated.
  • Determinism discipline at the team stage (PCA + fixed seed, not UMAP; fit-once; deterministic team-0/1 anchoring) with an explicit cross-run test.
  • Privacy gate held to the right (high) bar: public-adult-footage-only across all four phases, the ADR-0020 child-video DPIA explicitly inherited-not-discharged, ephemeral per-clip embeddings, no identity DB / no face recognition, fetch_models.py as the sole network-touching module.
  • Honest, swappable architecture: frozen WorldState with crisp per-stage I/O tables; the AGPL analysis correctly spots that the escape hatch is a detector + tracker swap (BoT-SORT is also Ultralytics/AGPL), not one module; stitch.py correctly scoped as a gated spike with no premature ID-switch target; capture-planning facts (GoPro thermal ~30 min, 50–65 GB/hr, CPU-bound de-warp) surfaced honestly.
  • YAGNI respected: vendor-the-recipe (Roboflow sports pinned to a SHA), one model resident at a time on the 12 GB 3060, SAHI opt-in/benchmark-first, no 3D ball height, no jersey OCR.

Must-fix before building (blockers)

  1. The no-network test does not prove "incapable of phoning home." As specified it asserts the run completes with egress blocked plus a static pipeline.py-doesn't-import-fetch_models check. That only exercises branches the short clip hits and does not constrain native code in ultralytics/transformers/roboflow, which historically auto-download missing weights and emit telemetry. For children's footage this is the most load-bearing technical control in the spec. Change: in §3/§11, (a) intercept socket.socket/connect and fail on any connection attempt (assert zero attempts, not zero swallowed exceptions); (b) have pipeline.py set HF_HUB_OFFLINE=1, TRANSFORMERS_OFFLINE=1 and ultralytics analytics-off unconditionally at startup, and assert they are set; (c) run two cases — models pre-populated, and a weight deliberately removed — asserting the second errors rather than fetches; (d) re-word the §3 claim to "pipeline.py sets offline guards and makes no run-time network calls; enforced by env guards + a fail-on-connect test." (Edit §3 final bullet, §5 fetch_models, §11 no-network test.)

  2. No model-fetch integrity pinning — the reproducibility and privacy claim both rest on it. fetch_models.py pins no SHA/version, and Roboflow Universe slugs can be re-trained under the same name, so two fetches months apart silently yield different weights and different stats.json; provenance.detector records only the model name, so the divergence is invisible. You cannot prove the binary you ran is the one you reviewed. Change: have fetch_models.py write a committed models/MANIFEST.json (per weight: URL, Roboflow model + dataset version, SHA256, byte size, fetch timestamp); verify SHA256 on load and refuse to run on mismatch; surface the weight hash in stats.json provenance. One committed manifest — no registry, no signing. (Edit §5 weight-fetch, §7.4 provenance.)

  3. The privacy firewall (vision/.gitignore) is never sequenced to exist before the first clip lands. The whole "no footage/weights in the repo" assurance rests on models/, samples/, out/ being gitignored, but the §6 tree only annotates this in prose and names no vision/.gitignore deliverable; a non-technical owner following §9 setup creates samples/ and drops a clip in with no ignore rule in force. The AGPL "should the repo ever be published" path makes git initialisation a live scenario. Change: list a committed vision/.gitignore as an explicit v1 deliverable in §6 (ignoring models/, samples/, out/, config/calibration.yaml, *.360, *.mp4); sequence it first in §9, before samples/ exists; add a test asserting no video/weight extensions are tracked under vision/. State it must land with the very first commit that introduces vision/. (Edit §6 project structure, §9 setup ordering.)

  4. The documented ffmpeg de-warp command cannot run as written and hides where the cost lands. v360 is a CPU-only filter; with -hwaccel cuda the decoded frames live in GPU memory and the graph fails ("Impossible to convert between the formats…"). The real bottleneck of the entire v4 de-warp is single-threaded CPU pixel-remapping at 8K — the 3060 does not accelerate it. Change: in §9/§10, (a) state plainly that v360 runs on CPU and is the dominant single-threaded cost; -hwaccel cuda only helps HEVC decode + NVENC encode around a CPU-bound middle; (b) either drop -hwaccel cuda (keep -c:v hevc_nvenc) or write the explicit hybrid graph (hwdownload,format=…,v360=…,hwupload_cuda) and note v360 still runs on CPU; (c) note the .360 is HEVC/proprietary-EAC, so the realistic path is GoPro Player export to equirect MP4 first, then ffmpeg v360 — don't imply GS_match.360 feeds -i directly; (d) name the precomputed-remap/remap_opencl route as the genuine GPU option for a fixed virtual camera, and benchmark on the real 8K file before quoting any wall-clock. (Edit §9 GoPro command, §10 cost narrative, §4 dewarp stage, §5 ffmpeg note.)

Should-fix (majors, prioritised)

  1. Distance/speed timebase contradicts the project's own GPS metric-hygiene rules (high materiality). Distance = Σ frame-to-frame pitch_xy deltas on thinned frames, with only an upper sprint-clamp. This is exactly the phantom-distance pathology metric-definitions.md §2.1 gates against on the GPS side, which mandates a walking floor (~0.4 m/s) and prefers Doppler over position-differencing. The CV path has no noise floor, so every cm of homography wobble on a standing player accrues as distance; max_speed_ms from a single inter-frame delta is forbidden by §2.2 ("require ≥0.3 s sustained"). Change: in §7.3 mirror §2.1/2.2/2.3 — add a lower movement floor derived from the per-region reprojection error (the ≤1.5 m near / ≤4 m far figures are the natural per-region thresholds), short moving-average smoothing before differencing, and redefine max_speed_ms as peak sustained over ≥0.3 s; state distance is a lower-noise-floored estimate.

  2. Homography accuracy spec conflates pixels with metres and validates on the fit points (high materiality). Projecting the same landmarks used to fit H back through H is near-zero by construction and tells you nothing about accuracy where radar error actually lives; the threshold is stated in metres while findHomography error is naturally in pixels, and the px→cm scale varies hugely across the frame. The §8 "sidesteps central-view instability entirely" wording also overstates: a constant H removes per-frame jitter but not the underlying ill-conditioning if the visible landmarks (centre circle, halfway line) are clustered mid-pitch — a stable wrong radar is still wrong. Change: validate H on held-out correspondences (pick 6–8, fit on 4, measure on the rest); state the threshold as pixel error at a named image region plus the implied metre error; add a calibrate.py guard that rejects near-collinear / small-convex-hull point sets and warns that a centre-clustered set gives a confidently-wrong far side; soften §8 to "removes per-frame jitter (not the underlying ill-conditioning)." (§7.2, §8, §11 pitch test.)

  3. Possession is computed in the wrong-confidence frame (medium materiality). The 3 m assign radius is applied in pitch_xy where the far side carries the ≤4 m reprojection error the plan itself admits — so far-side possession mis-assigns in a direction correlated with which team is attacking (structured bias). And the headline split renormalises team0+team1 to ~100 even at ball_known_fraction=0.5, so a half-observed clip still reports a confident "57.3 / 42.7." Change: restrict possession to the near/centre zone where reproj ≤ ~1.5 m and emit "far-side possession: not computed"; and when ball_known_fraction < ~0.7, surface the split as low-confidence rather than a crisp number. Both are honesty changes, not new ML. (§7.3.)

  4. Ball pass design is internally contradictory on tiling (medium materiality). The default runs the rejhg model at plain imgsz=1280 and treats 2×2 SAHI as an "opt-in slow path" — but that model was trained on 2×2-tiled 640 crops and its published mAP assumes tiled inference, so the default is the weak path and the "opt-in" path matches the training regime. Change: in v2, benchmark both (plain 1280 vs sv.InferenceSlicer 2×2) for recall and wall-clock; make the recall-per-second winner the documented default; record in §10 which config the wall-clock estimates assume (today they silently assume plain 1280); frame tiling as "matches model training," not "opt-in slow." (§5, §7.4 v2, §8, §10.)

  5. Convergence with the GPS pipeline is overstated and points at the wrong surface (medium materiality). §1/§4 sell "the same positions-over-time shape" so the two "converge later," but the real durable GPS surface is TeamShapeBucket/EventsResult (team-aggregate, lat/lon-equirectangular, no playerId by privacy design), not per-fix Telemetry. The vision side is per-player, pitch-cm, anchored team 0/1. Three concrete mismatches an adapter must bridge: per-player → team-aggregate identity; cm-on-SoccerPitchConfiguration → lat/lon-equirectangular; anchored-team-0/1 → teamless. Change: downgrade §1/§4 to "a structurally analogous positions-over-time series a future adapter can map onto the GPS analytics surface (TeamShapeBucket/EventsResult)"; list the three mismatches; note CV team-shape is pitch-aligned (true length×width) while GPS Track-A is orientation-free hull/stretch/spread, so they are not directly comparable yet. Pure doc edit. (§1, §4, §7.3, §12-Q2.)

  6. The convergence seam has no written data-plane bright line (medium materiality, privacy). §4 deliberately shapes WorldState to meet the live children's-location store, but never states the constraint that must hold when convergence happens. A future integrator could read "same shape, intentional seam" as licence to pipe image-derived artifacts (crops, annotated.mp4, face-bearing frames) into server/. Change: add one sentence to §4/§12-Q2: the interchange is the de-identified position series only ({track_id, team, pitch_xy, frame_ts}); no image artifact may cross into server/ or any live store; and the integration is itself gated behind the §14 future ADR whenever it touches real youth data. (§4 / §12-Q2.)

  7. Clip-selection safeguard rests on unaided judgement with no durable provenance (medium materiality, privacy). The only record is a free-text note in the gitignored samples/ dir — which the §10 prune deletes — and the rule is "when in doubt, discard," weak against the real failure mode (ambiguous semi-pro/academy/mixed-age footage that looks adult). Change: add a committed, non-gitignored vision/samples.manifest.jsonl with required fields (source URL, channel/uploader, competition/league, adult_senior_confirmed, date); exclude it from the §10 prune; restate the rule as default-deny: "discard any clip whose competition/age cannot be positively identified as adult/senior." (§3, §10.)

  8. Project structure self-shadows (medium materiality, will bite day one). Top-level vision/ contains an inner package vision/ plus a sibling pipeline.py; python -m vision.calibrate (§9) and import vision.types (§11) resolve only by CWD luck — a classic dual-import trap. Change: rename the inner package (e.g. footballcv/) so imports read from footballcv import types and the entry point is python -m footballcv.pipeline; declare one package in pyproject.toml; make §9 run-commands and §11 import paths consistent. (§6.)

  9. Lockfile is promised but never produced; some pins look invented (medium materiality). §5/§6 say versions are "frozen in a lockfile after venv validation" but define no file or tool, and the torch+CUDA wheel index (the part most likely to break a 3060 reinstall) is unpinned. Change: commit a real lock artifact as a v1 deliverable (pip-compile/uv lock, or at minimum pip freeze after the venv validates, matching server/bun.lock), and explicitly pin the torch/torchvision CUDA wheel source (cu12x index for the 3060); re-derive the ultralytics/supervision pins from versions that actually exist at build time. (§5, §6.)

  10. Determinism scope is too narrow to back the §11 "two runs identical" gate (low materiality). Determinism is engineered only at PCA/KMeans, but the gate depends on the whole stack: CUDA cuDNN kernels, BoT-SORT float-equal ties, and TensorRT-FP16 (the perf path) are not bit-reproducible. Change: set PYTHONHASHSEED + torch/np/random seeds + torch.use_deterministic_algorithms(True) where feasible; clarify the v1 criterion to assert team-id mapping/anchoring stability, not bit-identical floats; state TensorRT-FP16 is validated by the tolerance/visual checks, not the equality assertion; pin the assertion to the single device the owner runs (the 3060). (§5, §11.)

  11. Phase exit-criteria objectivity — make the anchoring check numeric (low materiality). Several gates are "looks right" spot-checks with no second reviewer; a left/right anchoring flip still produces "clustered dots," just on the wrong side, and possession depends on anchoring. Change: make only the anchoring check numeric — pick 2 frames with the ball visibly in the left third and assert >X% of the in-possession team's dots have pitch_x in the left third; pin the dewarp line-straightness procedure (overlay a known straight line, assert max pixel deviation ≤ N). Don't numericise the rest. (§7 v2, §11, §7 v4.)

Minor / polish

  • Pretrained ball-model figures: the quoted "mAP@50 ≈ 0.895, recall ≈ 0.78" don't match the public card (mAP@50 ≈ 0.925) and read as remembered numbers; all are the model's own Bundesliga held-out split, not the target view. Footnote them as the model's test-split figures, "target-view recall unmeasured and likely worse — which is why interpolation is mandatory"; verify against the live card at setup. (§5/§8.)
  • track_buffer:90 is in frames and its seconds-meaning is fps-coupled (3 s at native 30 fps, 18 s at 5 fps). Pin it to the tracker fps explicitly. Keep ReID-off as the v1 default but frame it "unevaluated on this view," not "buys little."
  • SigLIP-vs-ResNet/CLIP default is chosen on a blog post, not a measurement. State it's a TODO settled by the §11 labelled-crop test. Add a degenerate-cluster guard: if the two KMeans clusters aren't separated by a minimum margin, flag low team-split confidence rather than emitting a confident 50/50.
  • GK by nearest-centroid mis-assigns exactly during sustained attacks (the phase the possession spot-check evaluates). Prefer the GK's own kit cluster, or exclude GKs from possession/team-shape.
  • Passes is a product of three noisy stages and "no opponent touch between" is undetectable single-camera. Either drop the integer count from v3 or rename it "possession changes" in summary.txt; consider --passes opt-in.
  • Feet-point projection assumes feet are visible and on the ground plane — clipped/occluded far-side bbox bottoms are a distinct pitch_xy error source from homography; document it.
  • Retention posture overstates parity with ADR-0010: the vision prune is manual + size-based; ADR-0010 mandates automatic time-based purge. Soften to "bounded local scratch, lighter than ADR-0010 because public footage"; in §14 state the real-footage gate requires ADR-0010-style automatic purge.
  • annotated.mp4 is a face-bearing derivative — note now that under the §14 real-footage gate it is in-scope for the no-egress/erasure rules exactly like raw clips, not a "safe" output like stats.json.
  • Add an id-space marker to the WorldState series (track_id_space: 'raw'|'stitched') so analytics can assert its precondition instead of trusting call order — this makes the v3 stitch insertion actually safe. Extend stats.json provenance with weight SHA, tracker-config hash, seed, device, engine, vendored-sports SHA, and vision/ git SHA.
  • Two-team / GK / referee assumptions are baked into the frozen team ∈ {0,1} contract; add a one-line note that bibs/mixed-kit youth scenarios are known future contract pressure.
  • Pin a single interpreter (3.11) for the owner's env; treat 3.10–3.13 as "known-compatible, untested."
  • GoPro capture note: flag 300 Mbps as a GoPro Labs (non-default) feature; note the static centre-line tripod is the thermal worst case (plan for 5.6K or a half-time cool-down). Line-item the annotated.mp4 + radar.mp4 NVENC encode passes in the §10 budget, or state outputs are 1080p (a coach review clip doesn't need 4K).
  • Make calibrate.py self-grading (display reprojection error with GOOD/RE-PICK, warn on near-collinear points) and add a pipeline.py --selftest (synthetic clip + no-network test + device/seed/weight-hash + green/red) and a --bench for the §10 perf gate — so a non-technical owner never interprets raw metres or self-certifies a multi-hour run by eye.
  • Commit tiny synthetic test fixtures (no footage) for the pixel-dependent stage tests, plus one URL+SHA256-pinned public clip via fetch_fixtures.py, since samples/ is gitignored and the suite is otherwise unrunnable on a clean checkout.
  • ADR form: header says "Proposed" while the README index lists it as decided and the body reads as decided; it uses a bespoke 14-section build-spec structure, not the house Context→Decision→Consequences→Alternatives. Either add a short house-form "Decision" stanza or explicitly label it "ADR-0023 (build-spec form)" in the README index.

Open questions for the owner

  1. Is the camera/CV track actually going to be built, or is it speculative? If speculative, the must-fix privacy/test hardening still applies the moment any code lands, but the GoPro purchase and v3/v4 work can wait.
  2. Will you accept v2+ running "overnight batch" on a 1-hour match, or does it need a wall-clock ceiling you can live with? TensorRT (the thing that makes the budget hold) is non-trivial setup; the honest default is "v2+ is slow, that's fine" unless you say otherwise.
  3. Does v3 need a hard "passes" count at all, given it's the weakest, most over-claimable metric — or is "possession changes (heuristic)" enough for what you want to see?
  4. For the youth-footage day (still gated, not in this plan): are the children Serbian/in Serbia, and is the club or you the data controller? This is the existing ADR-0020 DPIA question — restated only so it isn't forgotten, since this plan inherits but does not discharge it.

Apply the four must-fix edits to ADR-0023 (no-network enforcement, model-fetch manifest, sequenced vision/.gitignore, corrected v360 command + cost narrative) and the high-materiality majors #1–#2 (distance/speed hygiene, held-out homography validation), since those change what the system emits or guarantees, not just its wording. Fold the remaining majors and the doc-honesty minors (convergence framing, data-plane bright line, lockfile, package rename) in the same pass. Then proceed to the v1 implementation plan — v1 is genuinely thin and ready, and none of the above blocks starting it once the privacy/reproducibility controls are real rather than asserted.

This record summarises the chair's synthesis of seven independent expert reviews, with major/blocker concerns adversarially verified against the model cards, server/src/types.ts, ffmpeg v360 behaviour, and the repo's own metric-definitions and privacy rules. Full per-expert findings are in the board workflow transcript.