fix: seek off the frame boundary, so the importer stops losing frames (#124)

scripts/video-dedupe-check.mjs

@@ -0,0 +1,81 @@
+// Does the video importer keep every frame it should?
+//
+// #124 sat open for months as "the exact-duplicate dedupe looks slightly off", with no way to
+// say which direction it erred in - the reporter had a video, the repository did not, and a
+// threshold can be wrong in two opposite ways. This builds its own video instead: one second at
+// 30fps where every single frame is a different colour. In `exact` mode nothing in it can
+// legitimately be merged, so `skipped` must be 0 at any import rate, and anything else is a bug
+// with a number on it.
+//
+// That is how the cause was found. Importing at 30fps kept 21 of 30 and merged 9, in the
+// giveaway pattern holds=[1,2,1,2,...]; at 6 and 15fps nothing merged, which is why it only ever
+// looked "slightly" off. The signature was innocent - the merged captures really were
+// byte-identical, because seeking to exactly a frame boundary returned the frame before it.
+//
+//   node --experimental-strip-types --no-warnings=ExperimentalWarning scripts/video-dedupe-check.mjs
+//
+// Needs ffmpeg on PATH and the dev server running (npm run dev).
+
+import { chromium } from 'playwright-core';
+import { execFileSync } from 'node:child_process';
+import { mkdtempSync, readFileSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+
+const ORIGIN = process.env.MV_ORIGIN || 'http://127.0.0.1:5175/';
+const FRAMES = 30;
+
+/** A clip whose every frame is a different colour, so any merge at all is a wrong answer. */
+function buildClip(dir) {
+    const out = join(dir, 'distinct.mp4');
+    execFileSync('ffmpeg', [
+        '-y', '-loglevel', 'error',
+        '-f', 'lavfi', '-i', `color=c=black:s=320x240:r=${FRAMES}:d=1`,
+        '-vf', "geq=r='mod(N*37+10,256)':g='mod(N*91+20,256)':b='mod(N*53+30,256)'",
+        '-frames:v', String(FRAMES),
+        // Lossless, so two frames can never become identical through compression - which would
+        // make a merge correct and the test meaningless.
+        '-c:v', 'libx264', '-qp', '0', '-pix_fmt', 'yuv444p', out,
+    ]);
+    return out;
+}
+
+const dir = mkdtempSync(join(tmpdir(), 'mv-dedupe-'));
+let bad = 0;
+try {
+    const clip = readFileSync(buildClip(dir)).toString('base64');
+    const browser = await chromium.launch({ channel: 'chrome' });
+    try {
+        const page = await browser.newPage();
+        await page.goto(ORIGIN);
+        await page.waitForLoadState('networkidle');
+        const rows = await page.evaluate(async (b64) => {
+            const bin = Uint8Array.from(atob(b64), c => c.charCodeAt(0));
+            const file = new Blob([bin], { type: 'video/mp4' });
+            const mod = await import('/src/canvas/videoFrames.ts');
+            const out = [];
+            for (const fps of [30, 15, 6]) {
+                const t0 = performance.now();
+                const r = await mod.extractVideoFrames(file, { fps, dedupe: 'exact', scale: 1, format: 'png' });
+                out.push({ fps, kept: r.frames.length, skipped: r.skipped, holds: r.holds.join(','), ms: Math.round(performance.now() - t0) });
+            }
+            return out;
+        }, clip);
+
+        console.log(`A one-second ${FRAMES}fps clip whose frames are all different colours.`);
+        console.log('In "exact" mode nothing in it can be merged, so skipped must be 0.\n');
+        for (const r of rows) {
+            const ok = r.skipped === 0;
+            if (!ok) bad++;
+            console.log(`  ${ok ? 'ok  ' : 'BAD '} fps=${String(r.fps).padStart(2)}  kept=${String(r.kept).padStart(2)}  skipped=${r.skipped}  ${String(r.ms).padStart(4)}ms  holds=[${r.holds}]`);
+        }
+        console.log(bad
+            ? `\n${bad} import rate(s) merged frames that differ. A holds pattern of [1,2,1,2,...] is the\nseek landing on a frame boundary and returning the frame before it - see seekTarget.`
+            : '\nEvery frame survived at every rate.');
+    } finally {
+        await browser.close();
+    }
+} finally {
+    rmSync(dir, { recursive: true, force: true });
+}
+process.exit(bad ? 1 : 0);

src/canvas/videoFrames.ts

@@ -28,13 +28,36 @@ export function fitRect(sw: number, sh: number, dw: number, dh: number): Rect {
  * with no error. The scene detector had learnt this and clamped; the frame extractor had not,
  * and it steps right up to the end of the range it was given.
  *
+ * And never exactly on a frame boundary, which is #124. Asking for time t when a frame starts
+ * at t is ambiguous - the frame ending there also touches it - and the browser may hand back
+ * the one before. The extractor steps by 1/fps, so at a frame rate near the video's own, every
+ * other seek came back as the previous picture. Those captures really are byte-identical, so
+ * the exact-duplicate dedupe merged them, and the import quietly lost frames.
+ *
+ * Measured on a one-second 30fps clip whose 30 frames are all different colours, seeking to
+ * each frame time and reading the pixels back:
+ *
+ *     offset 0        21 of 30 distinct, 9 consecutive repeats
+ *     offset 0.0001   30 of 30 distinct, 0 repeats
+ *     offset 1/60     30 of 30 distinct, 0 repeats
+ *
+ * A millisecond is far below any real frame interval - 8.3ms even at 120fps - so the nudge
+ * cannot reach the next frame, and far above the float error in `start + i * step`. It is
+ * applied before the end clamp so it can never push a seek past the point that hangs.
+ *
+ * Waiting longer was the obvious other theory and it is wrong: with the nudge, `seeked` alone
+ * is exact, and waiting on requestVideoFrameCallback instead cost 16x the time (133ms to
+ * 2123ms over 30 seeks) while fixing nothing.
+ *
  * @param {number} t
  * @param {number} duration
  * @returns {number}
  */
+const SEEK_NUDGE = 0.001;
+
 export function seekTarget(t: number, duration: number): number {
     if (!Number.isFinite(duration) || duration <= 0) return 0;
-    return Math.max(0, Math.min(t, duration - 0.02));
+    return Math.max(0, Math.min(t + SEEK_NUDGE, duration - 0.02));
 }
 
 /**

test/canvas/canvasHelpers.test.js

@@ -452,10 +452,33 @@ test('a seek never lands on the very last frame', () => {
     assert.equal(seekTarget(99, 10), 9.98, 'past the end clamps too');
 });
 
-test('a seek inside the video is left alone', () => {
-    assert.equal(seekTarget(0, 10), 0);
-    assert.equal(seekTarget(3.5, 10), 3.5);
-    assert.equal(seekTarget(9.97, 10), 9.97);
+test('a seek is nudged off the frame boundary it was asked for', () => {
+    // #124. Asking for time t when a frame *starts* at t is ambiguous - the frame ending there
+    // touches it too - and the browser may hand back the one before. The extractor steps by
+    // 1/fps, so importing at a rate near the video's own made every other seek return the
+    // previous picture; those captures are byte-identical, so the exact-duplicate dedupe merged
+    // them and the import quietly lost frames. Measured on a 30fps clip of 30 different
+    // colours: seeking on the boundary gave 21 distinct frames and 9 repeats, nudged gave 30
+    // and 0.
+    assert.equal(seekTarget(3.5, 10), 3.501);
+    assert.equal(seekTarget(0, 10), 0.001);
+});
+
+test('the nudge is far smaller than a frame and far larger than float error', () => {
+    // It must not be able to reach into the next frame - 1/120s is 8.3ms, the shortest interval
+    // worth worrying about - and must still clear the error in `start + i * step`.
+    const nudge = seekTarget(1, 10) - 1;
+    assert.ok(nudge > 0, 'the nudge has to move the seek at all');
+    assert.ok(nudge < 1 / 120, `a nudge of ${nudge}s could cross a 120fps frame`);
+    assert.ok(nudge > 1e-6, 'a nudge below float error would not reliably clear the boundary');
+});
+
+test('the nudge can never push a seek past the point that hangs', () => {
+    // The end clamp exists because seeking to the duration fires no `seeked` in some browsers.
+    // Applying the nudge after it would walk straight back into that.
+    assert.equal(seekTarget(9.98, 10), 9.98);
+    assert.equal(seekTarget(9.9799, 10), 9.98);
+    assert.ok(seekTarget(9.999, 10) <= 9.98);
 });
 
 test('a negative time becomes the start', () => {

vite.config.js

@@ -17,8 +17,13 @@ export default defineConfig({
   plugins: [react(), qrcode()],
   server: {
     host: true, // expose on LAN so a tablet on the same Wi-Fi can connect
-    port: 5173,
-    strictPort: false,
+    // 5175, not Vite's default: another project on this machine holds 5173 permanently. The two
+    // did not collide loudly, which was the problem - that one listens on ::1 and this one on
+    // ::, `localhost` resolves to ::1 first, and so the browser quietly got the *other* app on
+    // the address this one had just printed. strictPort turns the next such clash into a
+    // refusal to start, which is the one outcome that cannot be mistaken for working.
+    port: 5175,
+    strictPort: true,
     proxy: API_PROXY,
   },
   build: {