diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index b97e589..301f11b 100644 --- a/src/screen-name/model/SimModel.ts +++ b/src/screen-name/model/SimModel.ts @@ -440,6 +440,11 @@ export class SimModel { const tracks = this.tracksProperty.value.map((track) => { if (track.id !== id) return track; + // Reject duplicate frames. Without this guard a second click on the + // same frame (manual digitizing) or a repeated timeupdate event would + // add a second point at the same frame index, corrupting kinematics. + if (track.points.some((p) => p.frame === frame)) return track; + const point: TrackPoint = { frame, time, x, y }; const updated: Track = { ...track, points: [...track.points, point] }; return updated; diff --git a/src/screen-name/view/AutoTrackerNode.ts b/src/screen-name/view/AutoTrackerNode.ts index 4c66cf0..b667051 100644 --- a/src/screen-name/view/AutoTrackerNode.ts +++ b/src/screen-name/view/AutoTrackerNode.ts @@ -46,6 +46,8 @@ const TRAIL_DOT_RADIUS = 3; // radius of each past-position dot in the trail export class AutoTrackerNode extends Node { private readonly model: SimModel; private readonly trail: Array<{ x: number; y: number }> = []; + /** Frames already recorded to the active track; cleared on track change or reset. */ + private readonly recordedFrames = new Set(); private readonly hintText: Text; private readonly selectionRect: Rectangle; @@ -57,9 +59,10 @@ export class AutoTrackerNode extends Node { private selecting = false; private selStart = Vector2.ZERO; - // Kept for removeEventListener in dispose() + // Kept for removeEventListener / unlink in dispose() private readonly boundVideoElement: HTMLVideoElement; private readonly boundOnFrame: () => void; + private readonly boundClearRecordedFrames: () => void; public constructor( videoElement: HTMLVideoElement, @@ -222,20 +225,14 @@ export class AutoTrackerNode extends Node { const frameDuration = model.frameDurationProperty.value; const frame = Math.round(time / frameDuration); - // Avoid duplicate points for the same frame. - const activeTrack = model.tracksProperty.value.find( - (t) => t.id === activeId, - ); - const alreadyRecorded = activeTrack - ? activeTrack.points.some((p) => p.frame === frame) - : false; - - if (!alreadyRecorded) { + // O(1) duplicate-frame check via Set (vs O(n) linear scan). + if (!this.recordedFrames.has(frame)) { // Convert video-pixel coords to global coords, then to model coords. const globalPt = this.localToGlobalPoint(new Vector2(pt.x, pt.y)); const modelPt = model.modelViewTransformProperty.value.inversePosition2(globalPt); model.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y); + this.recordedFrames.add(frame); } } }; @@ -246,6 +243,13 @@ export class AutoTrackerNode extends Node { this.boundVideoElement = videoElement; this.boundOnFrame = onFrame; + // Clear the recorded-frames set whenever the user switches to a different + // track so frames from the previous track don't suppress recording on the + // new one. + const clearRecordedFrames = () => this.recordedFrames.clear(); + model.activeTrackIdProperty.lazyLink(clearRecordedFrames); + this.boundClearRecordedFrames = clearRecordedFrames; + // ── Show/hide based on combined "video loaded && autoTracking" ──────── autoTrackingShownProperty.link((shown) => { if (!shown) this.reset(); @@ -280,6 +284,7 @@ export class AutoTrackerNode extends Node { public reset(): void { this.model.tracker.dispose(); this.trail.length = 0; + this.recordedFrames.clear(); this.selecting = false; this.selectionRect.visible = false; this.trailPath.shape = null; @@ -290,6 +295,7 @@ export class AutoTrackerNode extends Node { public override dispose(): void { this.boundVideoElement.removeEventListener("timeupdate", this.boundOnFrame); this.boundVideoElement.removeEventListener("seeked", this.boundOnFrame); + this.model.activeTrackIdProperty.unlink(this.boundClearRecordedFrames); this.model.tracker.dispose(); super.dispose(); } diff --git a/src/screen-name/view/DataTableNode.ts b/src/screen-name/view/DataTableNode.ts index f912639..d88a381 100644 --- a/src/screen-name/view/DataTableNode.ts +++ b/src/screen-name/view/DataTableNode.ts @@ -291,6 +291,43 @@ function buildHTMLTable( return wrapper; } +/** + * Build a single `` for a data row. + * Used by the incremental-update path to avoid rebuilding the whole table. + */ +function buildSingleDataRow( + row: DataRow, + tracks: readonly Track[], + cellStyle: string, + isEven: boolean, + colors: TableColors, +): HTMLTableRowElement { + const tr = document.createElement("tr"); + tr.style.background = isEven ? colors.rowEven : colors.rowOdd; + + const addCell = (text: string) => { + const td = document.createElement("td"); + td.textContent = text; + td.style.cssText = cellStyle; + tr.appendChild(td); + }; + + addCell(String(row.frame)); + addCell(row.time.toFixed(CELL_DECIMAL_PLACES)); + for (const track of tracks) { + const val = row.values.get(track.id); + if (val) { + addCell(val.x.toFixed(CELL_DECIMAL_PLACES)); + addCell(val.y.toFixed(CELL_DECIMAL_PLACES)); + } else { + addCell("—"); + addCell("—"); + } + } + + return tr; +} + /** * Download icon (simple arrow pointing down). */ @@ -309,6 +346,14 @@ export class DataTableNode extends Panel { private tableWrapper: HTMLDivElement; private readonly disposeDataTable: () => void; + // ── Incremental-update state ───────────────────────────────────────────── + // Tracks whether the table needs a full structural rebuild or just new rows. + private lastTrackIds: string[] = []; + private lastUnit: string = ""; + private tableBodyRef: HTMLTableSectionElement | null = null; + private readonly frameRowMap: Map = new Map(); + private maxRenderedFrame: number = -Infinity; + public constructor( model: SimModel, videoLoadedProperty: TReadOnlyProperty, @@ -407,27 +452,123 @@ export class DataTableNode extends Panel { this.tableWrapper = tableWrapper; + // ── Full rebuild helper ────────────────────────────────────────────────── + // Replaces the entire table DOM and refreshes cached references. + const doFullRebuild = (tracks: readonly Track[], unit: string) => { + const colors = getTableColors(); + const newWrapper = buildHTMLTable(tracks, unit, colors, getLabels()); + + this.tableWrapper.innerHTML = ""; + if (newWrapper.firstChild) { + this.tableWrapper.appendChild(newWrapper.firstChild); + } + this.tableWrapper.style.cssText = newWrapper.style.cssText; + + // Cache reference and rebuild the frame→row map. + const tbody = this.tableWrapper.querySelector("tbody"); + this.tableBodyRef = tbody instanceof HTMLTableSectionElement ? tbody : null; + this.frameRowMap.clear(); + this.maxRenderedFrame = -Infinity; + + if (this.tableBodyRef) { + const dataRows = buildDataRows(tracks); + const trs = Array.from(this.tableBodyRef.querySelectorAll("tr")); + dataRows.forEach((row, i) => { + const tr = trs[i]; + if (tr instanceof HTMLTableRowElement) { + this.frameRowMap.set(row.frame, tr); + if (row.frame > this.maxRenderedFrame) { + this.maxRenderedFrame = row.frame; + } + } + }); + } + + this.lastTrackIds = tracks.map((t) => t.id); + this.lastUnit = unit; + }; + // ── Rebuild table function ─────────────────────────────────────────────── + // Performs a full rebuild on structural changes (track added/removed, unit + // or colour change) and an incremental row-append on data-only changes + // (new points added to existing tracks). During auto-tracking this fires + // ~30 times/s, so avoiding unnecessary full DOM rebuilds is critical. const rebuildTable = () => { const tracks = model.tracksProperty.value; const unit = unitProperty.value; + const trackIds = tracks.map((t) => t.id); + + const isStructural = + unit !== this.lastUnit || + trackIds.length !== this.lastTrackIds.length || + trackIds.some((id, i) => id !== this.lastTrackIds[i]); + + if (isStructural || !this.tableBodyRef) { + doFullRebuild(tracks, unit); + return; + } + + // ── Incremental path: same track structure, only new points ──────────── + const dataRows = buildDataRows(tracks); - // Build new table with current colors and labels - const newWrapper = buildHTMLTable( - tracks, - unit, - getTableColors(), - getLabels(), + // If any new row would be inserted before an already-rendered row the + // sort order of the table would break; fall back to full rebuild in that + // rare case (out-of-order manual digitizing on an earlier frame). + const hasOutOfOrder = dataRows.some( + (row) => + !this.frameRowMap.has(row.frame) && + row.frame < this.maxRenderedFrame, ); + if (hasOutOfOrder) { + doFullRebuild(tracks, unit); + return; + } - // Replace the content - this.tableWrapper.innerHTML = ""; - if (newWrapper.firstChild) { - this.tableWrapper.appendChild(newWrapper.firstChild); + const colors = getTableColors(); + const cellStyle = `padding: 3px 6px; border: 1px solid ${colors.gridStroke}; text-align: center;`; + + // Remove the "no data" placeholder row when the first real rows arrive. + if (this.frameRowMap.size === 0 && dataRows.length > 0) { + this.tableBodyRef.innerHTML = ""; } - // Copy styles - this.tableWrapper.style.cssText = newWrapper.style.cssText; + for (const row of dataRows) { + if (this.frameRowMap.has(row.frame)) { + // Update cells in an existing row (a second track filled in this frame). + const tr = this.frameRowMap.get(row.frame)!; + const cells = tr.querySelectorAll("td"); + let cellIdx = 2; // skip Frame and Time columns + for (const track of tracks) { + const val = row.values.get(track.id); + const xCell = cells[cellIdx]; + const yCell = cells[cellIdx + 1]; + if (xCell) + xCell.textContent = val + ? val.x.toFixed(CELL_DECIMAL_PLACES) + : "—"; + if (yCell) + yCell.textContent = val + ? val.y.toFixed(CELL_DECIMAL_PLACES) + : "—"; + cellIdx += 2; + } + } else { + // Append a brand-new row at the bottom. + const rowIndex = this.frameRowMap.size; // 0-based index of this row + const tr = buildSingleDataRow( + row, + tracks, + cellStyle, + rowIndex % 2 !== 0, // isEven flag: index 0 → rowOdd, index 1 → rowEven, … + colors, + ); + this.tableBodyRef.appendChild(tr); + this.frameRowMap.set(row.frame, tr); + if (row.frame > this.maxRenderedFrame) { + this.maxRenderedFrame = row.frame; + } + } + } }; // ── Reactive updates ───────────────────────────────────────────────────── @@ -437,11 +578,16 @@ export class DataTableNode extends Panel { const unitListener = () => rebuildTable(); unitProperty.link(unitListener); - // Rebuild table when color profile or locale changes - const tableHeaderBgListener = () => rebuildTable(); + // Colour profile and locale changes require a full rebuild because cell + // colours and label strings are baked into the DOM; they are not captured + // by the track-ID / unit structural-change check above. + const fullRebuild = () => + doFullRebuild(model.tracksProperty.value, unitProperty.value); + + const tableHeaderBgListener = () => fullRebuild(); TrackLabColors.tableHeaderBackgroundProperty.lazyLink(tableHeaderBgListener); - const frameStringListener = () => rebuildTable(); + const frameStringListener = () => fullRebuild(); dataTableStrings.frameStringProperty.lazyLink(frameStringListener); const videoLoadedListener = (loaded: boolean) => { diff --git a/src/screen-name/view/DigitizingOverlayNode.ts b/src/screen-name/view/DigitizingOverlayNode.ts index f1978fd..353e468 100644 --- a/src/screen-name/view/DigitizingOverlayNode.ts +++ b/src/screen-name/view/DigitizingOverlayNode.ts @@ -1,7 +1,6 @@ import { Vector2 } from "scenerystack/dot"; import { Shape } from "scenerystack/kite"; import { - Circle, DOM, FireListener, Line, @@ -260,7 +259,11 @@ export class DigitizingOverlayNode extends Node { }); // ── Mark dots layer ───────────────────────────────────────────────────── + // One Path per track is reused across rebuilds. This avoids allocating + // and discarding SceneryStack nodes on every video frame (~30 Hz during + // playback), which caused significant GC pressure with many track points. const marksLayer = new Node({ pickable: false }); + const trackPaths = new Map(); // track id → Path const rebuildMarks = () => { const frameDuration = model.frameDurationProperty.value; @@ -268,24 +271,37 @@ export class DigitizingOverlayNode extends Node { model.currentTimeProperty.value / frameDuration, ); const mvt = model.modelViewTransformProperty.value; - const circles: Circle[] = []; - for (const track of model.tracksProperty.value) { + const tracks = model.tracksProperty.value; + const activeTrackIds = new Set(tracks.map((t) => t.id)); + + // Update or create one Path per track. + for (const track of tracks) { + let path = trackPaths.get(track.id); + if (!path) { + path = new Path(null, { fill: track.color, pickable: false }); + trackPaths.set(track.id, path); + marksLayer.addChild(path); + } + const shape = new Shape(); for (const point of track.points) { if (point.frame <= currentFrame) { const localPt = mvt.transformPosition2( new Vector2(point.x, point.y), ); - circles.push( - new Circle(MARK_DOT_RADIUS, { - fill: track.color, - x: localPt.x, - y: localPt.y, - }), - ); + shape.circle(localPt.x, localPt.y, MARK_DOT_RADIUS); } } + path.shape = shape; + } + + // Remove paths for tracks that have been deleted. + for (const [id, path] of trackPaths) { + if (!activeTrackIds.has(id)) { + marksLayer.removeChild(path); + path.dispose(); + trackPaths.delete(id); + } } - marksLayer.children = circles; }; const currentTimeListener = () => rebuildMarks(); @@ -355,6 +371,10 @@ export class DigitizingOverlayNode extends Node { model.frameRateProperty.unlink(frameRateListener); model.activeTrackIdProperty.unlink(activeTrackListener); model.magnifyVideoProperty.unlink(magnifyListener); + for (const path of trackPaths.values()) { + path.dispose(); + } + trackPaths.clear(); }; } diff --git a/src/screen-name/view/VideoPlayerNode.ts b/src/screen-name/view/VideoPlayerNode.ts index bb8a438..986367d 100644 --- a/src/screen-name/view/VideoPlayerNode.ts +++ b/src/screen-name/view/VideoPlayerNode.ts @@ -16,6 +16,8 @@ export class VideoPlayerNode extends Node { public readonly webcamPanel: WebcamPanel; private readonly model: SimModel; private readonly disposeVideoPlayer: () => void; + /** Tracks the current blob URL so it can be revoked when a new one is loaded. */ + private currentBlobUrl: string | null = null; public constructor(model: SimModel, listParent: Node) { super(); @@ -117,8 +119,13 @@ export class VideoPlayerNode extends Node { (blob, duration) => { model.isPlayingProperty.value = false; autoTrackerNode.reset(); - const blobUrl = URL.createObjectURL(blob); - this.videoElement.src = blobUrl; + // Revoke the previous blob URL before creating a new one to prevent + // the browser from holding the recorded video in memory indefinitely. + if (this.currentBlobUrl) { + URL.revokeObjectURL(this.currentBlobUrl); + } + this.currentBlobUrl = URL.createObjectURL(blob); + this.videoElement.src = this.currentBlobUrl; this.videoElement.load(); if (duration > 0) { model.durationProperty.value = duration; @@ -144,6 +151,10 @@ export class VideoPlayerNode extends Node { this.videoElement.removeEventListener("durationchange", updateDuration); this.videoElement.removeEventListener("ended", onEnded); this.videoElement.removeEventListener("timeupdate", onTimeUpdate); + if (this.currentBlobUrl) { + URL.revokeObjectURL(this.currentBlobUrl); + this.currentBlobUrl = null; + } autoTrackerNode.dispose(); playbackControlsNode.dispose(); videoSourceControlNode.dispose(); @@ -158,12 +169,7 @@ export class VideoPlayerNode extends Node { /** Pause playback and advance by exactly one frame. */ public stepForward(): void { - this.model.isPlayingProperty.value = false; - const frameDuration = this.model.frameDurationProperty.value; - const raw = this.videoElement.currentTime + frameDuration; - const clamped = Math.max(0, Math.min(raw, this.videoElement.duration)); - this.videoElement.currentTime = clamped; - this.model.currentTimeProperty.value = clamped; + this.seekByFrames(1); } private seekByFrames(direction: number): void { diff --git a/src/tracking/OpenCVTracker.ts b/src/tracking/OpenCVTracker.ts index 45d7a9f..cc2f6a9 100644 --- a/src/tracking/OpenCVTracker.ts +++ b/src/tracking/OpenCVTracker.ts @@ -96,11 +96,17 @@ export class OpenCVTracker { if (this.templateMat) this.templateMat.delete(); + // Clamp the origin first, then use the clamped values when bounding the + // width and height. Without this, a negative region.x / region.y makes + // `offscreen.width - region.x` larger than the canvas, causing OpenCV to + // read outside the source image and crash. + const clampedX = Math.round(Math.max(0, region.x)); + const clampedY = Math.round(Math.max(0, region.y)); const roi = new this.cv.Rect( - Math.round(Math.max(0, region.x)), - Math.round(Math.max(0, region.y)), - Math.round(Math.min(region.w, this.offscreen.width - region.x)), - Math.round(Math.min(region.h, this.offscreen.height - region.y)), + clampedX, + clampedY, + Math.round(Math.min(region.w, this.offscreen.width - clampedX)), + Math.round(Math.min(region.h, this.offscreen.height - clampedY)), ); this.templateMat = gray.roi(roi).clone(); } finally { diff --git a/src/webcam.ts b/src/webcam.ts index 8ceb549..cca447b 100644 --- a/src/webcam.ts +++ b/src/webcam.ts @@ -72,12 +72,25 @@ export function fixWebmDuration( const blobUrl = URL.createObjectURL(blob); video.src = blobUrl; + // Shared cleanup: cancel the timeout and revoke the blob URL. + // Called exactly once from whichever path settles the promise first. + const cleanup = (timeoutId: ReturnType) => { + clearTimeout(timeoutId); + URL.revokeObjectURL(blobUrl); + }; + + // Start the timeout *before* assigning event handlers so the ID is + // available inside the handlers through the closure. + const timeoutId = setTimeout(() => { + URL.revokeObjectURL(blobUrl); + reject(new Error("Timeout while fixing video duration")); + }, 10000); + video.onloadedmetadata = () => { // If duration is already valid, return immediately if (Number.isFinite(video.duration) && video.duration > 0) { - const duration = video.duration; - URL.revokeObjectURL(blobUrl); - resolve({ blob, duration }); + cleanup(timeoutId); + resolve({ blob, duration: video.duration }); return; } @@ -89,7 +102,7 @@ export function fixWebmDuration( // Now duration should be available const duration = video.duration; video.currentTime = 0; // Reset to beginning - URL.revokeObjectURL(blobUrl); + cleanup(timeoutId); if (Number.isFinite(duration) && duration > 0) { resolve({ blob, duration }); @@ -99,15 +112,9 @@ export function fixWebmDuration( }; video.onerror = () => { - URL.revokeObjectURL(blobUrl); + cleanup(timeoutId); reject(new Error("Failed to load video for duration fix")); }; - - // Timeout after 10 seconds - setTimeout(() => { - URL.revokeObjectURL(blobUrl); - reject(new Error("Timeout while fixing video duration")); - }, 10000); }); }