Repository navigation
fix(producer): long, mostly still films reach their first frame before the stall check stops them - #7521
fix(producer): long, mostly still films reach their first frame before the stall check stops them#7521miguel-heygen wants to merge 6 commits into
Conversation
Edit accuracy: accurate 267 (base branch 267), smooth 103 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
There was a problem hiding this comment.
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.checkthat 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:
staticDedupFrameRangecomes 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-durationroots, 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.
…ith later sessions
…ck files at their base
…as stall-watcher activity
…t only its own frames
3fb46b1 to
d5ba63f
Compare
There was a problem hiding this comment.
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 stopspasses on this head. Withobservability.tsswapped back to main's, it fails withcapture_disk stalled: no progress for 10000ms (stuck at 0).frameCapture-staticDedupShare.test.tsandframeCapture-staticDedupDefer.test.tspass (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,
armStaticDedupused to run atframeCapture.ts:1316whilecaptureModewas still"screenshot". The probe session now defers at that point.armDeferredStaticDedupruns aftercaptureMode = "drawelement"(line 1438), soarmStaticDedupFramestakes thecapture_modegate 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 thevideo_injectiongate. Before, it armed without an injector.
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: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
SubTimelineWaitMemopattern (packages/engine/src/types.ts): one object per render, passed in the capture options of every session. The producer createsStaticDedupMemobeside it inexecuteRenderPipeline.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 withprobeSession ?? openSession()and then callprepareCaptureSessionForReuse(captureStage.ts,captureStreamingStage.ts,captureSegmentedStage.ts). Calibration opens its own session and only closes the probe's (render/captureCost.ts).renderOrchestrator.tsreads the probe page and closes it. No path captures with the probe session withoutprepareCaptureSessionForReuse.probeStage.ts, used again when its session is reused),buildCaptureOptions(workers, calibration, HDR and layered paths) and the distributed chunk's options (renderChunk.ts).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 inrender/shared.ts.How
claimStaticDedupCheckgives the first session for a key the check and every later one a promise of its result.armStaticDedupsettles 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.[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.CaptureOptions.deferStaticDedupmakesarmStaticDedupstop after its motion-blur classification and mark the sessionstaticDedupDeferred.prepareCaptureSessionForReuse(now async) runs the deferred check througharmDeferredStaticDedup.CaptureOptions.onStaticDedupActivityis called after every check screenshot, spot checks included.deferStaticDedup: trueand report activity for the render.frameStallRulereads the per-render counter asactivity.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.Test plan
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 reportscapture_disk stalled: no progress for 10000ms (stuck at 0)while the check is still taking screenshots. It passes on this branch.reports the deferred check's screenshots to the render's stall watcher when its session is reusedfails. Without the probe'sdeferStaticDedup, the probe stage defer test fails. Without the call inprepareCaptureSessionForReuse,runs when the session is reused to capturefails. Without the screenshot callback,reports every check screenshot to onStaticDedupActivityfails. 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 inframeCapture-staticDedupShare.test.ts.gives each parallel worker its own browser and the probe session to one workercaught a race in an earlier version, where two workers took the probe session. Fixed by releasing it before the await.frameCapture*andparallelCoordinator*: 471. Producer under vitest (renderOrchestrator*,observability): 409. Under bun: segmented and streaming stages 88,probeStage.test.ts75,renderChunkpasses. Typecheck,oxfmt --check,oxlintand the comment ratchet pass.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.data-duration, rendered to PNG. With 6 workers all 1,800 frames are byte-identical toHF_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 toHF_STATIC_DEDUP=falsewith 1 worker.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.