Repository navigation
fix(engine): keep BeginFrame time monotonic across frame rates - #3011
Conversation
Warmup advances Chromium on a fixed 33ms clock, but the capture baseline was derived only from the output frame interval. At rates above 30fps this moved BeginFrame timestamps backwards and could stall headless Chrome. Preserve the legacy baseline whenever it is already monotonic. Otherwise continue from the warmup clock, then add capture-rate headroom. Centralize commit and probe tick derivation so every producer path uses the same ordering contract. Add locked, unlocked, integer, and NTSC regression coverage, plus production probe argument assertions.
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
R1 — Monotonic BeginFrame timing (head 22bc4d6)
Verdict: Approve
Clean fix for a real hang at high frame rates. The root cause is clear: warmup runs at 33ms intervals, but the capture baseline was calculated from the output fps. At 60fps, the commit tick falls BEFORE the last warmup tick (1067ms vs 1947ms), Chrome sees time go backwards, and beginFrame stalls.
What was verified
deriveBeginFrameTimeTicks logic — correct at all tested rates:
| fps | captureInterval | legacyCommit | lastWarmup | legacy safe? | result |
|---|---|---|---|---|---|
| 30 | 33.33ms | 2133ms | 1947ms | yes | legacy (2333ms) |
| 60 | 16.67ms | 1067ms | 1947ms | no | monotonic (2147ms) |
| 120 | 8.33ms | 533ms | 1947ms | no | monotonic (2063ms) |
| 240 | 4.17ms | 267ms | 1947ms | no | monotonic (2022ms) |
The crossover is at ~33fps (where captureInterval matches warmupInterval). Everything at or below ~33fps preserves the legacy baseline (backward-compatible). Everything above gets rebased past warmup. The invariant warmup < commit < probe < capture holds in all cases.
SSOT improvements:
- Probe tick calculation was inline in TWO places (
renderChunk.tsandprobeStage.ts) — both now usederiveBeginFrameProbeTimeTicks. Good dedup. - Commit tick calculation was inline in
initializeSession— now throughpreparedBeginFrameTimeline.commitParams. - Three magic constants (10, 6, 5) are now named:
BEGIN_FRAME_CAPTURE_HEADROOM_INTERVALS,BEGIN_FRAME_COMMIT_LEAD_INTERVALS,BEGIN_FRAME_PROBE_LEAD_INTERVALS.
Tests: Strong coverage — parametric tests across 9 frame rates (24, 30, 31, 32, 33, 60, 120, 240, 59.94), monotonicity assertions, legacy-preservation assertions, unlocked warmup, and session-init wiring. Producer probe test asserts the exact frameTimeTicks passed to probeBeginFrameLiveness.
Edge cases:
LOCKED_WARMUP_TICKS = 0: legacy commit =4 * captureIntervalMs > 0→ always safe. Correct.- Unlocked warmup with fewer ticks: explicitly tested.
Math.max(0, ...)in probe derivation guards against negative ticks at very low capture rates. Correct.
No findings. Ship it.
Review by Miga
|
@miguel-heygen This is ready to merge. |
Fixes #3012
What
Fixes
HeadlessExperimental.beginFramehanging at 60fps and other high frame rates.Why
Warmup always advances Chrome in 33ms steps, but the first capture timestamp was calculated from the output fps.
At 60fps, the last warmup tick is 1947ms, while the next commit tick used to jump back to about 1067ms. Chrome sees time moving backwards and
beginFramecan stall.How
Keep the old timestamps when they are already safe. If the commit tick would go backwards, move the capture baseline past warmup first.
The init commit and both producer probes now use the same timestamp helpers, so they cannot calculate different values.
Test plan