diff --git a/scripts/video-dedupe-check.mjs b/scripts/video-dedupe-check.mjs new file mode 100644 index 0000000..a01bfb3 --- /dev/null +++ b/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); diff --git a/src/canvas/videoFrames.ts b/src/canvas/videoFrames.ts index 10eeb8a..f619dcf 100644 --- a/src/canvas/videoFrames.ts +++ b/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)); } /** diff --git a/test/canvas/canvasHelpers.test.js b/test/canvas/canvasHelpers.test.js index fff256f..98d5199 100644 --- a/test/canvas/canvasHelpers.test.js +++ b/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', () => { diff --git a/vite.config.js b/vite.config.js index de3a613..fe75c8b 100644 --- a/vite.config.js +++ b/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: {