feat(producer): stream parallel screenshot/beginframe capture to the encoder#2224
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
#2224 — parallel streaming router + eligible_off signal
Verdict: LGTM 🟢
New shouldStreamParallelCapture router for non-drawElement multi-worker renders: streams captured frames directly to FFmpeg stdin (interleaved reorder-buffer writer) instead of the disk path (workers write JPEGs → sequential encode pass reads them back). Measured 1.29x on uniform-cost comps, wash on bimodal.
Key design properties:
- Mutually exclusive with DE routers by construction. Both DE predicates require
useDrawElement; this requires its negation. No overlap possible. - Kill switch default OFF (
HF_CAPTURE_PARALLEL_STREAM). Theeligible_offpassive signal is smart — measures how many fleet renders WOULD route before enabling, without changing behavior. Cohort-sizing for the soak. - Per-render local flag (
captureParallelStreamForced), not env mutation — same discipline asdeParallelStreamForced. Prevents cross-talk in concurrent renders. - Telemetry threaded through
RenderCaptureObservability.captureParallelStream→RenderObservabilityTelemetryPayload.captureParallelStream→capture_parallel_streamevent property. Coversrender_completeANDrender_error.
Tests cover all 7 gates (kill switch, worker count, drawElement, format, streaming, layered/effect). Clean.
Review by Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Current head 85dfb59 is merge-conflicting (mergeStateStatus DIRTY / mergeable CONFLICTING) and has no current required CI run beyond WIP. The router/test logic is directionally scoped, but I cannot approve a conflicted head. Rebase onto the current base, resolve conflicts without dropping adjacent DE/router observability changes, and rerun the required checks; then request re-review.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 85dfb59.
Clean routing addition + telemetry-first shape. The one-predicate + per-render-flag pattern matches the deParallelStreamForced shape exactly (no env mutation across renders, no cross-talk), and the mutual exclusion with the two DE routers is enforced by construction — both DE predicates require useDrawElement === true, this predicate requires !useDrawElement, so the three can't co-fire at the same call site. Good.
The eligible_off signal is the right shape for a default-off soak: emits precisely when the ONLY failed gate is the kill switch. Cohort-sizing that goes silent as soon as the flag flips on — dashboards can chart eligible fleet fraction over time without needing a "would-route" derivation off flag state. Symmetric on both render_complete and render_error events (verified in the added test at packages/cli/src/telemetry/events.test.ts:265-278), so failure-path renders aren't dropped from the soak sample.
The uniform/bf 1.096× / 1.10 bar shortfall is a legit "valid negative result" call — capture is already CPU-bound on the 8-vCPU SwiftShader box, so the encode overlap ceiling is much smaller than the macOS hardware-GPU calibration point suggests. Documented in the PR body, not chased with impl changes — right call.
One cross-cut observation, not a blocker: the DE parallel router's outcome shape (deParallelRouter: "routed" | "reverted" | "none") doesn't carry a "eligible_off" equivalent — a render that would have routed through the DE parallel router but hit e.g. the new RAM floor (or HF_DE_PARALLEL_ROUTER=false) just becomes parallelRouter: false in perfSummary, indistinguishable from every other non-routed reason. If eligible_off is the fleet-standard cohort-sizing shape you're establishing here, the DE router could adopt it in a follow-up (I raised this same concern from a different angle on #2249's clarifying question). Non-blocking — this PR sets the pattern, the DE router follows later.
LGTM from my side.
…stream router is off
85dfb59 to
591235f
Compare
|
Rebased onto current main (head is now 591235f) and resolved the conflicts — Conflict resolution notes (per @miguel-heygen's ask, nothing adjacent was dropped):
Diff scope unchanged — still exactly the same 7 files. |
Superseded: merge-conflict blocker resolved at current head 591235f; current diff re-reviewed and checks are green.
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed current head 591235f after rebase. The former conflict blocker is resolved; router gates remain mutually exclusive with DE, kill switch defaults off, per-render state is local, and telemetry is wired for complete/error with stable enums. Required checks are green. My prior CHANGES_REQUESTED review was dismissed as superseded; LGTM pending an independent approver.
Summary
Routes multi-worker NON-drawElement renders (screenshot on macOS/Windows, BeginFrame on Linux/Docker) through the existing interleaved parallel-streaming encoder (PR #2056 machinery) instead of the parallel disk path, behind default-off
HF_CAPTURE_PARALLEL_STREAM. Zero engine/stage changes — one orchestrator predicate (shouldStreamParallelCapture) + per-render flag threading (captureParallelStreamForced, following the existingdeParallelStreamForcedpattern — noprocess.envmutation) + telemetry (capture_parallel_streamon bothrender_completeandrender_error).Motivation: on the disk path, encode is a purely additive tail after capture completes. On Linux the whole fleet is drawElement-ineligible (SwiftShader, no hardware GPU), so this is the only parallel-encode speedup lever available there.
Soak signal while default-off: renders that pass every gate except the kill switch emit
capture_parallel_stream: "eligible_off"(observability-only, no log line, no behavior change). This sizes the eligible cohort fleet-wide before anyone enables the flag;"screenshot"/"beginframe"values mean the render actually routed.Benchmarked end-to-end on native Linux (AMD EPYC 9R14, 8 vCPU, chrome-headless-shell 152.0.7928.2, software/SwiftShader rendering) — 3 reps per cell, arms interleaved, hygiene between runs, every arm verified from logs (not flags).
Numbers (Linux, native)
Task 2 — pre-implementation baseline (escape-hatch
HF_DE_PARALLEL_STREAM=true)Per-cell medians (ms) and streaming-vs-disk speedup:
Go/no-go gate: GO — all 36 rows (2 comps × 2 capmodes × 3 arms × 3 reps) verified
okfrom logs (zeroNO_STREAMING/DISK_FALLBACK/NOT_BEGINFRAME/NOT_SCREENSHOT/DE_ENGAGED); this is the first time the streaming plumbing has been confirmed working end-to-end under BeginFrame in production (previously only code-verified).Task 6 — post-implementation validation (production router
HF_CAPTURE_PARALLEL_STREAM=true)Router smoke: log line +
captureParallelStreamobservability field present when enabled; with no env var set the render uses the disk path and reportseligible_off(multi-worker eligible renders) or no field at all (ineligible, e.g. single-worker) — all three directions verified end-to-end.Criterion 1 — router-mode
w3streamwithin ±10% of Task 2's flag-modew3stream(proves the new gate reuses the identical mechanism, not a divergent path):Criterion 2 — uniform comp:
w3stream≥1.10× faster thanw3disk:Criterion 3 — static comp:
w3streamno more than 1.10× slower thanw3disk:Criterion 4 — PSNR parity (re-run on router-mode outputs):
All ≥45dB — PASS across all 6 comparisons.
On the uniform/bf 1.096× shortfall (criterion 2)
Not a wiring defect. Stage decomposition from the disk path (
captureMs/encodeMsfromperfSummary.stages):uniform/bfdisk-path capture takes ~79.2s, encode ~40.4s, giving a theoretical streaming ceiling ofdisk_total/captureMs ≈ 1.52×(if encode fully hid behind capture). The observed 1.096× captures only ~18% of that ceiling because this benchmark machine is an 8-core box running 3 concurrent SwiftShader (software-GL) capture workers — capture is already CPU-bound, leaving little idle capacity for the also-CPU-bound libx264 encode to overlap into. The 1.10× bar was calibrated to the original macOS spike's hardware-GPU numbers (1.29× observed there, where capture uses the GPU and leaves CPU free for encode); this machine's ceiling is comparable-or-larger (1.33–1.52× across cells) but a much smaller fraction gets captured due to CPU contention.uniform/ssclears the bar on the same hardware. Recorded in full in the benchmark notes; not chased with implementation changes per the plan's own "valid negative result" guidance.Test plan
renderOrchestrator.test.ts— 122/122 passingcapture_parallel_streamonrender_completeANDrender_error, including theeligible_offvalue) — 34/34 passingok), PSNR parity disk-vs-stream and w1-vs-w3stream all ≥45dBeligible_offfield, disk path unchanged; flag on → routed (beginframe); ineligible render (W1) → field absentbunx tsc --noEmitclean on bothpackages/producerandpackages/cli🤖 Generated with Claude Code