Skip to content

fix(producer): long, mostly still films reach their first frame before the stall check stops them - #7521

Open
miguel-heygen wants to merge 6 commits into
mainfrom
perf/static-dedup-verify-once
Open

miguel-heygen wants to merge 6 commits into
mainfrom
perf/static-dedup-verify-once

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

What

Long, mostly still compositions now reach their first frame instead of being stopped as capture_disk stalled ... (stuck at 0) before it. Three changes to the static-frame dedup check (reusing a frame that is byte-identical to the one before it), one fix:

  • The probe skips the check unless a stage later reuses its session to capture. In a parallel render the probe's session never captures, so its check was pure wait.
  • The check runs once per page. The first capture session that loads a given page runs it; every later session with the same page waits for that result, then spot-checks a few short runs inside its task's frames.
  • Check work counts as activity for the stall watcher. Every check screenshot moves a per-render counter. A change restarts the watcher's silence clock without widening its window or changing the reported progress. The check is bounded by its own 300 s budget, so a check that hangs still ends, and then the normal window applies.

Why

On a long composition that is mostly still, the check takes thousands of full-size screenshots and can use its whole 300 s budget. It ran in the probe, in calibration and in every capture worker, all before the first frame, on the same machine. The stall watcher's first-frame window is 300 s at least (sized from the probe's start-up since #7517), so the check alone could outlast it, and the render was stopped with no frame captured.

Why the check needs thousands of full-size screenshots. A run of predicted-still frames is proven by capturing its anchor (the frame before the run) and comparing it byte for byte with a frame at least every 24 frames up to the run's end (STATIC_VERIFY_REFERENCE_STRIDE). Comparing against the anchor rather than the previous frame catches a slow drift that changes too little between neighbouring frames to show. So a still span of N frames costs about N / 24 full-resolution screenshots: 2,148 for the 28-minute test film below.

Checking once removes the repeated screenshots, but on its own it does not move the first frame: waiting sessions block on the checking one for up to its budget. Skipping the probe's check and counting check work as activity are what keep the render alive and start it sooner, so the three ship together.

Follow-up, not in this PR: a cheaper proof of the same property, such as comparing a hash of a downscaled capture, would cut each check's own cost. It changes what the check proves, so it needs its own review.

Related work

  • Follows the existing SubTimelineWaitMemo pattern (packages/engine/src/types.ts): one object per render, passed in the capture options of every session. The producer creates StaticDedupMemo beside it in executeRenderPipeline.
  • The probe's session reaches capture only through prepareCaptureSessionForReuse (packages/engine/src/services/frameCapture.ts), which now runs the deferred check. Searched every use of the probe session in the producer: the disk, streaming and segmented stages take it with probeSession ?? openSession() and then call prepareCaptureSessionForReuse (captureStage.ts, captureStreamingStage.ts, captureSegmentedStage.ts). Calibration opens its own session and only closes the probe's (render/captureCost.ts). renderOrchestrator.ts reads the probe page and closes it. No path captures with the probe session without prepareCaptureSessionForReuse.
  • Every capture-options builder that can run the check reports its screenshots: the probe's options (probeStage.ts, used again when its session is reused), buildCaptureOptions (workers, calibration, HDR and layered paths) and the distributed chunk's options (renderChunk.ts).
  • The watcher change sits in watchStall (packages/producer/src/services/render/observability.ts), beside the existing progress and tail handling; the per-render counter sits beside the worker-phase map in render/shared.ts.

How

  • Engine, single check: claimStaticDedupCheck gives the first session for a key the check and every later one a promise of its result. armStaticDedup settles that promise on every exit, so a waiting session never hangs. The key, staticDedupMemoKey, covers the composition HTML hash, viewport, device scale, capture mode, image format and quality, alpha, fps, hidden elements, variables, motion blur and engine version; any difference gets its own check.
  • Engine, spot check: a waiting session narrows the shared set to the frames its own timeline walk also predicts still (no screenshots), then re-verifies up to 4 runs of at most 48 frames inside its task's [startFrame, endFrame) (staticDedupFrameRange; for an interleaved task that span includes frames other workers capture, which is still the same page). Anything short of fully verified turns dedup off for that session only, and logs it once. A failed check (mismatch, infrastructure failure, unprofitable plan, or a budget stop with nothing verified) is shared too, so waiting sessions turn dedup off without a screenshot.
  • Engine, deferral: CaptureOptions.deferStaticDedup makes armStaticDedup stop after its motion-blur classification and mark the session staticDedupDeferred. prepareCaptureSessionForReuse (now async) runs the deferred check through armDeferredStaticDedup. CaptureOptions.onStaticDedupActivity is called after every check screenshot, spot checks included.
  • Producer: the probe's options set deferStaticDedup: true and report activity for the render. frameStallRule reads the per-render counter as activity. watchStall's tick moved its activity check and its trip handling into two named helpers to stay under the repo's complexity limit; behaviour is the same and the existing stall tests pass unchanged.
  • Segmented stage: the probe session is released before awaiting its reuse, since its workers open together and only one may borrow it. If that reuse fails, the borrowed session is closed.
  • Distributed chunks pass no memo and check as before; their check screenshots count as activity.

Test plan

  • fails without the fix: keeps the first-frame window open while bounded check work goes on, and stalls once it stops (observability.test.ts) fails on main: main's watcher reports capture_disk stalled: no progress for 10000ms (stuck at 0) while the check is still taking screenshots. It passes on this branch.
  • More break checks, each restored afterwards. Without the probe's activity callback, reports the deferred check's screenshots to the render's stall watcher when its session is reused fails. Without the probe's deferStaticDedup, the probe stage defer test fails. Without the call in prepareCaptureSessionForReuse, runs when the session is reused to capture fails. Without the screenshot callback, reports every check screenshot to onStaticDedupActivity fails. Letting every session check for itself fails the single-flight case; reusing without the spot check, spot-checking outside the task's range, or sharing a failed check as frames each fail their cases in frameCapture-staticDedupShare.test.ts.
  • The existing segmented-stage test gives each parallel worker its own browser and the probe session to one worker caught a race in an earlier version, where two workers took the probe session. Fixed by releasing it before the await.
  • New and changed tests 3 runs in a row. Engine frameCapture* and parallelCoordinator*: 471. Producer under vitest (renderOrchestrator*, observability): 409. Under bun: segmented and streaming stages 88, probeStage.test.ts 75, renderChunk passes. Typecheck, oxfmt --check, oxlint and the comment ratchet pass.
  • First frame on a 28-minute synthetic composition (51,558 frames, mostly still, no root data-duration, alpha, webm, 6 workers), back to back on the same machine: main 294 s and 15,036 screenshots; the single check alone 309 s and 4,329; this branch 175 s and 2,181. Probe start-up fell from 150 s to 0.9 s.
  • A check that uses its whole budget inside capture: a 28-minute composition with 5,000 tweens. With the probe starting in 1.4 s the first-frame window is its 300 s floor. The first worker's check ran 300 s inside the capture stage and frames flowed at 306 s, with no stall.
  • Output unchanged: a 60-second composition with no root data-duration, rendered to PNG. With 6 workers all 1,800 frames are byte-identical to HF_STATIC_DEDUP=false, and a temporary log showed one worker checking the final page and the other 5 reusing it with a 6-screenshot spot check each. With 1 worker, which reuses the probe session and runs the deferred check there, it is byte-identical to HF_STATIC_DEDUP=false with 1 worker.
  • Not run: the full engine and producer suites (CI runs them), a render with calibration enabled, an HDR or layered render, a distributed render.

No visible change

Output frames are unchanged (byte-equal, above). Start-up order, the dedup log lines and the time before the first frame change.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 267 (base branch 267), smooth 103 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (8)

Unstable (1)

  • seqplainkeys-none-pct-r0-nested-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo false, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes at d77c55cf.

The single-flight claim, the spot check and the memo key look correct to me. claimStaticDedupCheck → armStaticDedup settles the claim on every exit. shareableStaticFrames matches what the checking session arms for itself. The key covers the inputs that decide the pixels. My blockers are about whether this fixes the bug it is meant to fix.

1. The PR's own measurement shows the defect is not fixed. The bug is that long films are killed before their first frame because each worker repeats the still-frame check for up to five minutes. The watchdog is FIRST_FRAME_STALL_MS = 300_000 (renderOrchestrator.ts:1222, widened only by firstFrameWindowMs). The test plan reports the first frame at 294 s on main and 309 s on this branch for the 28-minute film. It also says "the waiting workers do not reach their first frame sooner". Single-flight takes the duplicated screenshots out, but each waiting worker now blocks on the checking session (up to STATIC_VERIFY_CEILING_MS, 300 s) and then runs its own spot check. So the worst-case time to first frame is the same or slightly longer, and a film near the limit is still killed. Please either:

  • add a change that actually moves the first frame, such as a wait cap on claim.check that turns dedup off and starts capturing, or starting capture before the shared result arrives, or
  • re-scope the PR as a screenshot-cost reduction and stop presenting it as the fix for the kill. Then the kill needs its own fix.

2. The "fails without the fix:" line does not name a test that fails on main for this defect. It describes four deliberately broken versions of the new code and the case groups they trip. On main, the new test file fails only because the new exports do not exist. Please name one specific test, by its test name, that reproduces the defect against main: a session reaching its first frame inside the stall window, or the check being repeated per worker.

Non-blocking:

  • staticDedupFrameRange comes from [task.startFrame, task.endFrame). For interleaved tasks (stride > 1) that range includes frames other workers capture. The spot check is still sound, since every frame belongs to the same page, but the doc comment ("the frames this session captures") is wrong for those tasks.
  • The probe almost never shares, because its compiled HTML differs from the workers' page. So for no-data-duration roots, the probe's full check still runs before the workers start their own full check, two back to back. That is worth weighing for item 1.

@miguel-heygen miguel-heygen changed the title perf(engine): check still frames once per page and share the result with later sessions fix(producer): long, mostly still films reach their first frame before the stall check stops them Nov 11, 2026
@miguel-heygen
miguel-heygen force-pushed the perf/static-dedup-verify-once branch from 3fb46b1 to d5ba63f Compare November 11, 2026 01:12

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at d5ba63f6. Both earlier blockers are resolved.

  • The fix now moves the first frame. The probe defers its check to prepareCaptureSessionForReuse, so a probe session that never captures never checks. Check screenshots count as stall-watcher activity: they restart the silence clock without widening the window or changing reported progress. Each check stays bounded by its own 300 s budget, so a hang still trips. Reported first frame: 294 s on main, 175 s here.
  • The "fails without the fix:" line names a test, and I confirmed it locally. keeps the first-frame window open while bounded check work goes on, and stalls once it stops passes on this head. With observability.ts swapped back to main's, it fails with capture_disk stalled: no progress for 10000ms (stuck at 0). frameCapture-staticDedupShare.test.ts and frameCapture-staticDedupDefer.test.ts pass (21/21). CI is green on this head.
  • The single-flight and spot-check logic is unchanged from my earlier review. The interleaved-range doc comment is fixed. The segmented stage now releases the probe session before awaiting reuse, so two workers can't take it, and it closes the session if reuse fails.

Non-blocking:

  • drawElement + probe-session reuse loses dedup. On the drawElement init path, armStaticDedup used to run at frameCapture.ts:1316 while captureMode was still "screenshot". The probe session now defers at that point. armDeferredStaticDedup runs after captureMode = "drawelement" (line 1438), so armStaticDedupFrames takes the capture_mode gate and turns dedup off. Output stays correct, but a sequential drawElement render that reuses the probe session (macOS fast-capture) no longer dedups. Consider a follow-up that runs the deferred check against the pre-injection mode, or skips deferral when the probe session will capture.
  • A side benefit: the deferred check now runs after the reuse sets onBeforeCapture, so a reused probe session with video injection correctly takes the video_injection gate. Before, it armed without an injector.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant