Repository navigation
fix(engine): a PNG-sequence frame no longer captures DOM still pending in virtual rAF - #6007
santhiprakash wants to merge 2 commits into
Conversation
…g in virtual rAF The virtual-time shim flushed the rAF queue once per seek, so a callback that re-registers itself (a deferred reveal chained through several rAF turns) advanced one turn per rendered frame and early frames of each worker captured a pre-settled DOM. seekToTime now drains the queue to quiescence at the authored virtual time, bounded by turn and total-execution caps so perpetual rAF loops stay bounded too. The capture path then runs a post-seek settle barrier before the screenshot: it re-drains callbacks queued by post-seek hooks, forces a reflow so font/image loads start, waits for document.fonts and image decodes, and — only when the compositor free-runs and the settle actually observed pending work — commits two native paint frames through the shim's stored originalRequestAnimationFrame. All waits race one shared wall-clock budget (HF_POST_SEEK_SETTLE_MS, default 1500ms; 0 disables). Fixes heygen-com#5888
…barrier CI measured the engine tarball at 428660 bytes against the 428347 budget after the settle barrier landed (new runtime code, not packaging drift; the tarball ships only dist/). New budget is the check's own 5% headroom convention over the measured size; the unpacked measurement stays under its budget and is unchanged.
|
CI note on the first run (failures are on 924303c; current head a809a0e is all green):
|
miguel-heygen
left a comment
There was a problem hiding this comment.
Thank you for the depth here, the repro, the sabotage check and the before and after frame counts made this very easy to read.
My blocker is the semantics change in seekToTime. Draining to quiescence means a callback that re-registers itself runs up to 64 times per captured frame instead of once, and virtual time is frozen for all 64, so any loop that mutates state per callback rather than from the timestamp renders wrong. I lifted both versions of the shim and ran a plain function tick(){ rotation += 1; requestAnimationFrame(tick); } for 24 frames: rotation is 24 on main and 1536 on this branch. Your own test asserts the 64, so I read this as a deliberate trade rather than an oversight, but it contradicts the contract the shim documents at the top of fileServer.ts, and nothing warns when it bites.
Could the drain stay in the capture path only, where settlePostSeekDomUpdates already does one, or stop as soon as a turn re-registers the same callback? That keeps the finite reveal chain in #5888 fixed without touching perpetual loops.
Two smaller things: the branch now conflicts with main, including the size budgets file which has been reformatted there. And settlePostSeekDomUpdates forces a synchronous layout and walks every img on every frame of every render, not just png sequences, so I would like a number on something longer than the 48 frame repro before it goes in.
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes. #5888 is real and still open, and the repro work here is excellent, but the core change alters what every perpetual rAF loop does at render time. Nothing has been pushed since Miguel's change request, so his blocker stands.
I checked the drain at fileServer.ts line 316 onward on this head. seekToTime now runs the queue up to 64 turns at the same frozen virtual time. A finite reveal chain settles, which is the fix. But a loop that re-registers itself every frame (function tick(){ rotation += 1; requestAnimationFrame(tick) }, or any frame counter) now runs 64 times per captured frame instead of once. Miguel measured rotation 24 on main vs 1536 here over 24 frames. That breaks the one-turn-per-seek contract the shim documents, silently.
A narrower shape keeps #5888 fixed without touching perpetual loops: drain only in the capture path (where settlePostSeekDomUpdates already re-drains), or stop draining as soon as a turn re-registers a callback that already ran this seek.
Also needed:
- Rebase.
scripts/package-size-budgets.jsonconflicts with main, which reformatted that file. settlePostSeekDomUpdatesforces a layout and walks everyimgon every frame of every render, not only png sequences. A timing number on a longer render than the 48-frame repro would help before this goes in.
Need: yes, #5888 is open and reproduces · Size: large for a bug fix; the settle barrier and the shim drain could land separately · Reuse: extends the existing shim queue and capture readiness steps · Simpler: a drain that stops on re-registration, or a capture-path-only drain, is smaller and avoids the semantics change
— Rames
| // pathological opposite (a queue that never empties): turns bound drain | ||
| // depth, and each turn dequeues at most ranCap - ran entries so a single | ||
| // oversized batch cannot blow the total-execution cap. | ||
| var RAF_DRAIN_MAX_TURNS = 64; |
There was a problem hiding this comment.
With 64 turns per seek, a self-rescheduling loop runs 64 times at the same virtual time on every captured frame, so per-callback state (counters, rotation += 1) advances 64x faster than on main. Stopping the drain once a turn re-registers a callback that already ran this seek would keep finite reveal chains settling without changing perpetual loops.
|
Thank you for the work here: the repro, the sabotage check and the before and after frame counts made the problem easy to see, and the pending-DOM bug you describe is real and still open. We are closing this version because the core change alters what every perpetual animation-frame loop does while rendering. |
What
Makes frame capture wait out DOM updates that author code defers through
requestAnimationFrame. Two pieces:seekToTimenow drains the rAF queue to quiescence at the authored virtual time instead of running exactly one flush. The drain is bounded by a turn cap (64) and a total-execution cap (2048 per-seek, sliced per turn) so perpetual self-rescheduling loops stay bounded.prepareFrameForCapturegains a post-seek settle step before the screenshot: it re-drains callbacks queued by post-seek hooks, forces a reflow so font/image loads start, awaitsdocument.fonts.readyandimg.decode()(src-keyed, so each asset is awaited once), and — only when the compositor free-runs and the settle observed pending work — commits two native paint frames via the shim's preservedoriginalRequestAnimationFrame.Why
Fixes #5888. On the current
main(852f6f9) the reporter's minimal repro — a GSAPonUpdatethat hides text and reveals it through a 10-turn rAF chain — renders 18/48 fully transparent frames with--format png-sequence --workers 2(reproduced exactly: frames 1–9 on worker 1, 25–33 on worker 2). A flush-once seek runs each queued callback once, so a callback that re-registers stays pending until the next seek; the first frames of each worker's slice capture the DOM mid-reveal.Related work
How
packages/producer/src/services/fileServer.ts:flushAnimationFrameis replaced bydrainAnimationFrames(maxTurns, maxCallbacks). Each turn dequeues at mostranCap - ranentries so one oversized batch cannot exceed the execution cap.seekToTimecalls it;__HF_VIRTUAL_TIME__also exposes it plus alastDrainstats getter ({turns, ran, pending}) so the capture side can tell whether deferred work ran.packages/engine/src/services/frameCapture.ts: newsettlePostSeekDomUpdates(page, allowNativePaintWait)runs after the existing seek-completion and LUT waits, before page-side compositing. Every wait races one shared wall-clock budget armed through the shim'soriginalSetTimeout(in-pageDate.now/performance.noware virtualized), default 1500 ms viaHF_POST_SEEK_SETTLE_MS(0 disables the settle entirely). The paint commit is evidence-gated: it only runs when the seek's drain, the safety-net drain, pending fonts, or undecoded images prove deferred work happened — steady-state frames pay a single no-op evaluate. BeginFrame-driven sessions (beginFrameTimeTicks !== 0) skip the native-paint wait because the capture's own BeginFrame is the commit.scripts/package-size-budgets.json: the post-seek settle barrier grows the packed engine tarball to 428660 bytes against the previous 428347 budget (new runtime code, not packaging drift — the package ships onlydist/). Raised to 450093, the check's own 5% headroom over the measured size; unpacked is unchanged and still under budget.Design notes:
seekToTimerather than only in the capture path means probe/distributed paths get the same settled-DOM guarantee, and the callbacks run at the seeked virtual timestamp — matching what deferred author work expects.document.fontsandimg.decode()waits are bounded by the shared budget and degrade to no-ops where the shim is absent (non-virtualized pages).Test plan
fileServer-rafDrain.test.ts(bun:test + node:vm shim harness): finite 10-turn reveal chain settles inside one seek, single-shot callbacks run at the seeked time, a perpetual loop is bounded to 64 turns/seek withpendingreported, fan-out growth is capped at 2048 executions,lastDrainstats, explicitdrainAnimationFrames, cancellation preserved.frameCapture.postSeekSettle.test.ts(vitest): post-seek drain, CSS-background re-decode gating, evidence-gated paint commits, BeginFrame skip, absent-shim no-op, src-keyed decode dedup across seeks.drainAnimationFrames is not a function, chains never settle); green after the fix.render --format png-sequence --fps 24 --workers 2: 18/48 transparent frames before, 0/48 after (aarch64, Chromium screenshot capture).HF_POST_SEEK_SETTLE_MS=0render also clean (shim drain alone carries the fix; the settle is the readiness-contract layer).frameCapture*.test.tsvitest files (300 tests) and the producer fileServer/probe/renderOrchestrator/vfx suites pass. The only local failures are two pre-existing host-dependentresolveRenderWorkerCounttests that fail identically on the base commit (aarch64 core-count heuristics, unrelated).HF_POST_SEEK_SETTLE_MSenv knob is documented in the code comment; happy to add a docs page entry if you'd like it user-facing.AI-assisted. I wrote and verified this change.