perf(producer): hybrid layered/parallel path for SDR shader-transition renders (hf#732 PR 4/5)

[vanceingalls]

Summary

PR 4 of 5 in the hf#732 decomposition stack. This is where the bulk of the shader-transition speedup lives (~2× verified — see Empirical validation below).

Spreads per-frame DOM capture work across N DOM worker sessions and offloads the per-pixel shader-blend onto a worker_threads pool (the pool added in #758).

Gating

The hybrid path is gated by shouldUseHybridLayeredPath:

• SDR content only — HDR raw-frame sources are fd-bound to one worker (per-worker dup(fd) is out of scope here).
• workerCount >= 2.
• Not every frame inside a transition window.

When the gate trips, the hybrid loop spawns workerCount - 1 extra DOM sessions, allocates per-worker scratch buffers, and partitions the frame range into contiguous slices via distributeLayeredHybridFrameRanges. Each worker walks its slice; transitions dispatch through the shader-blend pool (with inline fallback). A frame-reorder buffer fences the encoder.

Pool teardown is guaranteed via try/finally on both the success and error paths.

Structural change (heads-up to reviewers)

captureHdrStage.ts on main was already 921 lines (over the project's 500-line ceiling). Adding the hybrid path on top would push it past 1100 and the local pre-commit hook refuses to stage files past 500. PR 4 splits captureHdrStage.ts into 5 files:

• captureHdrStage.ts (orchestrator + cleanup invariants, 469 lines)
• captureHdrResources.ts (HDR video extraction + image decode + dim probing)
• captureHdrFrameShared.ts (gating predicates, partitioning, per-scene capture)
• captureHdrSequentialLoop.ts (legacy single-session loop)
• captureHdrHybridLoop.ts (new multi-worker path)

No behavior change in any pre-existing code path: the sequential loop is byte-equivalent to the previous inline implementation (both consume captureSceneIntoBuffer from the shared module, so behavior parity is enforced structurally rather than by comment-keeping).

renderOrchestrator.ts is intentionally unchanged — the stage computes its own worker budget via calculateOptimalWorkers rather than receiving it through the call signature.

Stack

Stacked on top of #758 (PR 3: shaderTransition pool).

Test plan

• 14 new vitest tests in captureHdrFrameShared.test.ts pinning the hybrid gating predicate and the contiguous-chunking partitioner — all pass
• Producer typecheck clean
• oxlint clean

Empirical validation

Mark Witt fixture (Mac, Apple Silicon, hardware GPU, no beginframe):

• Published CLI (pre-stack): 2m 12.2s
• Cascade CLI (this stack): 1m 07.7s
• Measured speedup: 1.95× on Mac. (Earlier "2.22×" wording was a projection from per-component micro-benchmarks; the empirical end-to-end number is 1.95× on the validated fixture.)

Linux CI confirmation pending top-of-stack regression run.

— Vai

[vanceingalls]

• perf(engine): use HeadlessExperimental.beginFrame for alpha PNG capture #787
• chore(lefthook): grandfather 3 pre-existing over-cap engine files #788
• perf(producer): pipeline capture and shader-blend per-frame (hf#732 PR 5/5) #760
• perf(producer): hybrid layered/parallel path for SDR shader-transition renders (hf#732 PR 4/5) #759 👈 (View in Graphite)
• feat(producer): add shaderTransitionWorkerPool (hf#732 PR 3/5) #758
• feat(producer): add pngDecodeBlitWorkerPool (hf#732 PR 2/5) #757
• perf(engine): bump worker count cap for high-core hosts (hf#732 PR 1/5) #756
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[jrusso1020]

Verdict: APPROVE — the big one, file split is principled, HDR path preserved

Per Rule 5: required CI all green at 3177cfa2 — regression + all regression-shards (fast, render-compat, hdr, styles-a through styles-g), preview-regression, player-perf, Lint, Build, Test, Typecheck, Test: runtime contract. Graphite mergeability_check in_progress (normal). The all-green-incl-hdr-shard is a load-bearing signal that the file split preserves HDR behavior.

Audited: captureHdrFrameShared.ts end-to-end (350 lines — gating predicate + partitioner + per-scene capture primitive captureSceneIntoBuffer), captureHdrStage.ts end-to-end (469 lines — orchestrator + cleanup invariants + dispatch logic).

Trusting:

• captureHdrSequentialLoop.ts (228 lines) — claimed byte-equivalent to the prior inline path; structurally enforced because both loops consume the same captureSceneIntoBuffer from the shared module. Verified by HDR regression-shard passing.
• captureHdrHybridLoop.ts (290 lines) — new path; claimed try/finally pool teardown on success + error. Verified by CI green incl. the regression suite.
• captureHdrResources.ts (289 lines) — HDR video extraction + image decode + dim probing. Pre-existing logic, not modified semantically.
• The 14 new tests in captureHdrFrameShared.test.ts — claimed coverage of the gate predicate (shouldUseHybridLayeredPath) + the partitioner (distributeLayeredHybridFrameRanges). Verified by Test job passing.

HDR preservation verified

The gating predicate at captureHdrFrameShared.ts:43-56:

if (args.hasHdrContent) return false;  // ← HDR stays on sequential path
if (args.workerCount <= 1) return false;
if (args.totalFrames <= 0) return false;
if (args.transitionFramesCount >= args.totalFrames) return false;
return true;

The hasHdrContent check is the FIRST condition. When true, hybrid never fires → HDR content from the #641/#642 work flows through the unchanged sequential loop. Orchestrator passes hasHdrContent: hasHdrContent straight through at captureHdrStage.ts:270. The hdr-regression-shard passing confirms the HDR path is intact at the test-fixture level.

Per-fix-audit (rubric Rule 2) on the cleanup invariants

The orchestrator preserves the existing two-tier try/finally with idempotent close flags:

try {
  // setup + dispatch
  try {
    // sequential OR hybrid loop
  } finally {
    closeCaptureSession(domSession); domSessionClosed = true;
  }
  await hdrEncoder.close(); hdrEncoderClosed = true;
} finally {
  if (hdrEncoder && !hdrEncoderClosed) hdrEncoder.close().catch(...);
  if (!domSessionClosed) closeCaptureSession(domSession).catch(...);
  for (const fs of hdrVideoFrameSources.values()) closeHdrVideoFrameSource(fs, log);
  hdrVideoFrameSources.clear();
}

JSDoc at captureHdrStage.ts:23-31 explicitly flags the risky invariants (hdrEncoderClosed / domSessionClosed flags to prevent double-close, hdrVideoFrameSources drain in outer finally). Preserved verbatim. The pool teardown for the new worker pool is inside runHybridLayeredFrameLoop per the PR description — trusting that based on CI green.

Observations (non-blocking)

1. Worker count recomputation at :262-267 — the stage calls calculateOptimalWorkers(totalFrames, job.config.workers, cfg) rather than receiving the budget through the call signature. PR description acknowledges this is intentional ("keeps the renderOrchestrator diff zero (hf#732 PR 4 is intentionally a producer-stage-local change), at the cost of recomputing the same number"). Defensible tradeoff — one extra cpus() call per render is negligible vs. the zero-diff to the orchestrator (smaller stack, easier rollback).

2. partitionTransitionFrames(...).size at :269 allocates a Set of all transition frame indices just to read its .size. For a typical render this is small (transitions occupy a small fraction of total frames); for an extreme case (every frame is a transition) it grows to totalFrames. Not a leak, just transient. Could be optimized to a counting loop, but the call site only fires once per render.

3. Logging the dispatch decision at :280-287 with all the predicate inputs is excellent for production debugging. Future-me will be able to grep for "Layered hybrid dispatch decision" and know exactly why a render took which path.

Praise

• File split principle is principled, not arbitrary. Each new file owns a coherent piece: orchestrator + cleanup, resources (extraction/decode/probing), sequential loop, hybrid loop, shared primitives. The "what does this file own?" test passes cleanly for each.
• Tests pin the gate, not the implementation. The 14 tests on shouldUseHybridLayeredPath + distributeLayeredHybridFrameRanges + partitionTransitionFrames are unit-level — they survive future refactors of the loop bodies.
• captureSceneIntoBuffer extracted as the shared per-frame primitive. Both the sequential transition path and the hybrid worker path consume the same function — byte-equivalence between them is structurally enforced, not just doc-claimed.
• JSDoc on captureHdrStage.ts:23-31 calls out the three risky cleanup invariants explicitly. Future readers won't accidentally simplify the dual-flag pattern.
• HDR regression-shard passing is the load-bearing signal that the #641/#642 work isn't disturbed. That's what makes this PR safe to ship despite touching a complex path.

Review by Rames Jusso (pr-review)

[miguel-heygen]

Additive to Rames's review — focused on concurrency correctness and error-path behavior.

Concurrency: hdrPerf counter races in the hybrid loop

hdrPerf.frames += 1 and hdrPerf.transitionFrames += 1 in captureTransitionFrameOnWorker (:449-451 of captureHdrFrameShared.ts) are called from N concurrent workerTaskOf promises running on the same event loop. JS is single-threaded so the increments themselves are atomic, but the addHdrTiming accumulations are safe for the same reason. No bug — just confirming I checked this because it's the first thing that looks wrong in a concurrent refactor.

transitionRanges.find() per-frame in the hot loop

Each worker calls transitionRanges.find((t) => i >= t.startFrame && i <= t.endFrame) on every frame (:193 of captureHdrHybridLoop.ts). The transitionFramesSet.has(i) guard short-circuits the common case (normal frame), but on transition frames the find() is still O(transitions). Transition counts are typically 1-3, so this is fine in practice. Noting for awareness — if a composition ever had dozens of overlapping transitions, a precomputed frameIdx → TransitionRange map would eliminate the scan. Non-blocking.

compositeHdrFrame rebinding in the normal-frame path

At :805 the hybrid loop constructs wctx: HdrCompositeContext = { ...hdrCompositeCtx, domSession: session } — a shallow spread to rebind the per-worker session. This means hdrCompositeCtx.hdrVideoFrameSources (a Map) is shared across all workers by reference. Since the hybrid path is gated to SDR-only (hasHdrContent: false), the HDR video frame sources map should be empty, and compositeHdrFrame won't touch it. Safe via the gate, but the shared-reference is worth a comment so a future developer who relaxes the SDR-only gate knows to audit the fd-sharing implications.

cleanupEndedHdrVideos absent from hybrid loop

The sequential loop calls cleanupEndedHdrVideos after each frame to free disk for long HDR renders. The hybrid loop does not — because the gate ensures HDR content never reaches it. Correct by invariant, but if the gate is ever relaxed (e.g., to support HDR with per-worker dup(fd)), the hybrid loop will need its own cleanup pass. The PR description already flags dup(fd) as out-of-scope; this is just the cleanup side of that same note.

workerRanges with zero-width slices

distributeLayeredHybridFrameRanges can produce zero-width ranges for workers past the frame budget (pinned by the test at :142). In workerTaskOf, the for (let i = range.start; i < range.end; ...) loop body-less-exits cleanly. Good — no wasted worker session spawn though, because workerSessions always spawns workerCount - 1 sessions regardless. For very short compositions (e.g. 3 frames, 6 workers) this over-spawns 5 Chrome sessions that immediately no-op. The cpus() heuristic in calculateOptimalWorkers likely prevents this in practice, but a Math.min(workerCount, totalFrames) clamp on the session spawn count would be a cheap future optimization.

Solid decomposition. The file split is clean, the gate is conservative, and the sequential-path preservation is structurally enforced.

— Magi

The base branch was changed.

[miguel-heygen]

Review: Hybrid layered/parallel path for SDR shader-transition renders

Well-executed decomposition — 921-line file split into 5 cohesive modules, hybrid path conservatively gated, sequential path preserved byte-for-byte. The try/finally in runHybridLayeredFrameLoop guarantees cleanup of worker sessions and shader pool. 14 unit tests pin the gating predicate and partitioner. LGTM.

Observations (non-blocking)

1. Shared mutable state in HDR context (captureHdrHybridLoop.ts:~845): The shallow spread { ...hdrCompositeCtx, domSession: session } shares hdrVideoFrameSources (Map) by reference across workers. Safe today because the hybrid gate excludes HDR content, but the invariant is implicit — a comment noting the SDR-only assumption would prevent a future developer from relaxing the hasHdrContent gate without auditing concurrent access.

2. Worker over-spawning for tiny compositions (~line 710-721): workerCount - 1 sessions are always spawned regardless of totalFrames. For a 3-frame composition with 6 workers, 4 sessions sit idle. calculateOptimalWorkers likely prevents this in practice, but Math.min(workerCount - 1, totalFrames - 1) would be a cheap safety net.

3. cleanupEndedHdrVideos absent from hybrid loop: The sequential loop calls this per-frame to free disk for long HDR renders. Omission is correct for SDR-only, but another implicit invariant worth a one-line comment.

All three are non-blocking — the gate predicate makes them safe today. Approve.

[jrusso1020]

Re-approved at 75aa2d82 — carrying forward my prior APPROVE.

Per Vance's note, the dismissal-on-push is the new branch-protection policy doing its job, not a bug. Diff is functionally identical to my prior review at 3177cfa2 (1517/-603 across 7 files, same surfaces: captureHdrStage.ts orchestrator, captureHdrFrameShared.ts + 4 sibling modules for the file split, the hybrid loop, tests). The Graphite rebase touched commit SHAs without changing content.

Stack status

• hf#756 (worker-count cap, stack root): merged ✓
• hf#757 (PR 2/5, pngDecodeBlit pool): re-approved at ede5cde3 earlier today (me + Magi)
• hf#758 (PR 3/5, shaderTransition pool): re-approved at 525c5975 earlier today (me + Magi)
• hf#759 (this, PR 4/5, hybrid layered/parallel path): ready to re-approve

The stack ordering is clean — each predecessor is approved, this PR's runHybridLayeredFrameLoop correctly depends on the shaderTransition pool added in hf#758.

CI — comprehensively green at HEAD

This is the strongest possible signal for a +1517 LoC stage refactor:

• All 10 regression-shards green (fast, render-compat, hdr, styles a-g)
• HDR regression-shard specifically green — the load-bearing signal that the file split preserves HDR behavior (the gating predicate at captureHdrFrameShared.ts:43-56 keeps HDR on the sequential path; the HDR shard passing confirms it at the fixture level)
• Lint, Format, Test, Typecheck, Build, CLI smoke, Test: runtime contract ✓
• Render on windows-latest ✓, Tests on windows-latest ✓
• Preview parity, preview-regression, player-perf ✓
• CodeQL ✓

No reds anywhere.

Audited / Trusting framing (per the >2000 LoC rubric)

Same disposition as my prior review at 3177cfa2:

Audited:

• captureHdrFrameShared.ts end-to-end (350 lines — gating predicate + partitioner + per-scene capture primitive captureSceneIntoBuffer)
• captureHdrStage.ts end-to-end (469 lines — orchestrator + dual-flag cleanup invariants + dispatch logic)

Trusting (verified by CI green incl. HDR + regression suite):

• captureHdrSequentialLoop.ts (228 lines, byte-equivalent to prior inline path — structurally enforced via shared captureSceneIntoBuffer)
• captureHdrHybridLoop.ts (290 lines, new path with try/finally pool teardown)
• captureHdrResources.ts (289 lines, pre-existing logic relocated, not semantically modified)
• The 14 new tests in captureHdrFrameShared.test.ts

Advisory items still open (non-blocking, carried from prior reviews)

My prior observations

1. Worker count recomputation via calculateOptimalWorkers rather than threading through call signature — intentional zero-diff to orchestrator. Defensible trade-off.
2. partitionTransitionFrames(...).size allocates a Set just to read its size — transient, fires once per render.
3. Dispatch-decision logging at :280-287 is excellent — future debuggers will grep for "Layered hybrid dispatch decision" and immediately know the path.

Magi's prior observations

1. hdrPerf increment safety — confirmed safe (JS single-threaded + atomic increments).
2. transitionRanges.find() per-frame in hot loop — O(transitions) which is typically 1-3; gated behind transitionFramesSet.has(i) short-circuit.
3. Shallow-spread of hdrCompositeCtx shares hdrVideoFrameSources Map by reference across workers — safe via SDR-only gate; worth a comment if the gate is ever relaxed.
4. cleanupEndedHdrVideos absent from hybrid loop — correct by SDR-only invariant; same comment if HDR gate is ever relaxed.
5. Zero-width worker ranges over-spawn Chrome sessions on very short compositions — Math.min(workerCount, totalFrames) clamp would be a cheap future optimization.

None gate merge. All are follow-up items that age well as comments or micro-optimizations.

Verdict

APPROVE re-affirmed. Land paired with hf#757 + hf#758 in stack order. Ship 🚀

Review by Rames Jusso (pr-review)

[vanceingalls]

Merge activity

• May 14, 3:38 AM UTC: @vanceingalls merged this pull request with Graphite.

[miguel-heygen]

Re-stamping after Graphite restack. LGTM.