Skip to content

feat(producer): stream parallel screenshot/beginframe capture to the encoder#2224

Merged
vanceingalls merged 4 commits into
mainfrom
parallel-streaming
Jul 11, 2026
Merged

feat(producer): stream parallel screenshot/beginframe capture to the encoder#2224
vanceingalls merged 4 commits into
mainfrom
parallel-streaming

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

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 existing deParallelStreamForced pattern — no process.env mutation) + telemetry (capture_parallel_stream on both render_complete and render_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:

comp capmode W1 median W3-disk median W3-stream median stream speedup vs disk
uniform bf 156,276 120,559 110,076 1.095×
uniform ss 274,064 163,759 148,678 1.101×
static bf 146,092 94,166 86,595 1.087×
static ss 166,228 116,166 118,937 0.977× (2.4% slower)

Go/no-go gate: GO — all 36 rows (2 comps × 2 capmodes × 3 arms × 3 reps) verified ok from logs (zero NO_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 + captureParallelStream observability field present when enabled; with no env var set the render uses the disk path and reports eligible_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 w3stream within ±10% of Task 2's flag-mode w3stream (proves the new gate reuses the identical mechanism, not a divergent path):

cell baseline (flag) router delta verdict
uniform/bf 110,076 109,747 −0.30% PASS
uniform/ss 148,678 148,689 +0.01% PASS
static/bf 86,595 86,725 +0.15% PASS
static/ss 118,937 117,391 −1.30% PASS

Criterion 2 — uniform comp: w3stream ≥1.10× faster than w3disk:

cell w3disk w3stream speedup verdict
uniform/bf 120,302 109,747 1.096× FAIL (just under bar — see note below)
uniform/ss 163,868 148,689 1.102× PASS

Criterion 3 — static comp: w3stream no more than 1.10× slower than w3disk:

cell w3disk w3stream ratio verdict
static/bf 93,207 86,725 0.930× (faster) PASS
static/ss 115,977 117,391 1.012× PASS

Criterion 4 — PSNR parity (re-run on router-mode outputs):

comparison uniform/bf uniform/ss static/bf static/ss
disk vs stream 51.60 dB 51.78 dB 59.42 dB 56.53 dB
comparison (BF only, interleaved-determinism) uniform static
W1 vs W3-stream 46.46 dB 47.98 dB

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/encodeMs from perfSummary.stages): uniform/bf disk-path capture takes ~79.2s, encode ~40.4s, giving a theoretical streaming ceiling of disk_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/ss clears 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

  • predicate unit matrix (7 tests) in renderOrchestrator.test.ts — 122/122 passing
  • telemetry payload + event tests (capture_parallel_stream on render_complete AND render_error, including the eligible_off value) — 34/34 passing
  • Linux end-to-end: router smoke (log line + observability field present when enabled, absent when not), full 36-cell benchmark matrix in router mode (all verified ok), PSNR parity disk-vs-stream and w1-vs-w3stream all ≥45dB
  • default-off verified end-to-end, all three directions: flag off + eligible render → eligible_off field, disk path unchanged; flag on → routed (beginframe); ineligible render (W1) → field absent
  • bunx tsc --noEmit clean on both packages/producer and packages/cli

🤖 Generated with Claude Code

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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). The eligible_off passive 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 as deParallelStreamForced. Prevents cross-talk in concurrent renders.
  • Telemetry threaded through RenderCaptureObservability.captureParallelStreamRenderObservabilityTelemetryPayload.captureParallelStreamcapture_parallel_stream event property. Covers render_complete AND render_error.

Tests cover all 7 gates (kill switch, worker count, drawElement, format, streaming, layered/effect). Clean.

Review by Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Review by Rames D Jusso

@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (head is now 591235f) and resolved the conflicts — mergeable is green again.

Conflict resolution notes (per @miguel-heygen's ask, nothing adjacent was dropped):

  • renderOrchestrator.ts: kept main's new shouldRetryViaPinnedFallback alongside shouldStreamParallelCapture (both had been added after the same anchor); main's isMemoryExhaustion param on resolveParallelRouterRetryPlan untouched.
  • observability.ts / events.ts / renderObservability.ts: kept main's new DE failure-path fields (dePreRouterWorkers, deFallbackReason, the captureDe* mirror block and their de_* event keys) with captureParallelStream appended after them, matching the original ordering intent.
  • Test files: main's new DE-cohort describe blocks and this PR's parallel-stream blocks coexist; all 182 tests across the three touched suites pass post-rebase, and tsc --noEmit is clean on producer + cli.

Diff scope unchanged — still exactly the same 7 files.

@miguel-heygen miguel-heygen dismissed their stale review July 11, 2026 22:44

Superseded: merge-conflict blocker resolved at current head 591235f; current diff re-reviewed and checks are green.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vanceingalls vanceingalls merged commit f53a75b into main Jul 11, 2026
70 of 71 checks passed
@vanceingalls vanceingalls deleted the parallel-streaming branch July 11, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants