fix: discard the warmup capture frame in the parallel coordinator

[mvanhorn]

What

Discards the throwaway warmup frame that each parallel worker captures before its first real seek has painted, so no fully transparent frame is emitted at a worker chunk boundary.

Why

In parallel capture, a worker's very first capture runs before the seek it issued has painted, so the first frame of every worker chunk came back fully transparent. Issue #2477 reported a transparent frame at each chunk boundary. The single-worker path already warms up and discards that first frame; the parallel coordinator was missing the same step.

How

parallelCoordinator now calls discardWarmupCapture(session) immediately after a worker session is initialized, mirroring the existing single-worker warmup. frameCapture.ts was left unchanged because its existing helper already restores the frame counters, so accounting stays correct after the discard.

Test plan

• Unit tests added/updated

Added parallelCoordinator tests covering call ordering (warmup before the first real capture), worker 0 vs non-zero workers, frame accounting after the discard, warmup failure, abort handling, and session cleanup. Targeted Vitest: 30 passed. oxfmt and oxlint are clean on the changed files.

Fixes #2477

[miguel-heygen]

Blocking finding

[P1] The warmup neither targets the failing boundary nor preserves the compositor's monotonic-time contract — packages/engine/src/services/parallelCoordinator.ts:421-428

I reproduced #2477 from the base before evaluating this patch: at e038cc93c, executeWorkerTask initializes a fresh session and immediately enters captureFrameRange; the first operation for worker 1 is therefore the seek/capture for frame 36, with no paint-settlement barrier. That matches the reported transparent frames at 36/72/108.

This change does not warm that failing seek. discardWarmupCapture(session) uses its defaults, frame 0/time 0 (frameCapture.ts:3250-3262), even when the task starts at frame 36/time 1.2. My current-head negative proof asserted the helper call for runWorker(1, 36) and received only [session], not [session, 36, 1.2]. The new test at parallelCoordinator.test.ts:63-80 mocks the helper as a no-op and checks only call order, so it passes without exercising the frame/time contract.

More critically, blindly capturing a warmup frame reinstates a known compositor deadlock. The current production path documents why discardWarmupCapture is intentionally absent: a second beginFrame at an already-used frameTimeTicks wedges Chrome (packages/producer/src/services/distributed/renderChunk.ts:579-582). Commit 24f64f02c removed the same approach after real multi-chunk validation found both same-time and backward-time deadlocks. This PR calls the helper for worker 0 at frame 0/time 0, immediately followed by the real frame 0/time 0 capture, which is exactly the same-time failure mode. Passing each worker's startFrame would still duplicate that timestamp; passing startFrame - 1 was the already-reverted backward-time failure.

Please fix the actual first-seek paint race without issuing an extra screenshot/beginFrame at a duplicate or backward compositor timestamp—for example, a seek/paint-settlement primitive separated from capture. The regression must reproduce the real boundary: render/decode the transparent multi-worker fixture, assert non-transparent output at interior starts, and prove worker 0 completes without a protocol timeout. A mocked “warmup was called” assertion cannot establish either invariant.

What is good

The failure and abort tests at parallelCoordinator.test.ts:91-115 correctly preserve session cleanup if a pre-capture stage fails. That cleanup discipline should remain in the eventual fix.

CI snapshot: core build/lint/typecheck/test, Windows render/tests, producer suites, and CodeQL are green at 82803ad95598a9e690817f5f8162ac095178f2fa; several regression shards are still running. The blocker above is deterministic and independent of those pending lanes.

Verdict: REQUEST CHANGES

Reasoning: #2477 is reproducible in the base call graph, but this patch warms frame 0 rather than the worker boundary and reintroduces a previously validated Chrome compositor deadlock pattern. It needs a monotonic-time-safe paint-settlement fix and a real boundary-render regression test.

[yahelshiran-rvr]

Two code changes that might address both review findings:

1. Pass the worker's actual boundary instead of relying on the defaults, so the failing seek is the one that gets warmed:

   await discardWarmupCapture(
   session,
   task.startFrame,
   task.startFrame * captureOptions.fps.den / captureOptions.fps.num
   );

With the current defaults (frame 0 / time 0) the warmup seeks a frame the worker never renders, so the boundary race at task.startFrame is untouched.

2. Guard the warmup to screenshot mode:

   if (session.captureMode !== "beginframe") { ... }

The deadlock documented in renderChunk.ts:579-582 (and hit by 24f64f02c) is a beginFrame timestamp contract problem: the warmup spends a compositor tick, so the real capture repeats frameTimeTicks and wedges. Screenshot capture has no compositor clock, a repeated seek to the same time is idempotent there, so the deadlock surface doesn't exist in that mode.

That would leave beginFrame mode still needing the seek/paint-settlement primitive the review asks for, but a mode-guarded, startFrame-targeted warmup could fix the screenshot path (where transparent overlay renders live) without reintroducing the reverted failure modes.

WDYT? @mvanhorn

[miguel-heygen]

Re-review after taking over the fix at exact head 121a02e0ec9aa69c57e1fbf5e14759ef0ad8cf46.

The original unconditional warmup was unsafe because BeginFrame/drawElement capture cannot be repeated at the same compositor timestamp without risking the deadlock previously removed in 24f64f02c. The revised implementation confines paint settlement to the affected path: fresh non-zero screenshot workers only, at the exact first worker frame/time. Worker 0 and both compositor-driven capture modes are explicitly excluded, and the helper rejects non-screenshot sessions defensively. Cleanup on warmup failure/abort remains covered.

Evidence:

• TDD pinned exact frame/time, worker-0 exclusion, BeginFrame/drawElement exclusion, cleanup, and abort behavior; focused suite: 40/40.
• Full @hyperframes/engine suite with repo-compatible static FFmpeg: 44 files passed (1 skipped), 1008 tests passed (3 skipped).
• Engine typecheck, build, oxfmt, oxlint, fallow, and commit hooks passed.
• Real four-worker transparent WebM regression render passed 60/60 frames; alpha-plane samples remained nonzero across every worker boundary.
• Fresh CI at this exact head is fully green, including Linux tests, both Windows render/test lanes, CodeQL, Preview parity, and all eight producer regression shards.

Verdict: APPROVE

Reasoning: The patch now fixes the screenshot-worker paint race at its actual boundary without reintroducing the duplicate-timestamp compositor deadlock, and tests plus complete current-head CI cover the relevant modes and multi-worker transparency path.

— Deepwork

Exact reporter failure was not reproduced; approval was premature. Reopening verification gate until RED→GREEN evidence exists.

[miguel-heygen]

Verification correction at head 121a02e0ec9aa69c57e1fbf5e14759ef0ad8cf46.

The revised patch is internally safe against the known duplicate-BeginFrame deadlock and passes synthetic multi-worker transparency coverage, but I did not reproduce the reporter's exact #2477 failure before the change. Therefore I cannot distinguish with sufficient evidence between (a) the first-paint race this patch addresses and (b) a second composition/environment-specific cause. I dismissed my approval; this remains a candidate fix pending a reporter artifact or equivalent deterministic RED→GREEN reproduction.

Verdict: COMMENT

Reasoning: Safety and regression evidence are strong, but root-cause closure is not proven without reproducing the reported blank-boundary failure and demonstrating that this change removes it.

— Deepwork

[mvanhorn]

Got a deterministic RED→GREEN for this. In parallelCoordinator.test.ts, "settles a non-zero screenshot worker at its first absolute frame" asserts the warmup capture fires at the chunk-boundary frame before the real capture — order [initialize, warmup, capture:36, capture:37] for worker 1 in a 4-worker/144-frame layout. Revert settleFirstScreenshotFrame and that test (plus 3 others) goes RED: no warmup, so the worker's first real capture lands on frame 36 racing the unpainted seek — frame 36 = 1*ceil(144/4), exactly the interior chunk boundary the reporter saw transparent. It reproduces the root cause deterministically at the coordinator level (no flaky browser timing), which is what separates the first-paint race from an environment-specific cause. Happy to add an end-to-end pixel check too if you'd want belt-and-suspenders.

[miguel-heygen]

Re-reviewed exact head 121a02e.

The revised scope is safe and matches the failure mechanism: only fresh non-zero screenshot workers take the settlement capture, using the same relative frame index and absolute composition time as their first real capture. Worker 0 and the BeginFrame/drawElement paths are excluded, so the duplicate/backward compositor-timestamp deadlock from the earlier revision is not reintroduced. The helper also restores capture counters, dedup state, and the last-frame buffer, including on failure.

Independent verification:

• focused coordinator/frame-capture tests: 40/40 passed
• full repository build passed
• four-worker transparent WebM rendered 60/60 frames
• direct VP9 alpha-plane inspection stayed nonzero at and around all three worker boundaries (15/30/45)
• exact-head CI is fully green, including Windows, CodeQL, preview parity, and all eight regression shards

I also ran the same four-worker fixture on the base commit. It was green there on this host, so I could not produce an environment-level RED-to-GREEN; the original report remains load-sensitive and no reporter artifact was supplied. That limits certainty about exact issue closure, but it does not expose a code blocker: the deterministic coordinator contract covers the missing first-worker settlement, and the implementation is narrowly isolated from the known compositor-clock hazard.

One non-blocking test note: the checked-in regression fixture allows two visual failures, so the direct boundary-alpha assertion remains manual rather than encoded. Tightening that would improve future proof, but the unit contract and current-head empirical result are sufficient for this patch.

Verdict: APPROVE

Reasoning: The current implementation safely closes the missing non-zero screenshot-worker settlement path, preserves capture state, avoids the known duplicate-timestamp modes, and has proportional unit, render, alpha-plane, and full-CI evidence.

— Magi

[miguel-heygen]

Correction to my prior approval after exact-head verification.

Blocking finding

The warmup does not fully restore capture performance state. capturePerf.frameMs is a mutable array, but discardWarmupCapture snapshots capturePerf with a shallow spread. captureFrameCore appends the discarded sample to that same array; assigning the shallow snapshot back therefore retains the warmup sample and biases getCapturePerfSummary().p50TotalMs.

The implementation comment that all capturePerf fields are primitive is false: the type includes frameMs: number[]. The existing fake session omits frameMs under an unknown cast, so the tests never exercise the alias.

I added the minimal missing test locally at exact head: seed frameMs with [13, 17], clone the expected array, push 50 in the injected warmup capture, then assert full restoration. Current head fails deterministically with received [13, 17, 50] (7 tests pass, 1 fails).

Please snapshot frameMs by value (and restore it on both success and failure) and add the focused regression before merge.

Issue-closure correction

The distributed single-worker path explicitly does not call discardWarmupCapture because of the BeginFrame timestamp contract. The coordinator test proves the newly selected call ordering, not a stale/transparent browser frame, and all real base/head comparisons remained identical. The patch may be useful defensive hardening, but exact #2477 closure is not demonstrated without the reporter artifact/environment. Change Fixes #2477 to Related to #2477 and keep the issue open pending reporter validation.

Verdict: REQUEST CHANGES

Reasoning: Current head leaks the discarded warmup timing sample into p50 telemetry through a shallow-copied frameMs array, and the existing tests omit the nested state. The fix is small and deterministic; exact issue closure should remain provisional.

— Magi

[jrusso1020]

Additive review after Magi's accounting finding.

The mode/start guard at packages/engine/src/services/parallelCoordinator.ts:476 is a good correction: it keeps worker 0 and compositor-clock-driven capture modes out of the duplicate-timestamp path.

Blocking finding

[blocker] The warmup settles one seek, then the real capture immediately issues another seek before reading pixels — packages/engine/src/services/parallelCoordinator.ts:422,365-368,481; packages/engine/src/services/frameCapture.ts:2145-2151

discardWarmupCapture is a full captureFrameCore: it seeks to the boundary and screenshots it. captureFrameRange then calls the normal capture for that same boundary, and captureFrameCore seeks to the same timestamp again before taking the emitted screenshot. For a supported composition or adapter that schedules rAF/compositor work on every seek, the throwaway screenshot can settle the first seek, while the second seek schedules fresh work and recreates the original stale/transparent-frame race.

The engine already documents the required settlement shape at packages/engine/src/services/frameCapture.ts:3492-3500: seek once, take a throwaway screenshot to force rAF work, then take the retained screenshot without seeking again. The coordinator test at packages/engine/src/services/parallelCoordinator.test.ts:56-82 mocks warmup and capture separately, so it cannot exercise this invariant; the checked-in visual fixture also permits two failed frames (packages/producer/tests/webm-transparency/meta.json:6).

Please make the first-frame path prepare/seek once and capture twice, or introduce a screenshot-only paint barrier that does not run prepareFrameForCapture again. Add coverage that pins the absence of a second seek and, ideally, a zero-alpha boundary assertion.

I also agree with Magi's existing blocker that the shallow capturePerf snapshot leaks the discarded frameMs sample into p50 accounting. That needs the focused value-copy regression already described there.

Verdict: REQUEST CHANGES
Reasoning: The current implementation does not guarantee that the screenshot it emits observes the paint forced by the warmup, because it performs another seek in between. Together with the definite frameMs state leak, current head should not merge.

— Codex

[mvanhorn]

Pushed 04b68b2 for the blocker: the warmup now settles the boundary seek once and the real capture reads pixels without re-seeking, so the duplicate-timestamp path is gone. Engine suite is green locally (1008 passed / 44 files) plus typecheck and lint.