From df221c1fd685862b62e3057438203f79eedae111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 3 Jul 2026 12:30:38 -0700 Subject: [PATCH] fix(engine): scale static-dedup verification density with run length (#1903) * fix(engine): scale static-dedup verification density with run length Reported symptom: a 10-scene template composition (shared card layout, per-scene text/progress-bar content) rendered scene 1 correctly, but every scene after that had its text/progress-bar card missing from the final MP4 -- even though snapshot and validate showed correct per-scene content when seeking directly to those timestamps. Setting HF_STATIC_DEDUP=false fixed every scene. Render log showed a large, mostly-reusable static-frame run engaging (2430 frames, 34% reusable). verifyStaticFramesSafe already does a real, pixel-exact comparison (anchor vs. candidate screenshot) before trusting a predicted-static run -- the reuse mechanism itself is correct and already regression- locked (frameCapture-staticDedupIndex.test.ts). The gap was sample density: interior checks per run were capped at a flat min(sampleCount, 8) points, so the stride between checks grew with the run's span. A 2000+ frame run (plausible for a 10-scene comp where computeStaticFrameSet's GSAP-tween-only interval walk can't see whatever mechanism swaps each scene's text) could space checks ~285 frames apart, letting a real content change hide between two verified points and get the whole run wrongly trusted as static. Fix: extract the point-selection into a pure, exported computeStaticVerificationPoints(a, b, sampleCount), and bound the STRIDE by sampleCount (HF_STATIC_DEDUP_SAMPLES) instead of just the point count, so density scales with run length. Short/typical runs are unaffected (the two formulas agree there); long runs get proportionally denser checks. The existing hardCap safety valve is untouched -- if this makes verification too expensive for a pathological composition, dedup still disarms entirely rather than trusting a sparsely-checked set. Test: new frameCapture-staticDedupVerifyDensity.test.ts asserts the max gap between consecutive verification points never exceeds sampleCount on long runs (would fail pre-fix at span=2000/10000), matches the prior stride on short runs, and always includes both run endpoints. Full engine suite (845 tests) passes. * fix(engine): decouple verification density scaling from sampleCount polarity Addresses review feedback on the static-dedup density fix (PR #1903): 1. The prior revision bounded the interior-check STRIDE by sampleCount directly, which inverted HF_STATIC_DEDUP_SAMPLES' polarity: raising it widened the allowed gap between checks instead of narrowing it, and the "raise HF_STATIC_DEDUP_SAMPLES to verify more" log guidance became backwards for exactly the long runs it's meant to help. Fix: introduce a fixed STATIC_VERIFY_REFERENCE_STRIDE (24 frames, independent of sampleCount) that drives the length-scaling behavior -- this alone fixes the original bug (long runs going nearly unverified) regardless of how sampleCount is configured. sampleCount is now purely a per-run point-count FLOOR: raising it only ever increases density, restoring correct, monotonic polarity. 2. hardCap wasn't re-tuned for the new cost model. The old flat 8-point cap cost ~8 checks/run; the new density costs ~span/24 checks/run -- ~103 for the reported 2430-frame run, ~417 for a 10k-frame run. Sizing the budget only off sampleCount (which no longer drives density for long runs) would make a genuinely-static long composition spuriously disarm under the new, more thorough checking. hardCap now also scales with the total predicted-static frame count, with a 3x margin over the expected minimum verification cost. Softened the budget-exhausted log message accordingly -- it no longer prescribes raising sampleCount, which would often just add cost without proportionally raising the now length-driven budget. 3. The 5 existing tests only asserted sample-point geometry (gaps, endpoints, stride shape), not the actual point of the fix -- that a real content change hiding between the OLD sample gaps now gets caught. Added a behavior-level test: mocks pageScreenshotCapture to simulate a transient content change at a frame the pre-fix formula would have skipped (reconstructed locally in the test, commented as historical-only) but the new formula samples, and asserts the real verifyStaticFramesSafe (now exported) detects it via the real computeStaticVerificationPoints -- not a reimplementation. Also added a direct polarity regression test (raising sampleCount past the length-scaled floor must strictly tighten the gap) and reworded the short-run test to reflect the corrected formula. Full engine suite (847 tests) passes. --- ...meCapture-staticDedupVerifyDensity.test.ts | 150 ++++++++++++++++++ packages/engine/src/services/frameCapture.ts | 82 ++++++++-- 2 files changed, 215 insertions(+), 17 deletions(-) create mode 100644 packages/engine/src/services/frameCapture-staticDedupVerifyDensity.test.ts diff --git a/packages/engine/src/services/frameCapture-staticDedupVerifyDensity.test.ts b/packages/engine/src/services/frameCapture-staticDedupVerifyDensity.test.ts new file mode 100644 index 000000000..ea0c7c54c --- /dev/null +++ b/packages/engine/src/services/frameCapture-staticDedupVerifyDensity.test.ts @@ -0,0 +1,150 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + computeStaticVerificationPoints, + verifyStaticFramesSafe, + type CaptureSession, +} from "./frameCapture.js"; +import { pageScreenshotCapture } from "./screenshotService.js"; + +vi.mock("./screenshotService.js", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, pageScreenshotCapture: vi.fn() }; +}); + +/** + * Regression lock for static-dedup verification sample density. + * + * The prior formula capped points-per-run at a flat `min(sampleCount, 8)`, + * so the stride between checks grew linearly with the run's span — a run of + * a few thousand frames could end up with checks hundreds of frames apart. + * A genuine content change hiding between two such checks (e.g. text + * swapped by a mechanism the GSAP tween walk in computeStaticFrameSet can't + * see) would never get sampled, and the run would be wrongly trusted as + * static. + * + * A first version of this fix bounded the STRIDE by `sampleCount` directly — + * which fixed the density but inverted the config knob's polarity: raising + * `HF_STATIC_DEDUP_SAMPLES` widened the allowed gap instead of shrinking it. + * The current formula uses a fixed internal reference stride (independent of + * sampleCount) for the length-scaling fix, and sampleCount as a pure + * point-count floor that only ever increases density. + */ +describe("computeStaticVerificationPoints", () => { + const REFERENCE_STRIDE = 24; // matches STATIC_VERIFY_REFERENCE_STRIDE in frameCapture.ts + + function maxGap(points: number[]): number { + let max = 0; + for (let i = 1; i < points.length; i++) max = Math.max(max, points[i] - points[i - 1]); + return max; + } + + it("never leaves a gap wider than the reference stride on a long run, even with a low sampleCount", () => { + // Pre-fix (flat 8-point cap): stride = floor(2000/7) = 285. Using a LOW + // sampleCount (5) here proves the length-scaling fix is independent of the + // user's sampleCount setting, not just true when sampleCount happens to be large. + const points = computeStaticVerificationPoints(0, 2000, 5); + expect(maxGap(points)).toBeLessThanOrEqual(REFERENCE_STRIDE); + }); + + it("scales point count up further for an even longer run", () => { + const points = computeStaticVerificationPoints(0, 10_000, 24); + expect(maxGap(points)).toBeLessThanOrEqual(REFERENCE_STRIDE); + expect(points.length).toBeGreaterThan(400); + }); + + it("raising sampleCount only ever increases density (never decreases it)", () => { + // A prior version of this fix used sampleCount as a stride CAP, so raising + // it widened the allowed gap instead of narrowing it. Once sampleCount + // exceeds the length-scaled floor, it must now visibly tighten the gap. + const lowSample = computeStaticVerificationPoints(0, 2000, 24); + const highSample = computeStaticVerificationPoints(0, 2000, 200); + expect(maxGap(highSample)).toBeLessThan(maxGap(lowSample)); + }); + + it("sampleCount still governs density on short runs where the length-scaled floor is small", () => { + const points = computeStaticVerificationPoints(100, 150, 24); + expect(points[0]).toBe(100); + expect(points[points.length - 1]).toBe(150); + // perRun = max(3, 24, ceil(50/24)+1=4) = 24 → stride = floor(50/23) = 2. + expect(maxGap(points)).toBeLessThanOrEqual(2); + }); + + it("always includes the run's start and end", () => { + const points = computeStaticVerificationPoints(500, 500 + 3333, 24); + expect(points[0]).toBe(500); + expect(points[points.length - 1]).toBe(500 + 3333); + }); + + it("handles a single-frame run without dividing by zero", () => { + const points = computeStaticVerificationPoints(42, 42, 24); + expect(points).toEqual([42]); + }); +}); + +/** + * Behavior-level lock: a real content change that reverts before the run's end + * (so the always-checked endpoint alone would NOT reveal it) must still be + * caught once it falls within the new, denser sample spacing — even though the + * old flat 8-point-per-run density would have skipped straight past it. + */ +describe("verifyStaticFramesSafe catches drift the old fixed-point density would miss", () => { + const fps = 30; + + function oldFormulaPoints(a: number, b: number, sampleCount: number): number[] { + const perRun = Math.max(3, Math.min(sampleCount, 8)); + const span = b - a; + const stride = span > 0 ? Math.max(1, Math.floor(span / (perRun - 1))) : 1; + const pts = new Set(); + for (let f = a; f <= b; f += stride) pts.add(f); + pts.add(b); + return [...pts].sort((x, y) => x - y); + } + + beforeEach(() => { + vi.mocked(pageScreenshotCapture).mockReset(); + }); + + it("flags a transient content change hidden between the old sample gaps", async () => { + const a = 1; + const b = 2000; + const sampleCount = 24; + + // Pick a frame the OLD formula would have skipped but the NEW one samples, + // and confirm the endpoint alone (checked either way) would NOT reveal it — + // isolating the assertion to interior-sample density, not the end-of-run check. + const oldPoints = new Set(oldFormulaPoints(a, b, sampleCount)); + const newPoints = computeStaticVerificationPoints(a, b, sampleCount); + const changeAt = newPoints.find((f) => !oldPoints.has(f) && f !== a && f !== b); + if (changeAt === undefined) throw new Error("test setup: no frame differs between formulas"); + + // Content is "before" everywhere except a single transient frame that reverts + // immediately after — the anchor (a-1) and the run's end (b) both read "before". + const contentAt = (f: number) => (f === changeAt ? "glitch" : "before"); + + let lastFrameIdx = 0; + const page = { + evaluate: vi.fn(async (_fn: unknown, t: number) => { + lastFrameIdx = Math.round(t * fps); + }), + }; + vi.mocked(pageScreenshotCapture).mockImplementation(async () => + Buffer.from(contentAt(lastFrameIdx)), + ); + + const staticFrames = new Set(); + for (let f = a; f <= b; f++) staticFrames.add(f); + + const session = { options: {} } as unknown as CaptureSession; + const result = await verifyStaticFramesSafe( + session, + page as unknown as Parameters[1], + staticFrames, + fps, + sampleCount, + ); + + expect(result).not.toBeNull(); + expect(result?.budgetExhausted).toBe(false); + expect(result?.badFrame).toBe(changeAt); + }); +}); diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index 136c377fb..ea1c4e46a 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -1525,6 +1525,49 @@ async function computeStaticFrameSet( }; } +// Fixed density target for verification checks: never leave a gap wider than this +// many frames within a run, independent of the user-tunable sampleCount. This is +// what fixes long runs going nearly unverified — deliberately NOT derived from +// sampleCount, so that knob's effect on density stays monotonic (see below). +const STATIC_VERIFY_REFERENCE_STRIDE = 24; + +/** + * Interior verification points for a run [a..b], plus the always-included end `b`. + * Density used to be a flat point-count cap (min(sampleCount, 8)), so a run's + * stride grew with its span — on a long run (many merged static frames), two + * checks could land hundreds of frames apart. A genuine content change in + * between (e.g. text swapped by a mechanism computeStaticFrameSet's GSAP-only + * tween walk can't see) then hides between samples and the whole run gets + * wrongly trusted as static. + * + * `sampleCount` (HF_STATIC_DEDUP_SAMPLES) is a per-run point-count FLOOR, not a + * stride cap — raising it always increases density, never decreases it. (An + * earlier revision of this fix bounded the stride BY sampleCount directly, which + * inverted that: raising sampleCount widened the allowed gap instead of shrinking + * it, and the "raise HF_STATIC_DEDUP_SAMPLES to verify more" log guidance became + * backwards for exactly the long runs it's meant to help.) The length-scaling + * fix itself comes from STATIC_VERIFY_REFERENCE_STRIDE, which is independent of + * sampleCount, so density scales with run length regardless of how that knob is + * set; sampleCount only ever raises density further above that floor. + * + * Pure and exported so its scaling behavior is unit-testable without a real + * page/browser. + */ +export function computeStaticVerificationPoints( + a: number, + b: number, + sampleCount: number, +): number[] { + const span = b - a; + const lengthScaledPoints = span > 0 ? Math.ceil(span / STATIC_VERIFY_REFERENCE_STRIDE) + 1 : 1; + const perRun = Math.max(3, sampleCount, lengthScaledPoints); + const stride = span > 0 ? Math.max(1, Math.floor(span / (perRun - 1))) : 1; + const pts = new Set(); + for (let f = a; f <= b; f += stride) pts.add(f); + pts.add(b); + return [...pts].sort((x, y) => x - y); +} + /** * Empirically verify the predicted-static set before trusting it. Group static frames * into runs; each run [a..b] reuses anchor a-1. CRITICAL: compare against the ANCHOR, @@ -1534,7 +1577,7 @@ async function computeStaticFrameSet( * mismatch ⇒ the run isn't truly static ⇒ disable dedup whole-comp. Capture-mode- * independent (seeks + screenshots in normal DOM). Returns the first bad frame, or null. */ -async function verifyStaticFramesSafe( +export async function verifyStaticFramesSafe( session: CaptureSession, page: Page, staticFrames: Set, @@ -1561,31 +1604,36 @@ async function verifyStaticFramesSafe( }; // Verify EVERY run in order (no longest-first truncation that would leave runs armed // but unverified). Per run, compare the FIRST reused frame `a`, the END `b` (max - // accumulated drift), and interior points at a stride — against the anchor the run - // actually reuses. `sampleCount` sets the interior density (points per run ~ that many - // for a long run); a hard cap bounds pathological run counts, and hitting it DISABLES - // dedup (conservative: never trust an unverified set). - const perRun = Math.max(3, Math.min(sampleCount, 8)); - const hardCap = Math.max(sampleCount * 8, 400); + // accumulated drift), and interior points at a stride (see computeStaticVerificationPoints) + // — against the anchor the run actually reuses. + // + // hardCap bounds pathological cases and hitting it DISABLES dedup (conservative: + // never trust an unverified set). It must scale with the new density model: + // each run now costs roughly span/STATIC_VERIFY_REFERENCE_STRIDE + 1 checks (plus + // one anchor), not the ~8 the old flat point cap cost — sizing the budget only off + // sampleCount (which no longer drives density for long runs) would make a + // genuinely-static long composition spuriously disarm under the new, more + // thorough checking. `frames.length` approximates total interior checks; a 3x + // margin absorbs per-run anchor overhead and the 3-point floor on short runs. + const hardCap = Math.max( + sampleCount * 8, + 400, + Math.ceil(frames.length / STATIC_VERIFY_REFERENCE_STRIDE) * 3 + runs.length, + ); let spent = 0; for (const { a, b } of runs) { const anchor = a - 1; if (anchor < 0) continue; const anchorBuf = await seekCapture(anchor); spent++; - const span = b - a; - const stride = span > 0 ? Math.max(1, Math.floor(span / (perRun - 1))) : 1; - const pts = new Set(); - for (let f = a; f <= b; f += stride) pts.add(f); - pts.add(b); // always include the end (max drift) - for (const f of [...pts].sort((x, y) => x - y)) { + for (const f of computeStaticVerificationPoints(a, b, sampleCount)) { const cur = await seekCapture(f); spent++; if (!anchorBuf.equals(cur)) return { badFrame: f, budgetExhausted: false }; } - // Budget exhausted → can't fully verify → disarm. Reported distinctly from real - // drift so a `verification_budget` spike in telemetry signals "tune HF_STATIC_DEDUP_SAMPLES", - // not "compositions are non-static". + // Budget exhausted → can't fully verify → disarm, distinct from real drift so a + // `verification_budget` spike in telemetry reads as "this composition has a lot + // of static material to verify," not "compositions are non-static." if (spent > hardCap) return { badFrame: a, budgetExhausted: true }; } return null; @@ -1664,7 +1712,7 @@ async function armStaticDedup( logInitPhase( verdict.budgetExhausted ? `static-frame dedup: disabled (verification budget exhausted before frame ${verdict.badFrame}; ` + - `raise HF_STATIC_DEDUP_SAMPLES to verify more)` + `too much predicted-static material to fully verify — this is the safe fallback, not an error)` : `static-frame dedup: disabled (verification failed — content drifts from anchor at ` + `predicted-static frame ${verdict.badFrame})`, );