From e7d399e51eaf39aed67550f5f67756af995381e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 11:34:19 +0000 Subject: [PATCH] Fix video-seek, leak, and correctness issues found in codebase review Functional bugs: - Clicking a data-table row did not seek the video. The only model -> video time sync was gated on the `scrubbing` flag, which is set only by the slider's pointer DragListener, so the row-click handler moved the model clock while the displayed frame stayed put. - Keyboard scrubbing had the same root cause: Scenery's Slider routes keyboard input through AccessibleValueHandler's startInput/endInput, not the startDrag/endDrag that set `scrubbing`. Both are fixed by centralizing the sync: VideoPlayerNode now links currentTimeProperty -> videoElement.currentTime, guarded by the scrubbing flag and a half-frame deadband so it does not fight timeupdate during playback. startInput/endInput are wired on the scrubber as well. - PlaybackControlsNode orphaned an HSlider at construction. The duration link fired synchronously before this.children was assigned, so replaceScrubber()'s indexOf() returned -1 and the new slider was dropped undisposed, permanently retaining listeners on currentTimeProperty (measured: +4 per construct/dispose cycle). Fixed by using lazyLink and by disposing an uninsertable replacement instead of dropping it. - The OpenCV worker leaked a cv.Mat on every template capture: the intermediate header from blurred.roi() was never deleted. WASM heap, so it is never reclaimed by the JS GC. - Changing the frame rate after digitizing left stored points labelled with indices from the old rate, so everything keyed on `frame` addressed the wrong point. TrackingModel.retimeTrackPoints() re-derives each index from the point's authoritative timestamp; frame collisions from a lowered rate keep the earlier point, matching addPointToTrack()'s first-wins policy. Correctness and robustness: - TableRenderer.update() claimed to rebuild on colour/locale changes but only compared unit and track IDs, relying on DataTableNode observing one representative colour and one representative string. Those fire while their siblings still hold old values. The renderer now compares the full colour/label snapshot, and DataTableNode observes every contributing property. - OpenCVTracker.send() silently overwrote the pending resolve/reject pair, stranding the earlier Promise forever; a superseded request is now rejected. - The worker's no-template branch posted x/y as null against a response type declaring numbers. - calculateTickSpacing() clamped to rangeLength / 20, which could override the 1-2-5 "nice number" result at targetTicks = 15 (the scrubber path). Spacing now escalates through the nice sequence instead. i18n and accessibility: - The compile-time locale parity check covered only en/fr, leaving Spanish free to drift even though the language picker exposes it. - Four accessible names were snapshotted with .value, so they did not re-translate under supportsDynamicLocale. - KinematicsGraphNode leaked the VBox wrapping the track checkboxes on every rebuild. - CSV export revoked the object URL synchronously after click(), which cancels the download in some browsers. Tests: added specs for TrackExporter, ModelViewTransformFactory (including the retransform round-trip invariant), VideoPlaybackModel, frame retiming, and tick spacing; extended the memory-leak suite with listener-count regressions for PlaybackControlsNode. Each new regression test was verified to fail against the unfixed code. 22 -> 61 tests. Docs: corrected the sample-video path, the stale FFmpeg references in the CSP rationale, and the shipped-locale list. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vg2oNMWc4fmuQWmHh6Bxqo --- CLAUDE.md | 11 +- README.md | 2 +- public/opencv-worker.js | 15 ++- src/i18n/StringManager.ts | 9 +- src/track-lab/graph/GraphDataManager.ts | 15 ++- src/track-lab/model/TrackLabModel.ts | 8 ++ src/track-lab/model/TrackingModel.ts | 53 +++++++++ src/track-lab/view/DataTableNode.ts | 67 +++++++---- src/track-lab/view/KinematicsGraphNode.ts | 36 ++++-- src/track-lab/view/PlaybackControlsNode.ts | 47 ++++++-- src/track-lab/view/TableRenderer.ts | 33 +++++- src/track-lab/view/TrackListPanel.ts | 15 ++- src/track-lab/view/VideoPlayerNode.ts | 35 +++++- src/tracking/OpenCVTracker.ts | 15 ++- tests/memory-leak.test.ts | 62 +++++++++- .../track-lab/graph/GraphDataManager.test.ts | 48 ++++++++ .../model/ModelViewTransformFactory.test.ts | 102 ++++++++++++++++ tests/track-lab/model/TrackExporter.test.ts | 89 ++++++++++++++ tests/track-lab/model/TrackRetiming.test.ts | 110 ++++++++++++++++++ .../model/VideoPlaybackModel.test.ts | 84 +++++++++++++ vite.config.ts | 6 +- 21 files changed, 806 insertions(+), 56 deletions(-) create mode 100644 tests/track-lab/graph/GraphDataManager.test.ts create mode 100644 tests/track-lab/model/ModelViewTransformFactory.test.ts create mode 100644 tests/track-lab/model/TrackExporter.test.ts create mode 100644 tests/track-lab/model/TrackRetiming.test.ts create mode 100644 tests/track-lab/model/VideoPlaybackModel.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index d40ea01..538fccd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,6 +34,8 @@ Physics for educators: `doc/model.md`. Architecture: `doc/implementation-notes.m - `ModelViewTransformFactory.buildModelViewTransform()` maps real-world units to video pixels from coordinate-system position/rotation and calibration endpoints. Moving axes or calibration **re-expresses** digitized track points so they stay pinned on the video. - Velocity and acceleration are **finite-difference estimates** from position series — noise amplifies on differentiation. +- **Time sync is one-way and centralized.** `VideoPlayerNode` links `currentTimeProperty` → `videoElement.currentTime` (guarded by the `scrubbing` flag and a half-frame deadband). Writers of `currentTimeProperty` should *not* seek the element themselves; the video follows the model automatically. +- **`TrackPoint.time` is authoritative, `frame` is derived.** Changing the frame rate renumbers `currentFrameProperty`, so `TrackingModel.retimeTrackPoints()` re-derives every stored point's index from its timestamp (wired in `TrackLabModel`). Lowering the rate can collide two points onto one index; first wins. - Up to 4 concurrent tracks (labels A–Z available; symbols are not reused) with independent colors for graphs and table rows. ## Accessibility @@ -67,7 +69,12 @@ Actual specs: - `tests/track-lab/model/KinematicsComputer.test.ts` - `tests/track-lab/model/TrackingModel.test.ts` -- `tests/memory-leak.test.ts` +- `tests/track-lab/model/TrackRetiming.test.ts` — frame indices re-derived from timestamps on frame-rate change +- `tests/track-lab/model/ModelViewTransformFactory.test.ts` — includes the retransform round-trip (points stay pinned to the same video pixel) +- `tests/track-lab/model/TrackExporter.test.ts` +- `tests/track-lab/model/VideoPlaybackModel.test.ts` +- `tests/track-lab/graph/GraphDataManager.test.ts` — tick spacing stays on the 1-2-5 sequence +- `tests/memory-leak.test.ts` — WeakRef dispose regression, plus listener-count regressions for `PlaybackControlsNode` (an HSlider that is rebuilt but never inserted must still be disposed, or it retains listeners on `currentTimeProperty`) Run `npm test`. CI runs the suite when a `test` script is present. @@ -89,5 +96,5 @@ npm run generate-svg-icon # bouncing-ball icon SVG Extra `src/` root files beyond the standard set, each justified by cross-screen use: `TrackLabIcons.ts`, `TrackLabButton.ts`, `webcam.ts`, and the `src/tracking/` folder (OpenCV Web Worker). -- **OpenCV WASM** requires COOP/COEP headers (configured in Vite dev + production). Sample videos in `public/videos/`. +- **OpenCV WASM** requires COOP/COEP headers (configured in Vite dev + production). Sample videos live in top-level `videos/`, served at `/videos/` by the `serveVideos()` Vite plugin (Range-request support for seeking) and copied to `dist/videos/` on build — they are not under `public/`. - **Wall-clock timers (allowed exception)** — webcam/video code uses raw `setTimeout`/`setInterval` rather than `stepTimer`: camera-init timeout in `webcam.ts`, FPS-sampling interval in `WebcamPanel.ts`, source-switch debounce in `VideoSourceControlNode.ts`. These track real hardware time, independent of the sim clock. diff --git a/README.md b/README.md index 61f8fe2..8983fa3 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ export. - Draggable coordinate system and calibration tool for real-world units - OpenCV auto-tracking and manual frame-by-frame digitizing with magnifier - Kinematics graph, data table, measuring tape, and angle tool -- English and French UI, projector color profile, and installable PWA +- English, French, and Spanish UI, projector color profile, and installable PWA - OpenCV.js WASM tracking in a Web Worker (requires COOP/COEP headers) ## Quick Start diff --git a/public/opencv-worker.js b/public/opencv-worker.js index d852178..073223d 100644 --- a/public/opencv-worker.js +++ b/public/opencv-worker.js @@ -112,7 +112,15 @@ self.onmessage = async (event) => { if (templateMat) templateMat.delete(); const roi = new cv.Rect(clampedX, clampedY, roiW, roiH); - templateMat = blurred.roi(roi).clone(); + // roi() returns a new Mat header sharing `blurred`'s buffer; it must be + // deleted explicitly. Only the clone owns memory that outlives this + // message, and WASM heap allocations are never reclaimed by the JS GC. + const roiMat = blurred.roi(roi); + try { + templateMat = roiMat.clone(); + } finally { + roiMat.delete(); + } self.postMessage({ id, @@ -129,7 +137,10 @@ self.onmessage = async (event) => { } } else if (type === 'track') { if (!cv || !templateMat) { - self.postMessage({ id, type: 'track-result', x: null, y: null, confidence: 0 }); + // confidence 0 is below any threshold, so the main thread discards this + // result. x/y are still numbers so the payload matches the declared + // WorkerResponse shape in OpenCVTracker.ts. + self.postMessage({ id, type: 'track-result', x: 0, y: 0, confidence: 0 }); return; } diff --git a/src/i18n/StringManager.ts b/src/i18n/StringManager.ts index 9b3a7af..9815242 100644 --- a/src/i18n/StringManager.ts +++ b/src/i18n/StringManager.ts @@ -36,11 +36,18 @@ function applyStringTest(obj: T): T { } // ── Compile-time key-parity check ───────────────────────────────────────────── -// satisfies errors immediately if either locale file is missing keys from the other. +// satisfies errors immediately if any locale file is missing keys from another. +// Every shipped locale is checked against English in both directions; omitting +// one lets it drift silently, which matters because the language picker +// (supportsDynamicLocale) exposes all of them at runtime. // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion void (stringsEn satisfies typeof stringsFr); // biome-ignore lint/complexity/noVoid: intentional compile-time type assertion void (stringsFr satisfies typeof stringsEn); +// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion +void (stringsEn satisfies typeof stringsEs); +// biome-ignore lint/complexity/noVoid: intentional compile-time type assertion +void (stringsEs satisfies typeof stringsEn); // ── Build the reactive string property tree ─────────────────────────────────── const stringProperties = LocalizedString.getNestedStringProperties({ diff --git a/src/track-lab/graph/GraphDataManager.ts b/src/track-lab/graph/GraphDataManager.ts index 6215525..eb34327 100644 --- a/src/track-lab/graph/GraphDataManager.ts +++ b/src/track-lab/graph/GraphDataManager.ts @@ -20,6 +20,9 @@ export interface GridVisualizationConfig { } export default class GraphDataManager { + /** Upper bound on ticks per axis; spacing escalates until the count fits. */ + private static readonly MAX_TICKS = 20; + // ── Circular buffer ────────────────────────────────────────────────────── // Fixed-size ring buffer so that oldest-point eviction is O(1) rather than // O(n) (Array.shift reindexes every remaining element after removal). @@ -316,7 +319,17 @@ export default class GraphDataManager { spacing = 10 * magnitude; } - return Math.max(spacing, rangeLength / 20); + // Cap the tick count by stepping up through the 1-2-5 sequence rather than + // clamping to rangeLength / MAX_TICKS, which would hand back an arbitrary + // non-round spacing (visible at targetTicks = 15, where the nice value can + // fall just under the cap). + while (rangeLength / spacing > GraphDataManager.MAX_TICKS) { + const decade = 10 ** Math.floor(Math.log10(spacing)); + const step = Math.round(spacing / decade); + spacing = step < 2 ? 2 * decade : step < 5 ? 5 * decade : 10 * decade; + } + + return spacing; } /** diff --git a/src/track-lab/model/TrackLabModel.ts b/src/track-lab/model/TrackLabModel.ts index 8b12383..6ad75a0 100644 --- a/src/track-lab/model/TrackLabModel.ts +++ b/src/track-lab/model/TrackLabModel.ts @@ -45,6 +45,14 @@ export class TrackLabModel { this.overlayTools.modelViewTransformProperty.lazyLink((newMvt, oldMvt) => { this.tracking.retransformTrackPoints(oldMvt, newMvt); }); + + // currentFrameProperty is derived as round(time × fps), so changing the + // frame rate renumbers the *current* frame. Already-digitized points store + // a frame index too, so they must be renumbered from their timestamps in + // the same step or they stop lining up with the frame the UI is on. + this.playback.frameRateProperty.lazyLink((frameRate) => { + this.tracking.retimeTrackPoints(frameRate); + }); } /** diff --git a/src/track-lab/model/TrackingModel.ts b/src/track-lab/model/TrackingModel.ts index 3c25b09..bdf6817 100644 --- a/src/track-lab/model/TrackingModel.ts +++ b/src/track-lab/model/TrackingModel.ts @@ -287,6 +287,59 @@ export class TrackingModel { })); } + /** + * Re-derive every stored point's frame index from its recorded timestamp + * using `frameRate`. Called by TrackLabModel when the frame rate changes. + * + * `time` is the authoritative quantity — it is captured from the video + * element — while `frame` is a derived index that must stay consistent with + * `VideoPlaybackModel.currentFrameProperty`, which recomputes as + * `round(time × fps)`. Without this, changing the frame rate after + * digitizing leaves stored points labelled with indices from the old rate, + * and everything keyed on `frame` (the erase target, the current-frame ring, + * the table row highlight) silently addresses the wrong point. + * + * Lowering the frame rate can map two points onto the same index. Frames are + * unique keys within a track, so a collision keeps the earlier point, + * matching addPointToTrack()'s first-wins policy. + */ + public retimeTrackPoints(frameRate: number): void { + if (!(frameRate > 0)) { + return; + } + const tracks = this.tracksProperty.value; + if (tracks.length === 0) { + return; + } + + this.tracksProperty.value = tracks.map((track) => { + const seen = new Set(); + const points: TrackPoint[] = []; + // Points are already sorted by ascending frame, and frame is monotonic in + // time, so this walks the track in time order — the first point to claim + // an index is the earliest one. + for (const pt of track.points) { + const frame = Math.round(pt.time * frameRate); + if (seen.has(frame)) { + continue; + } + seen.add(frame); + points.push({ ...pt, frame }); + } + return { ...track, points }; + }); + + // The stash holds a point carrying an index from the previous frame rate. + // Restoring it would place it at the wrong frame, so re-derive it too. + const deleted = this.lastDeletedPointProperty.value; + if (deleted) { + this.lastDeletedPointProperty.value = { + trackId: deleted.trackId, + point: { ...deleted.point, frame: Math.round(deleted.point.time * frameRate) }, + }; + } + } + // ── Tracker facade ────────────────────────────────────────────────────── // Views interact with the tracker exclusively through these methods so that // the tracker implementation stays encapsulated inside the model layer. diff --git a/src/track-lab/view/DataTableNode.ts b/src/track-lab/view/DataTableNode.ts index 7daf102..072c98b 100644 --- a/src/track-lab/view/DataTableNode.ts +++ b/src/track-lab/view/DataTableNode.ts @@ -146,8 +146,17 @@ export class DataTableNode extends Node { const link = document.createElement("a"); link.href = url; link.download = `export${this.exportCounter}.csv`; + // The anchor must be in the document for the synthetic click to start + // a download in Firefox, and the object URL must outlive the click: + // revoking it synchronously cancels the download in some browsers. + // Wall-clock setTimeout is deliberate here — this is browser download + // plumbing, not simulation time, so it must not follow the sim clock. + document.body.appendChild(link); link.click(); - URL.revokeObjectURL(url); + setTimeout(() => { + link.remove(); + URL.revokeObjectURL(url); + }, 0); this.exportCounter++; }, }, @@ -196,16 +205,6 @@ export class DataTableNode extends Node { ); }; - const runRebuild = () => { - tableRenderer.rebuild( - tracking.tracksProperty.value, - unitProperty.value, - getTableColors(), - getLabels(), - getA11yLabels(), - ); - }; - const tracksListener = () => runUpdate(); tracking.tracksProperty.link(tracksListener); @@ -215,13 +214,37 @@ export class DataTableNode extends Node { const currentFrameListener = (frame: number) => tableRenderer.setCurrentFrame(frame); playback.currentFrameProperty.link(currentFrameListener); - // Color-theme and locale changes require a full rebuild because cell colours - // and label strings are baked into the DOM. - const tableHeaderBgListener = () => runRebuild(); - TrackLabColors.tableHeaderBackgroundProperty.lazyLink(tableHeaderBgListener); - - const frameStringListener = () => runRebuild(); - dataTableStrings.frameStringProperty.lazyLink(frameStringListener); + // Colour-theme and locale changes are baked into the DOM as CSS strings and + // text nodes, so they need a rebuild. Every contributing property is + // observed rather than one representative of each group: switching colour + // profile or locale updates these properties one at a time, so a single + // representative can fire while its siblings still hold their old values. + // TableRenderer.update() compares the full colour/label snapshot and + // rebuilds when it differs, so the last notification wins with final values. + const themeProperties = [ + TrackLabColors.tableHeaderBackgroundProperty, + TrackLabColors.tableHeaderTextProperty, + TrackLabColors.tableRowOddProperty, + TrackLabColors.tableRowEvenProperty, + TrackLabColors.tableGridStrokeProperty, + TrackLabColors.tableEmptyTextProperty, + TrackLabColors.tableSymbolShadowProperty, + TrackLabColors.tableBackgroundProperty, + TrackLabColors.tableCurrentRowProperty, + ]; + const localeProperties = [ + dataTableStrings.frameStringProperty, + dataTableStrings.timeSecondsStringProperty, + dataTableStrings.noDataStringProperty, + a11yStrings.dataTableStringProperty, + ]; + const themeOrLocaleListener = () => runUpdate(); + for (const property of themeProperties) { + property.lazyLink(themeOrLocaleListener); + } + for (const property of localeProperties) { + property.lazyLink(themeOrLocaleListener); + } const videoLoadedListener = (loaded: boolean) => { this.visible = loaded; @@ -420,8 +443,12 @@ export class DataTableNode extends Node { tracking.tracksProperty.unlink(tracksListener); unitProperty.unlink(unitListener); playback.currentFrameProperty.unlink(currentFrameListener); - TrackLabColors.tableHeaderBackgroundProperty.unlink(tableHeaderBgListener); - dataTableStrings.frameStringProperty.unlink(frameStringListener); + for (const property of themeProperties) { + property.unlink(themeOrLocaleListener); + } + for (const property of localeProperties) { + property.unlink(themeOrLocaleListener); + } videoLoadedProperty.unlink(videoLoadedListener); exportButton.dispose(); panel.localBoundsProperty.unlink(localBoundsListener); diff --git a/src/track-lab/view/KinematicsGraphNode.ts b/src/track-lab/view/KinematicsGraphNode.ts index 5ad83db..28b624f 100644 --- a/src/track-lab/view/KinematicsGraphNode.ts +++ b/src/track-lab/view/KinematicsGraphNode.ts @@ -6,7 +6,7 @@ * The velocity and acceleration groups can be hidden via user preferences. */ -import { Property, type TReadOnlyProperty } from "scenerystack/axon"; +import { DerivedProperty, Property, type TReadOnlyProperty } from "scenerystack/axon"; import { Node, Text, VBox } from "scenerystack/scenery"; import { PhetFont } from "scenerystack/scenery-phet"; import { Checkbox } from "scenerystack/sun"; @@ -35,7 +35,12 @@ export class KinematicsGraphNode extends Node { private readonly tracking: TrackingModel; private readonly selectedTracksProperty: Property>; private readonly trackCheckboxPanel: Node; - private readonly trackCheckboxes: Map }> = new Map(); + private readonly trackCheckboxes: Map< + string, + { checkbox: Checkbox; property: Property; nameProperty: TReadOnlyProperty } + > = new Map(); + /** VBox wrapping the current checkboxes; disposed and replaced on each rebuild. */ + private checkboxContainer: VBox | null = null; private readonly disposeKinematicsGraph: () => void; public constructor( @@ -108,9 +113,10 @@ export class KinematicsGraphNode extends Node { // Update checkboxes when tracks change (link fires immediately, building initial checkboxes) const tracksListener = (tracks: readonly Track[]) => { // Dispose old checkboxes - for (const [, { checkbox, property }] of this.trackCheckboxes) { + for (const [, { checkbox, property, nameProperty }] of this.trackCheckboxes) { checkbox.dispose(); property.dispose(); + nameProperty.dispose(); } this.trackCheckboxes.clear(); @@ -191,11 +197,14 @@ export class KinematicsGraphNode extends Node { preferencesModel.showAccelerationInGraphProperty.unlink(accelerationPrefListener); videoLoadedProperty.unlink(videoLoadedListener); this.graph.localBoundsProperty.unlink(graphBoundsListener); - for (const [, { checkbox, property }] of this.trackCheckboxes) { + for (const [, { checkbox, property, nameProperty }] of this.trackCheckboxes) { checkbox.dispose(); property.dispose(); + nameProperty.dispose(); } this.trackCheckboxes.clear(); + this.checkboxContainer?.dispose(); + this.checkboxContainer = null; this.selectedTracksProperty.dispose(); this.graph.dispose(); }; @@ -213,8 +222,13 @@ export class KinematicsGraphNode extends Node { private rebuildTrackCheckboxes(): void { const tracks = this.tracking.tracksProperty.value; + // The previous container is detached here and disposed either way; leaving + // it to be garbage collected would leak a VBox on every track change. + this.trackCheckboxPanel.children = []; + this.checkboxContainer?.dispose(); + this.checkboxContainer = null; + if (tracks.length === 0) { - this.trackCheckboxPanel.children = []; return; } @@ -241,18 +255,23 @@ export class KinematicsGraphNode extends Node { fill: getTrackColor(track.colorIndex), }); - // Create checkbox with accessibility label + // Create checkbox with accessibility label. Derived rather than a + // `.value` snapshot so the name re-translates when the user switches + // language (the sim enables supportsDynamicLocale). const kinematicsGraphStrings = StringManager.getInstance().getKinematicsGraph(); + const nameProperty = new DerivedProperty([kinematicsGraphStrings.trackItemStringProperty], (pattern) => + pattern.split("{{symbol}}").join(track.symbol), + ); const checkbox = new Checkbox(checkboxProperty, label, { checkboxColor: TrackLabColors.checkboxColorProperty, checkboxColorBackground: TrackLabColors.checkboxColorBackgroundProperty, spacing: 4, - accessibleName: kinematicsGraphStrings.trackItemStringProperty.value.split("{{symbol}}").join(track.symbol), + accessibleName: nameProperty, }); checkbox.addInputListener({ down: () => checkbox.focus() }); // Store for later disposal - this.trackCheckboxes.set(track.id, { checkbox, property: checkboxProperty }); + this.trackCheckboxes.set(track.id, { checkbox, property: checkboxProperty, nameProperty }); checkboxNodes.push(checkbox); } @@ -264,6 +283,7 @@ export class KinematicsGraphNode extends Node { align: "left", }); + this.checkboxContainer = checkboxContainer; this.trackCheckboxPanel.children = [checkboxContainer]; this.updateCheckboxPositions(); } diff --git a/src/track-lab/view/PlaybackControlsNode.ts b/src/track-lab/view/PlaybackControlsNode.ts index 3010097..04015af 100644 --- a/src/track-lab/view/PlaybackControlsNode.ts +++ b/src/track-lab/view/PlaybackControlsNode.ts @@ -144,12 +144,22 @@ export class PlaybackControlsNode extends HBox { minorTickLength: 4, minorTickStroke: TrackLabColors.textOnDarkProperty, minorTickLineWidth: 1, + // startDrag/endDrag fire for pointer interaction only. Keyboard input + // goes through AccessibleValueHandler's startInput/endInput, so both + // pairs are wired — otherwise arrow-key scrubbing would leave + // isScrubbing false and the video would never follow the thumb. startDrag: () => { this.isScrubbing = true; }, endDrag: () => { this.isScrubbing = false; }, + startInput: () => { + this.isScrubbing = true; + }, + endInput: () => { + this.isScrubbing = false; + }, enabledProperty: playback.videoLoadedProperty, accessibleName: a11yStrings.videoScrubberStringProperty, }); @@ -185,14 +195,20 @@ export class PlaybackControlsNode extends HBox { this.scrubber = createScrubber(); - // Update range and ticks when duration changes + // Update range and ticks when duration changes. + // lazyLink, not link: the scrubber built just above already reflects the + // current duration, and a synchronous first firing would run + // replaceScrubber() before this.children is assigned at the end of the + // constructor — indexOf() would return -1 and the freshly built slider + // would be dropped undisposed, permanently retaining its listeners on + // currentTimeProperty. const durationListener = (duration: number) => { // Update scrubber range — guard against Infinity (reported by WebM files) this.scrubberRange.max = Number.isFinite(duration) && duration > 0 ? duration : 1; // Recreate scrubber with new tick marks this.replaceScrubber(createScrubber()); }; - playback.durationProperty.link(durationListener); + playback.durationProperty.lazyLink(durationListener); // Recreate scrubber when frame rate changes const frameRateListener = () => { @@ -352,19 +368,26 @@ export class PlaybackControlsNode extends HBox { const oldScrubber = this.scrubber; const scrubberIndex = this.children.indexOf(oldScrubber); - if (scrubberIndex !== -1) { - // Remove old scrubber from scene graph first - this.removeChild(oldScrubber); + if (scrubberIndex === -1) { + // The current scrubber is not in the scene graph, so there is nothing to + // swap it with. Dispose the replacement rather than dropping it on the + // floor: an undisposed HSlider keeps listeners on currentTimeProperty + // alive forever. + newScrubber.dispose(); + return; + } - // Update reference - this.scrubber = newScrubber; + // Remove old scrubber from scene graph first + this.removeChild(oldScrubber); - // Insert new scrubber at the same position - this.insertChild(scrubberIndex, newScrubber); + // Update reference + this.scrubber = newScrubber; - // Dispose old scrubber after it's removed from scene graph - oldScrubber.dispose(); - } + // Insert new scrubber at the same position + this.insertChild(scrubberIndex, newScrubber); + + // Dispose old scrubber after it's removed from scene graph + oldScrubber.dispose(); } public get scrubbing(): boolean { diff --git a/src/track-lab/view/TableRenderer.ts b/src/track-lab/view/TableRenderer.ts index 203dbd8..f6468d1 100644 --- a/src/track-lab/view/TableRenderer.ts +++ b/src/track-lab/view/TableRenderer.ts @@ -75,6 +75,26 @@ export type A11yLabels = { // ── Private helpers ─────────────────────────────────────────────────────────── +/** Stable serialisation of every colour baked into the table DOM. */ +function colorsKey(colors: TableColors): string { + return [ + colors.headerBg, + colors.headerText, + colors.rowOdd, + colors.rowEven, + colors.gridStroke, + colors.emptyText, + colors.symbolShadow, + colors.background, + colors.currentRow, + ].join("|"); +} + +/** Stable serialisation of every string baked into the table DOM. */ +function labelsKey(labels: TableLabels, a11y: A11yLabels): string { + return [labels.frame, labels.timeSeconds, labels.noData, a11y.tableCaption].join("|"); +} + function makeCellStyle(colors: TableColors): string { return `padding: ${TABLE_CELL_PADDING_Y}px ${TABLE_CELL_PADDING_X}px; border: 1px solid ${colors.gridStroke}; text-align: center;`; } @@ -262,6 +282,13 @@ export class TableRenderer { // ── Incremental-update state ─────────────────────────────────────────────── private lastTrackIds: string[] = []; private lastUnit: string = ""; + // Colours and labels are baked into the DOM as CSS strings and text nodes, so + // a change to any of them needs a structural rebuild. Comparing serialised + // snapshots means correctness does not depend on *which* colour or string + // property happened to fire the update: whichever notification arrives last + // with the final values is the one that rebuilds. + private lastColorsKey: string = ""; + private lastLabelsKey: string = ""; private tableBodyRef: HTMLTableSectionElement | null = null; private readonly frameRowMap: Map = new Map(); private maxRenderedFrame: number = -Infinity; @@ -402,6 +429,8 @@ export class TableRenderer { this.lastTrackIds = tracks.map((t) => t.id); this.lastUnit = unit; + this.lastColorsKey = colorsKey(colors); + this.lastLabelsKey = labelsKey(labels, a11y); // The rebuild discarded the highlighted ; restore it on the new DOM. // No scroll — the frame has not moved, only the rows around it. @@ -429,7 +458,9 @@ export class TableRenderer { const isStructural = unit !== this.lastUnit || trackIds.length !== this.lastTrackIds.length || - trackIds.some((id, i) => id !== this.lastTrackIds[i]); + trackIds.some((id, i) => id !== this.lastTrackIds[i]) || + colorsKey(colors) !== this.lastColorsKey || + labelsKey(labels, a11y) !== this.lastLabelsKey; if (isStructural || !this.tableBodyRef) { this.rebuild(tracks, unit, colors, labels, a11y); diff --git a/src/track-lab/view/TrackListPanel.ts b/src/track-lab/view/TrackListPanel.ts index 51e7f79..71fc419 100644 --- a/src/track-lab/view/TrackListPanel.ts +++ b/src/track-lab/view/TrackListPanel.ts @@ -104,10 +104,19 @@ class TrackRowNode extends Node { }; isDigitizingProperty.lazyLink(digitizingListener); + // Derived rather than a `.value` snapshot so the name re-translates when the + // user switches language (the sim enables supportsDynamicLocale). + const digitizeNameProperty = new DerivedProperty([a11yStrings.digitizeTrackStringProperty], (pattern) => + pattern.split("{{symbol}}").join(track.symbol), + ); + const removeNameProperty = new DerivedProperty([a11yStrings.removeTrackStringProperty], (pattern) => + pattern.split("{{symbol}}").join(track.symbol), + ); + const checkbox = new Checkbox(isDigitizingProperty, new Rectangle(0, 0, 0, 0), { boxWidth: CHECKBOX_BOX_WIDTH, tandem: Tandem.OPT_OUT, - accessibleName: a11yStrings.digitizeTrackStringProperty.value.split("{{symbol}}").join(track.symbol), + accessibleName: digitizeNameProperty, }); checkbox.addInputListener({ down: () => checkbox.focus() }); checkbox.left = CHECKBOX_X; @@ -117,7 +126,7 @@ class TrackRowNode extends Node { const trashButton = createTrackLabButton(makeTrashIcon(), { baseColor: TrackLabColors.trashButtonBaseProperty, listener: () => tracking.removeTrack(track.id), - accessibleName: a11yStrings.removeTrackStringProperty.value.split("{{symbol}}").join(track.symbol), + accessibleName: removeNameProperty, }); trashButton.centerY = ROW_CY; trashButton.right = PANEL_WIDTH - TRASH_BUTTON_RIGHT_OFFSET; @@ -135,6 +144,8 @@ class TrackRowNode extends Node { checkbox.dispose(); trashButton.dispose(); isDigitizingProperty.dispose(); + digitizeNameProperty.dispose(); + removeNameProperty.dispose(); }; } diff --git a/src/track-lab/view/VideoPlayerNode.ts b/src/track-lab/view/VideoPlayerNode.ts index 8c2fc29..5b88780 100644 --- a/src/track-lab/view/VideoPlayerNode.ts +++ b/src/track-lab/view/VideoPlayerNode.ts @@ -85,7 +85,12 @@ export class VideoPlayerNode extends Node { this.videoElement.preload = "metadata"; this.videoElement.crossOrigin = "anonymous"; this.videoElement.style.display = "block"; - this.videoElement.setAttribute("aria-label", a11yStrings.videoPlayerStringProperty.value); + // Linked rather than snapshotted so the label re-translates when the user + // switches language (the sim enables supportsDynamicLocale). + const videoAriaLabelListener = (label: string) => { + this.videoElement.setAttribute("aria-label", label); + }; + a11yStrings.videoPlayerStringProperty.link(videoAriaLabelListener); const videoBackgroundListener = (c: import("scenerystack").Color) => { this.videoElement.style.background = c.toCSS(); }; @@ -373,6 +378,32 @@ export class VideoPlayerNode extends Node { }; this.videoElement.addEventListener("timeupdate", onTimeUpdate, { signal }); + // ── Sync video element from model time (model → video) ───────────────── + // Every writer of currentTimeProperty used to be responsible for also + // assigning videoElement.currentTime itself. Two writers did not — the + // data-table row click and the scrubber's *keyboard* interaction (Scenery's + // Slider routes keyboard input through startInput/endInput, not the + // startDrag/endDrag that set `scrubbing`) — so the model clock moved while + // the displayed frame stayed put. This single listener makes the sync + // structural instead of a convention each call site has to remember. + // + // Two guards keep it from fighting the other direction of the loop: + // - `scrubbing`: pointer drags are already synced in PlaybackControlsNode. + // - half-frame deadband: during playback onTimeUpdate writes the video's + // own time into the model, which lands back here; seeking the element + // to the time it already holds would stutter playback. + const syncVideoFromModelTime = (time: number) => { + if (this.playbackControlsNode.scrubbing || !Number.isFinite(time)) { + return; + } + const halfFrame = model.playback.frameDurationProperty.value / 2; + if (Math.abs(this.videoElement.currentTime - time) < halfFrame) { + return; + } + this.videoElement.currentTime = time; + }; + model.playback.currentTimeProperty.lazyLink(syncVideoFromModelTime); + // ── Apply video transform (translate + uniform scale) ──────────────── const videoTransformListener = (matrix: import("scenerystack/dot").Matrix3) => { this.videoContentLayer.matrix = matrix; @@ -394,6 +425,8 @@ export class VideoPlayerNode extends Node { model.playback.playbackRateProperty.unlink(playbackRateListener); model.playback.videoTransformProperty.unlink(videoTransformListener); model.playback.panelSizeScaleProperty.unlink(panelSizeScaleListener); + model.playback.currentTimeProperty.unlink(syncVideoFromModelTime); + a11yStrings.videoPlayerStringProperty.unlink(videoAriaLabelListener); if (this.currentBlobUrl) { URL.revokeObjectURL(this.currentBlobUrl); this.currentBlobUrl = null; diff --git a/src/tracking/OpenCVTracker.ts b/src/tracking/OpenCVTracker.ts index e0fc483..08c2f43 100644 --- a/src/tracking/OpenCVTracker.ts +++ b/src/tracking/OpenCVTracker.ts @@ -162,8 +162,21 @@ export class OpenCVTracker { } } - /** Send a message to the worker and return a Promise that resolves with the response. */ + /** + * Send a message to the worker and return a Promise that resolves with the response. + * + * Only one request may be in flight at a time. Callers currently serialise + * their own calls, but overwriting the pending handlers without settling them + * would strand the earlier Promise forever (an `await` that never returns), + * so a superseded request is rejected explicitly. + */ private send(msg: object): Promise { + if (this.pendingReject) { + this.pendingReject(new Error("Tracker request superseded by a newer request")); + this.pendingResolve = null; + this.pendingReject = null; + this.pendingId = -1; + } const id = this.nextMsgId++; return new Promise((resolve, reject) => { this.pendingId = id; diff --git a/tests/memory-leak.test.ts b/tests/memory-leak.test.ts index 3cb5037..56ffb83 100644 --- a/tests/memory-leak.test.ts +++ b/tests/memory-leak.test.ts @@ -7,10 +7,13 @@ * a block scope) so local strong references die when the helper returns. */ -import { Property } from "scenerystack/axon"; +import { Property, type TReadOnlyProperty } from "scenerystack/axon"; import { describe, expect, it } from "vitest"; import GraphControlsPanel from "../src/track-lab/graph/GraphControlsPanel.js"; import type { PlottableProperty } from "../src/track-lab/graph/PlottableProperty.js"; +import { TrackingModel } from "../src/track-lab/model/TrackingModel.js"; +import { VideoPlaybackModel } from "../src/track-lab/model/VideoPlaybackModel.js"; +import { PlaybackControlsNode } from "../src/track-lab/view/PlaybackControlsNode.js"; /** * Force garbage collection with multiple passes. When `earlyExitRef` is supplied @@ -46,6 +49,24 @@ const POSITION_PLOTTABLE: PlottableProperty = { const SAMPLE_PLOTTABLES: PlottableProperty[] = [TIME_PLOTTABLE, POSITION_PLOTTABLE]; +/** + * Number of listeners currently attached to `property`. + * + * Axon's TinyEmitter exposes getListenerCount() publicly but ReadOnlyProperty + * re-declares it private, so the cast is deliberate. The public alternative, + * hasListeners(), is too coarse: these properties always carry listeners from + * the model's own DerivedProperties, so only the exact count distinguishes a + * clean dispose from a retained one. + */ +function listenerCount(property: TReadOnlyProperty): number { + return (property as unknown as { getListenerCount(): number }).getListenerCount(); +} + +/** Shared no-op callback for view constructors under test. */ +const noop = (): void => { + /* no-op */ +}; + function createAndDisposeGraphControlsPanel(): WeakRef { // Explicit type argument: TypeScript narrows these consts to RecordPlottable // from their initializers, and Property is invariant in T. @@ -86,6 +107,45 @@ describe("Memory leak regression", () => { yPropertyProperty.dispose(); }); + // PlaybackControlsNode builds an HSlider bound to currentTimeProperty and + // rebuilds it whenever duration, frame rate, or frame count changes. A + // replacement that is never inserted into the scene graph must still be + // disposed: an orphaned HSlider keeps its listeners on the model property + // alive for the lifetime of the model, which no WeakRef on the node catches. + it("PlaybackControlsNode leaves no listeners on currentTimeProperty after dispose", () => { + const playback = new VideoPlaybackModel(); + const tracking = new TrackingModel(); + const videoElement = document.createElement("video"); + + const baseline = listenerCount(playback.currentTimeProperty); + + for (let i = 0; i < 3; i++) { + const node = new PlaybackControlsNode(playback, tracking, videoElement, noop, noop, noop); + node.dispose(); + } + + expect(listenerCount(playback.currentTimeProperty)).toBe(baseline); + }); + + it("PlaybackControlsNode scrubber rebuilds do not accumulate listeners", () => { + const playback = new VideoPlaybackModel(); + const tracking = new TrackingModel(); + const videoElement = document.createElement("video"); + + const baseline = listenerCount(playback.currentTimeProperty); + const node = new PlaybackControlsNode(playback, tracking, videoElement, noop, noop, noop); + + const afterConstruct = listenerCount(playback.currentTimeProperty); + // Each of these triggers replaceScrubber(). + playback.durationProperty.value = 10; + playback.durationProperty.value = 20; + playback.frameRateProperty.value = 60; + expect(listenerCount(playback.currentTimeProperty)).toBe(afterConstruct); + + node.dispose(); + expect(listenerCount(playback.currentTimeProperty)).toBe(baseline); + }); + it("repeated create/dispose cycles leave no survivors", async () => { const refs: WeakRef[] = []; for (let i = 0; i < 10; i++) { diff --git a/tests/track-lab/graph/GraphDataManager.test.ts b/tests/track-lab/graph/GraphDataManager.test.ts new file mode 100644 index 0000000..e4c6419 --- /dev/null +++ b/tests/track-lab/graph/GraphDataManager.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import GraphDataManager from "../../../src/track-lab/graph/GraphDataManager.js"; + +/** True for values of the form (1|2|5) x 10^k, the "nice number" sequence. */ +function isNiceNumber(value: number): boolean { + const decade = 10 ** Math.floor(Math.log10(value)); + const mantissa = value / decade; + return [1, 2, 5, 10].some((nice) => Math.abs(mantissa - nice) < 1e-9); +} + +describe("GraphDataManager.calculateTickSpacing", () => { + it("falls back to 1 for degenerate ranges", () => { + expect(GraphDataManager.calculateTickSpacing(0)).toBe(1); + expect(GraphDataManager.calculateTickSpacing(-5)).toBe(1); + expect(GraphDataManager.calculateTickSpacing(Number.NaN)).toBe(1); + expect(GraphDataManager.calculateTickSpacing(Number.POSITIVE_INFINITY)).toBe(1); + }); + + it("returns round numbers at the default tick target", () => { + for (const range of [1, 3, 7, 10, 47, 100, 250, 1000, 0.05]) { + expect(isNiceNumber(GraphDataManager.calculateTickSpacing(range))).toBe(true); + } + }); + + it("returns round numbers at the scrubber's 15-tick target", () => { + // Regression: the old rangeLength / 20 floor could override the nice value + // here and hand back arbitrary spacing like 15.35. + for (const totalFrames of [21, 33, 44, 52, 62, 176, 191, 249, 307, 328, 1000]) { + const spacing = GraphDataManager.calculateTickSpacing(totalFrames, 15); + expect(isNiceNumber(spacing)).toBe(true); + } + }); + + it("never produces more than 20 ticks", () => { + for (const targetTicks of [5, 10, 15, 20]) { + for (const range of [1, 17, 33, 99, 250, 1234]) { + const spacing = GraphDataManager.calculateTickSpacing(range, targetTicks); + expect(range / spacing).toBeLessThanOrEqual(20 + 1e-9); + } + } + }); + + it("stays near the requested tick count", () => { + const spacing = GraphDataManager.calculateTickSpacing(100, 5); + expect(100 / spacing).toBeGreaterThanOrEqual(2); + expect(100 / spacing).toBeLessThanOrEqual(10); + }); +}); diff --git a/tests/track-lab/model/ModelViewTransformFactory.test.ts b/tests/track-lab/model/ModelViewTransformFactory.test.ts new file mode 100644 index 0000000..f07b31c --- /dev/null +++ b/tests/track-lab/model/ModelViewTransformFactory.test.ts @@ -0,0 +1,102 @@ +import { type Transform3, Vector2 } from "scenerystack/dot"; +import { describe, expect, it } from "vitest"; +import { buildModelViewTransform } from "../../../src/track-lab/model/ModelViewTransformFactory.js"; +import { TrackingModel } from "../../../src/track-lab/model/TrackingModel.js"; + +/** 100 px maps to 1 model unit, origin at (100, 100), no rotation. */ +function standardTransform(): Transform3 { + return buildModelViewTransform(new Vector2(100, 100), 0, new Vector2(0, 0), new Vector2(100, 0), 1); +} + +describe("buildModelViewTransform", () => { + it("places the model origin at the coordinate-system position", () => { + const mvt = standardTransform(); + const pixel = mvt.transformPosition2(new Vector2(0, 0)); + + expect(pixel.x).toBeCloseTo(100); + expect(pixel.y).toBeCloseTo(100); + }); + + it("scales model units to pixels using the calibration length", () => { + const mvt = standardTransform(); + const pixel = mvt.transformPosition2(new Vector2(2, 0)); + + expect(pixel.x).toBeCloseTo(300); // 100 px origin + 2 units x 100 px/unit + }); + + it("flips y so that model +y points up the screen", () => { + const mvt = standardTransform(); + const pixel = mvt.transformPosition2(new Vector2(0, 1)); + + // Screen y grows downward, so a positive model y lands above the origin. + expect(pixel.y).toBeCloseTo(0); + }); + + it("round-trips a pixel through inverse and forward transforms", () => { + const mvt = buildModelViewTransform(new Vector2(37, 91), 0.7, new Vector2(10, 20), new Vector2(150, 60), 2.5); + const pixel = new Vector2(400, 250); + + const roundTripped = mvt.transformPosition2(mvt.inversePosition2(pixel)); + + expect(roundTripped.x).toBeCloseTo(pixel.x); + expect(roundTripped.y).toBeCloseTo(pixel.y); + }); + + it("returns identity when the calibration segment has zero length", () => { + const mvt = buildModelViewTransform(new Vector2(50, 50), 0, new Vector2(10, 10), new Vector2(10, 10), 1); + const pixel = mvt.transformPosition2(new Vector2(7, 8)); + + expect(pixel.x).toBeCloseTo(7); + expect(pixel.y).toBeCloseTo(8); + }); + + it("returns identity when the calibration distance is degenerate", () => { + const mvt = buildModelViewTransform(new Vector2(50, 50), 0, new Vector2(0, 0), new Vector2(100, 0), 0); + const pixel = mvt.transformPosition2(new Vector2(7, 8)); + + expect(pixel.x).toBeCloseTo(7); + expect(pixel.y).toBeCloseTo(8); + }); +}); + +describe("retransformTrackPoints", () => { + it("keeps every digitized point pinned to the same video pixel", () => { + const model = new TrackingModel(); + model.addTrack(); + + const oldMvt = standardTransform(); + // Digitize three points expressed in the OLD transform's model coordinates. + const originalPixels = [new Vector2(150, 120), new Vector2(300, 400), new Vector2(20, 60)]; + originalPixels.forEach((pixel, i) => { + const modelPt = oldMvt.inversePosition2(pixel); + model.addOrReplacePointOnTrack("track-A", i, i, modelPt.x, modelPt.y); + }); + + // Move the origin, rotate the axes, and rescale the calibration. + const newMvt = buildModelViewTransform(new Vector2(250, 60), 0.4, new Vector2(0, 0), new Vector2(37, 0), 3); + model.retransformTrackPoints(oldMvt, newMvt); + + const points = model.tracksProperty.value[0]?.points ?? []; + expect(points).toHaveLength(3); + points.forEach((pt, i) => { + const pixel = newMvt.transformPosition2(new Vector2(pt.x, pt.y)); + expect(pixel.x).toBeCloseTo(originalPixels[i]?.x ?? Number.NaN); + expect(pixel.y).toBeCloseTo(originalPixels[i]?.y ?? Number.NaN); + }); + }); + + it("leaves frame and time untouched", () => { + const model = new TrackingModel(); + model.addTrack(); + model.addOrReplacePointOnTrack("track-A", 7, 0.25, 1, 1); + + model.retransformTrackPoints( + standardTransform(), + buildModelViewTransform(new Vector2(0, 0), 1, new Vector2(0, 0), new Vector2(50, 0), 1), + ); + + const point = model.tracksProperty.value[0]?.points[0]; + expect(point?.frame).toBe(7); + expect(point?.time).toBe(0.25); + }); +}); diff --git a/tests/track-lab/model/TrackExporter.test.ts b/tests/track-lab/model/TrackExporter.test.ts new file mode 100644 index 0000000..f8b5538 --- /dev/null +++ b/tests/track-lab/model/TrackExporter.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; +import type { Track } from "../../../src/track-lab/model/Track.js"; +import { buildDataRows, generateCsv } from "../../../src/track-lab/model/TrackExporter.js"; + +const LABELS = { frame: "Frame", timeSeconds: "t (s)" }; + +function track(id: string, symbol: string, points: Array<[number, number, number, number]>): Track { + return { + id, + symbol, + colorIndex: 0, + points: points.map(([frame, time, x, y]) => ({ frame, time, x, y })), + }; +} + +describe("buildDataRows", () => { + it("returns no rows when there are no tracks", () => { + expect(buildDataRows([])).toEqual([]); + }); + + it("returns no rows when tracks carry no points", () => { + expect(buildDataRows([track("track-A", "A", [])])).toEqual([]); + }); + + it("sorts rows ascending by frame regardless of insertion order", () => { + const a = track("track-A", "A", [ + [5, 0.5, 1, 1], + [0, 0, 2, 2], + [3, 0.3, 3, 3], + ]); + + expect(buildDataRows([a]).map((r) => r.frame)).toEqual([0, 3, 5]); + }); + + it("merges two tracks that share a frame into one row", () => { + const a = track("track-A", "A", [[2, 0.2, 1, 1]]); + const b = track("track-B", "B", [[2, 0.2, 9, 9]]); + + const rows = buildDataRows([a, b]); + + expect(rows).toHaveLength(1); + expect(rows[0]?.values.get("track-A")).toEqual({ x: 1, y: 1 }); + expect(rows[0]?.values.get("track-B")).toEqual({ x: 9, y: 9 }); + }); + + it("keeps frames that only one track recorded", () => { + const a = track("track-A", "A", [ + [0, 0, 1, 1], + [1, 0.1, 2, 2], + ]); + const b = track("track-B", "B", [[1, 0.1, 8, 8]]); + + const rows = buildDataRows([a, b]); + + expect(rows.map((r) => r.frame)).toEqual([0, 1]); + expect(rows[0]?.values.has("track-B")).toBe(false); + }); +}); + +describe("generateCsv", () => { + it("emits a header per track and one line per frame", () => { + const a = track("track-A", "A", [ + [0, 0, 1, 2], + [1, 0.5, 3, 4], + ]); + + const lines = generateCsv([a], "m", LABELS).split("\n"); + + expect(lines[0]).toBe("Frame,t (s),x_A (m),y_A (m)"); + expect(lines).toHaveLength(3); + expect(lines[1]).toBe("0,0.0000,1.0000,2.0000"); + }); + + it("leaves cells empty for a track with no point at that frame", () => { + const a = track("track-A", "A", [[0, 0, 1, 2]]); + const b = track("track-B", "B", [[1, 0.5, 3, 4]]); + + const lines = generateCsv([a, b], "cm", LABELS).split("\n"); + + // Frame 0: track A has data, track B does not. + expect(lines[1]).toBe("0,0.0000,1.0000,2.0000,,"); + // Frame 1: the reverse. + expect(lines[2]).toBe("1,0.5000,,,3.0000,4.0000"); + }); + + it("emits only the header row when there is no data", () => { + expect(generateCsv([], "m", LABELS)).toBe("Frame,t (s)"); + }); +}); diff --git a/tests/track-lab/model/TrackRetiming.test.ts b/tests/track-lab/model/TrackRetiming.test.ts new file mode 100644 index 0000000..f54aa0c --- /dev/null +++ b/tests/track-lab/model/TrackRetiming.test.ts @@ -0,0 +1,110 @@ +/** + * Regression tests for frame-index retiming. + * + * TrackPoint.time is authoritative (captured from the video element) while + * TrackPoint.frame is a derived index that has to stay consistent with + * VideoPlaybackModel.currentFrameProperty. Changing the frame rate renumbers + * the current frame, so stored points must be renumbered in the same step. + */ + +import { beforeEach, describe, expect, it } from "vitest"; +import { TrackingModel } from "../../../src/track-lab/model/TrackingModel.js"; +import { TrackLabModel } from "../../../src/track-lab/model/TrackLabModel.js"; + +function frames(model: TrackingModel, trackId = "track-A"): number[] { + const track = model.tracksProperty.value.find((t) => t.id === trackId); + return (track?.points ?? []).map((p) => p.frame); +} + +describe("TrackingModel.retimeTrackPoints", () => { + let model: TrackingModel; + + beforeEach(() => { + model = new TrackingModel(); + model.addTrack(); + }); + + it("re-derives frame indices from timestamps", () => { + // Digitized at 30 fps: frames 0, 30, 60 at t = 0 s, 1 s, 2 s. + model.addOrReplacePointOnTrack("track-A", 0, 0, 1, 1); + model.addOrReplacePointOnTrack("track-A", 30, 1, 2, 2); + model.addOrReplacePointOnTrack("track-A", 60, 2, 3, 3); + + model.retimeTrackPoints(60); + + expect(frames(model)).toEqual([0, 60, 120]); + }); + + it("leaves timestamps and coordinates untouched", () => { + model.addOrReplacePointOnTrack("track-A", 30, 1, 4, 5); + + model.retimeTrackPoints(15); + + const point = model.tracksProperty.value[0]?.points[0]; + expect(point?.frame).toBe(15); + expect(point?.time).toBe(1); + expect(point?.x).toBe(4); + expect(point?.y).toBe(5); + }); + + it("keeps frames unique when a lower rate collides two points", () => { + // At 30 fps these are distinct frames; at 2 fps both round to frame 0. + model.addOrReplacePointOnTrack("track-A", 0, 0, 1, 1); + model.addOrReplacePointOnTrack("track-A", 3, 0.1, 2, 2); + + model.retimeTrackPoints(2); + + // First-wins, matching addPointToTrack()'s dedup policy. + expect(frames(model)).toEqual([0]); + expect(model.tracksProperty.value[0]?.points[0]?.time).toBe(0); + }); + + it("keeps points sorted ascending by frame", () => { + for (let i = 0; i < 5; i++) { + model.addOrReplacePointOnTrack("track-A", i * 30, i, i, i); + } + + model.retimeTrackPoints(24); + + const result = frames(model); + expect(result).toEqual([...result].sort((a, b) => a - b)); + }); + + it("retimes the undo stash so a restored point lands on the right frame", () => { + model.addOrReplacePointOnTrack("track-A", 30, 1, 7, 7); + model.removePointFromTrack("track-A", 30); + + model.retimeTrackPoints(60); + model.restoreLastDeletedPoint(); + + expect(frames(model)).toEqual([60]); + }); + + it("ignores a non-positive frame rate", () => { + model.addOrReplacePointOnTrack("track-A", 30, 1, 1, 1); + + model.retimeTrackPoints(0); + + expect(frames(model)).toEqual([30]); + }); +}); + +describe("TrackLabModel frame-rate wiring", () => { + it("retimes stored points so they stay aligned with the current frame", () => { + const model = new TrackLabModel(); + model.tracking.addTrack(); + + // Park the video at t = 1 s and digitize there at the default 30 fps. + model.playback.currentTimeProperty.value = 1; + const frameAtDigitize = model.playback.currentFrameProperty.value; + model.tracking.addOrReplacePointOnTrack("track-A", frameAtDigitize, 1, 5, 5); + expect(model.tracking.hasPointAtFrame("track-A", model.playback.currentFrameProperty.value)).toBe(true); + + // Switching frame rate renumbers currentFrameProperty; the stored point has + // to follow, or the erase button silently targets nothing. + model.playback.frameRateProperty.value = 60; + + expect(model.playback.currentFrameProperty.value).toBe(60); + expect(model.tracking.hasPointAtFrame("track-A", model.playback.currentFrameProperty.value)).toBe(true); + }); +}); diff --git a/tests/track-lab/model/VideoPlaybackModel.test.ts b/tests/track-lab/model/VideoPlaybackModel.test.ts new file mode 100644 index 0000000..0bcb6fa --- /dev/null +++ b/tests/track-lab/model/VideoPlaybackModel.test.ts @@ -0,0 +1,84 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { DEFAULT_FRAME_RATE, VideoPlaybackModel } from "../../../src/track-lab/model/VideoPlaybackModel.js"; + +describe("VideoPlaybackModel frame derivation", () => { + let model: VideoPlaybackModel; + + beforeEach(() => { + model = new VideoPlaybackModel(); + }); + + it("derives the current frame by rounding time x frame rate", () => { + model.currentTimeProperty.value = 1; + expect(model.currentFrameProperty.value).toBe(DEFAULT_FRAME_RATE); + }); + + it("keeps adjacent timestamps on distinct frames at 29.97 fps", () => { + model.frameRateProperty.value = 29.97; + const frameDuration = 1 / 29.97; + + // Ten consecutive frame boundaries must map to ten distinct indices; this is + // what multiplying by fps (rather than dividing by 1/fps) protects. + const derived = new Set(); + for (let i = 0; i < 10; i++) { + model.currentTimeProperty.value = i * frameDuration; + derived.add(model.currentFrameProperty.value); + } + + expect(derived.size).toBe(10); + }); + + it("agrees with timeToFrame()", () => { + model.frameRateProperty.value = 25; + model.currentTimeProperty.value = 3.44; + + expect(model.timeToFrame(3.44)).toBe(model.currentFrameProperty.value); + }); +}); + +describe("VideoPlaybackModel seeking", () => { + let model: VideoPlaybackModel; + + beforeEach(() => { + model = new VideoPlaybackModel(); + model.durationProperty.value = 10; + }); + + it("advances by exactly one frame duration", () => { + model.seekByFrames(1); + expect(model.currentTimeProperty.value).toBeCloseTo(1 / DEFAULT_FRAME_RATE); + }); + + it("clamps at the start of the video", () => { + model.seekByFrames(-1); + expect(model.currentTimeProperty.value).toBe(0); + }); + + it("clamps at the end of the video", () => { + model.currentTimeProperty.value = 10; + model.seekByFrames(1); + expect(model.currentTimeProperty.value).toBe(10); + }); + + it("does nothing when no video is loaded", () => { + const empty = new VideoPlaybackModel(); + empty.seekByFrames(1); + expect(empty.currentTimeProperty.value).toBe(0); + }); + + it("pauses playback when stepping", () => { + model.isPlayingProperty.value = true; + model.seekByFrames(1); + expect(model.isPlayingProperty.value).toBe(false); + }); + + it("seekToStart pauses and rewinds", () => { + model.currentTimeProperty.value = 5; + model.isPlayingProperty.value = true; + + model.seekToStart(); + + expect(model.currentTimeProperty.value).toBe(0); + expect(model.isPlayingProperty.value).toBe(false); + }); +}); diff --git a/vite.config.ts b/vite.config.ts index 8712c00..21c27a5 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -118,7 +118,7 @@ function serveOpenCV(): Plugin { /** * Security headers required for: - * - COOP/COEP: SharedArrayBuffer (FFmpeg WASM) + * - COOP/COEP: SharedArrayBuffer (OpenCV WASM in the tracking worker) * - CSP: restrict resource loading to same-origin + known blob/data exceptions * - X-Content-Type-Options: prevent MIME sniffing * - X-Frame-Options: prevent clickjacking (belt-and-suspenders alongside frame-ancestors) @@ -128,10 +128,10 @@ const securityHeaders: Record = { "Cross-Origin-Embedder-Policy": "require-corp", "Content-Security-Policy": [ "default-src 'self'", - // 'wasm-unsafe-eval' is required for FFmpeg/OpenCV WASM modules + // 'wasm-unsafe-eval' is required for the OpenCV WASM module // 'unsafe-eval' is required for SceneryStack query parameter parsing "script-src 'self' 'wasm-unsafe-eval' 'unsafe-eval'", - // FFmpeg and OpenCV spin up blob: workers + // OpenCV spins up blob: workers "worker-src blob: 'self'", // Inline styles are set via element.style / cssText throughout the UI layer "style-src 'self' 'unsafe-inline'",