From 06ff5a16923db946294ea5d26eef4474d2ddff63 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 00:43:47 +0000 Subject: [PATCH 1/2] fix: correct OpenCV ROI clamp and eliminate per-frame DOM rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenCVTracker: the width/height bounds of the template ROI were computed against the unclamped region.x/y values. When a drag begins outside the video edge, region.x or region.y can be negative, making `offscreen.width - region.x` exceed the canvas width and causing an OpenCV native crash. Clamp x/y first, then derive w/h from the clamped origin. DataTableNode: tracksProperty fired on every addPointToTrack call (~30 Hz during auto-tracking), each time destroying and recreating the full HTML table. Replace with an incremental strategy: - Structural changes (track added/removed, unit/colour/locale change) still trigger a full rebuild. - Data-only changes (new points on existing tracks) append only the new elements and update cells in existing rows, leaving the rest of the DOM untouched. A frameRowMap (frame → ) and maxRenderedFrame counter make both the duplication check and out-of-order detection O(1). https://claude.ai/code/session_015sLL1uRLTjrDsyMiPS13VJ --- src/screen-name/view/DataTableNode.ts | 176 +++++++++++++++++++++++--- src/tracking/OpenCVTracker.ts | 14 +- 2 files changed, 171 insertions(+), 19 deletions(-) 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/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 { From aa520c6c299491516832de8ccf3716dfa545289b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 00:52:23 +0000 Subject: [PATCH 2/2] fix: apply remaining code-review recommendations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit webcam.ts — fixWebmDuration: store the setTimeout ID and call clearTimeout() in every resolution branch (onloadedmetadata, onseeked, onerror). Previously the timeout fired after the Promise was already settled, double-revoking the blob URL and spuriously calling reject() on a resolved Promise. VideoPlayerNode.ts — track `currentBlobUrl` as a class field; revoke the previous blob URL before assigning a new one from a webcam recording, and revoke on dispose. Previously each recorded video silently leaked a blob URL for the lifetime of the browser tab. VideoPlayerNode.ts — stepForward() was an exact copy of seekByFrames(1). Collapse it to a one-liner delegate call to eliminate the duplication. DigitizingOverlayNode.ts — rebuildMarks() was called on every video frame (~30 Hz during playback) and created a fresh SceneryStack Circle node for every digitized point on every call. Replace with one reusable Path per track: rebuild only the Shape (O(n) in points) while the SceneryStack node itself is created once and updated in place. Remove the now-unused Circle import. AutoTrackerNode.ts — the per-frame duplicate-frame check scanned all existing track points with Array.some() (O(n)). Replace with a `recordedFrames: Set` field for O(1) lookup. The set is cleared in reset() and also when the active track changes (via a new lazyLink on activeTrackIdProperty, unlinked in dispose()). SimModel.ts — addPointToTrack() now rejects duplicate frame numbers at the model level. Previously only AutoTrackerNode guarded against this, so manual digitizing (clicking the same frame twice) could corrupt the kinematics by introducing two points at the same frame index. https://claude.ai/code/session_015sLL1uRLTjrDsyMiPS13VJ --- src/screen-name/model/SimModel.ts | 5 +++ src/screen-name/view/AutoTrackerNode.ts | 26 +++++++----- src/screen-name/view/DigitizingOverlayNode.ts | 42 ++++++++++++++----- src/screen-name/view/VideoPlayerNode.ts | 22 ++++++---- src/webcam.ts | 29 ++++++++----- 5 files changed, 84 insertions(+), 40 deletions(-) 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/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/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); }); }