refactor(producer): extract captureHdrStage (HDR / shader-transition path)#733
Conversation
…path) Move the Z-ordered HDR / shader-transition layered composite branch (`if (useLayeredComposite)`) out of `executeRenderJob` into `services/render/stages/captureHdrStage.ts`. The largest extraction by LOC (~745 lines of body lifted verbatim) and the riskiest by cleanup invariants. Body is lifted byte-for-byte — only the surrounding scope changes. Cleanup invariants preserved verbatim (design doc §11 flagged these explicitly): - `hdrEncoderClosed` / `domSessionClosed` flags gate the defensive-close paths so they don't run twice when the success path already closed. - `hdrVideoFrameSources` is drained + cleared in the outer `finally` regardless of how the body exited. - `cfg.forceScreenshot = true` is set unconditionally inside the layered path because `captureAlphaPng` hangs under `--enable-begin-frame-control`. Other invariants preserved: - `hdrPerf` is created at the top of the stage and returned; the sequencer's `finalizeHdrPerf` consumes it for the perf summary. - The `Layered compositing frame N/M` `updateJobStatus` payload fires at the same per-frame point with `25 + frameProgress * 55`. - `composition` and `compiled` are read-only in the stage. - `hdrDiagnostics` is mutated in place (counters incremented at the same code points). - `nativeHdrIds` is recomputed inside the stage from `nativeHdrVideoIds` + `nativeHdrImageIds` (the sequencer's computation is unchanged; the stage just doesn't need it passed in). To support the extraction, the following symbols are newly exported from `renderOrchestrator.ts`: - Helper functions: `createHdrPerfCollector`, `addHdrTiming`, `closeHdrVideoFrameSource`, `blitHdrVideoLayer`, `blitHdrImageLayer`, `compositeHdrFrame`. - Types: `HdrPerfCollector`, `HdrPerfTimingKey`, `HdrVideoFrameSource`, `HdrImageBuffer`, `HdrCompositeContext`, `HdrTransitionMeta`, `TransitionRange`. These are internal helpers — the stage is currently the only consumer, and the cycle (orchestrator imports `runCaptureHdrStage`; stage imports helpers back) is safe at runtime. A future PR will consolidate the helpers into a shared module (same follow-up planned for the capture helpers in PRs 1.6 and 1.7). Removes the now-orphaned imports from the orchestrator: `openSync`, `fpsToFfmpegArg`, `spawnStreamingEncoder`, `StreamingEncoder` type, `runFfmpeg`, `initTransparentBackground`, `decodePngToRgb48le`, `queryElementStacking`, `TRANSITIONS`, `crossfade`, `resampleRgb48leObjectFit`, `normalizeObjectFit`, `TransitionFn` type, `createHdrImageTransferCache`. Verified inside `Dockerfile.test`: - **HDR fixtures (3/3 PASS)**: hdr-regression, hdr-hlg-regression, vignelli-stacking — audio correlations 1.000 / 1.000 / 0.982. - **Non-HDR fixtures (4/4 PASS)**: font-variant-numeric, many-cuts, sub-composition-video, gsap-letters-render-compat — audio correlations 1.000 / 0.994 / 0.947 / 1.000. - 7/7 fixtures total pass with PSNR / audio correlations matching every prior PR in the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: approve.
Largest extraction in the stack (~750 lines of body lifted into captureHdrStage.ts). Highest risk of overlap with active work — see the cross-stack note below. Despite the size, the diff is byte-clean against the pre-#733 if (useLayeredComposite) branch.
Verified against the pre-#733 orchestrator:
- Sequential
for (let i = 0; i < totalFrames; i++)loop preserved verbatim. No restructure — the dual-scene transition path, the normal-frame composite path, and the in-loopcloseHdrVideoFrameSourcecleanup all sit at the same indentation depth, in the same order, with the same per-frameaddHdrTimingcalls. - All three cleanup invariants from the JSDoc preserved:
hdrEncoderClosed/domSessionClosedflags gate the outerfinally;hdrVideoFrameSourcesis drained + cleared in the outerfinally;cfg.forceScreenshot = trueset unconditionally at the top of the stage (line 205 of the new file). nativeHdrIds = new Set([...nativeHdrVideoIds, ...nativeHdrImageIds])recomputed inside the stage — same expression the sequencer used pre-extraction. The two input maps (nativeHdrVideoIds,nativeHdrImageIds) flow in cleanly viaCaptureHdrStageInput.- Six private helpers (
createHdrPerfCollector,addHdrTiming,closeHdrVideoFrameSource,blitHdrVideoLayer,blitHdrImageLayer,compositeHdrFrame) + eight types newly exported fromrenderOrchestrator.ts. The helper bodies stay in the orchestrator — same runtime-cycle pattern as #730 / #731, acknowledged in the PR body. Safe at runtime; the follow-up consolidation #737 only addressesupdateJobStatus. - Stage interface diverges from #730 / #731 —
captureHdrStagedoes not takecaptureAttemptsorneedsAlpha(HDR path is single-session and locked to rgb48le), but addsprojectDir,compiledDir,composition,hasHdrContent,effectiveHdr,nativeHdrVideoIds,nativeHdrImageIds,videoTransfers,imageTransfers,hdrImageSrcPaths,hdrDiagnostics. The divergence is justified by the HDR pipeline's domain (z-order queries, raw rgb48le extraction, transfer-curve resolution) and isn't gratuitous. However: it does mean the three capture stages don't share an interface even though they conceptually do the same job. Worth a follow-up to factor a commonCaptureStageBaseshape — but explicitly not for this PR.
Cross-stack — coordinate with hf#732 (the hybrid layered/parallel path for shader transitions):
hf#732 is open against main, mergeable, and modifies the same if (useLayeredComposite) body that #733 extracts. The fix introduces partitionTransitionFrames, shouldUseHybridLayeredPath, and per-frame helpers (processLayeredNormalFrame, processLayeredTransitionFrame) that route non-transition frames through a pool of additional Chrome sessions instead of serializing every frame on the legacy single-session loop.
If #733 lands first, hf#732 has to be rewritten against the new captureHdrStage interface (the hybrid path moves inside the stage, the dual-scene loop becomes the slow fallback). If hf#732 lands first, #733's "byte-clean extraction" rebases on top of the now-hybridized body — the extraction becomes a cleanup of the hybrid path, not an extraction of the pre-fix sequential loop.
Recommend merge order: hf#732 first, then #733 second.
Reasoning: (1) hf#732 is small and targeted; #733 is the biggest PR in the stack. Rewriting hf#732 against captureHdrStage's stage interface is ~5× more work than rebasing #733 over hf#732's pre-extracted hybrid path. (2) hf#732 has user-validation pending for issue #677 — landing the extraction first gates the user fix behind 8 PRs of refactor that have no observable customer benefit. (3) The extraction is mechanically simpler to redo on a hybridized body than the hybrid path is to redo on an extracted stage.
If James prefers #733 first because the stack rebase cost dominates the rewrite cost on his end, that's fine — but the rewrite needs to land before this stack ships to keep the user fix in scope.
Important (carry-forward, non-blocking):
captureHdrStage.ts:205—cfg.forceScreenshot = trueis now a stage-level side effect. The mutation moved with the code, but pure stages can't mutate inputs if they're going to be retry-safe / replay-safe. Same concern flagged on the foundation stack (applyRenderModeHintsin #718). This is the largest stage now and the worst place to leave the mutation un-fixed. Recommend a follow-up to either (a) returnforceScreenshot: truein the result and have the sequencer flip it, or (b) build a stage-local cfg copy with the flag pre-set. Not blocking #733, but it's the highest-priority cleanup before chunk rendering, where retry-on-stage-failure makes the mutation unsafe.- No per-stage observability hook (tracer, log scope) yet.
captureHdrStageis now the most complex stage in the codebase — every other stage has at most ~10 timing buckets, this one has 16 viaHdrPerfCollectorplus inlinelog.info/log.debugcalls. If we're going to land per-stage tracing instrumentation, this is the stage that benefits most. Foundation-stack feedback recommended landing it with the first stage that benefits; that stage is now here. Worth scoping into a follow-up before chunk-rendering plugs inchunkStage.
Nits:
captureHdrStage.ts:871—Layered composite frame ${i + 1}/${job.totalFrames}usesjob.totalFrameswhile the loop bound usestotalFrames. Pre-extraction the orchestrator did the same thing; the values are equivalent (post-probe narrowing). Same drift hazard as #731 — worth a tiny test pinning the format.- The 14 dropped orchestrator imports (
openSync,fpsToFfmpegArg,spawnStreamingEncoder,StreamingEncoder,runFfmpeg,initTransparentBackground,decodePngToRgb48le,queryElementStacking,TRANSITIONS,crossfade,resampleRgb48leObjectFit,normalizeObjectFit,TransitionFn,createHdrImageTransferCache) are all flagged by oxlint per the PR description. Clean diff hygiene.
Praise: the HDR-stage extraction is the keystone of the refactor and the highest-stakes change in the stack — the cleanup invariants, the dual-scene transition compositing, the in-loop frame-source GC, the per-frame timing buckets, the defensive-close finally with two flag-gated paths — every piece preserved verbatim. The PR description's "Preserved cleanup invariants (the risky ones)" section is exactly the right framing for a change of this size; that section is what makes the extraction reviewable in finite time.
— Vai

What
Phase 1 PR 1.8 of the distributed-render refactor. Largest extraction in the stack (~745 lines of body lifted verbatim) and the one with the most cleanup-risk per the design doc.
Moves the Z-ordered HDR / shader-transition layered composite branch out of
executeRenderJobintopackages/producer/src/services/render/stages/captureHdrStage.ts. The sequencer now callsrunCaptureHdrStagewhenuseLayeredComposite === true.The standard SDR / DOM-only-HDR capture paths (PR 1.6 disk, PR 1.7 streaming) are unchanged — they live in the
elsebranch.Why
Final capture-path extraction. After this, the only capture work left in
executeRenderJobis the in-sequencer setup (file-server init, calibration, worker resolution, preset selection) and the three stage calls. PR 1.9 will extract the encode + assemble stages, and PR 1.10 reduces the sequencer to a thin composition.How
captureHdrStage.tsexportsrunCaptureHdrStage(input) → CaptureHdrStageResult. The function body is the originalif (useLayeredComposite)body lifted byte-for-byte with only these substitutions:hdrPerf = createHdrPerfCollector()→ uses the function-scopelet hdrPerf.perfStages.captureMs = Date.now() - stage4Start→captureDurationMs = Date.now() - stageStart(stage's own internal timer).perfStages.encodeMs = hdrEncodeResult.durationMs→encodeMs = hdrEncodeResult.durationMs.renderOrchestrator.tsare newly exported for the stage to call:createHdrPerfCollector,addHdrTiming,closeHdrVideoFrameSource,blitHdrVideoLayer,blitHdrImageLayer,compositeHdrFrame. The helper bodies remain in the orchestrator (they don't depend onexecuteRenderJob's local scope; they'll move to a shared module in a follow-up PR).HdrPerfCollector,HdrPerfTimingKey,HdrVideoFrameSource,HdrImageBuffer,HdrCompositeContext,HdrTransitionMeta,TransitionRange.if (useLayeredComposite) { ... 745 lines ... }body with a call torunCaptureHdrStage(...). AssignslastBrowserConsole,hdrPerf,perfStages.captureMs,perfStages.encodeMsfrom the result.openSync,fpsToFfmpegArg,spawnStreamingEncoder,StreamingEncodertype,runFfmpeg,initTransparentBackground,decodePngToRgb48le,queryElementStacking,TRANSITIONS,crossfade,resampleRgb48leObjectFit,normalizeObjectFit,TransitionFntype,createHdrImageTransferCache.Preserved cleanup invariants (the risky ones)
hdrEncoderClosed/domSessionClosedflags gate the defensive-close paths in the outerfinallyso they don't run twice when the success path already closed.hdrVideoFrameSourcesis drained + cleared in the outerfinallyregardless of how the body exited. Each frame source is closed viacloseHdrVideoFrameSourceso file descriptors don't leak.cfg.forceScreenshot = trueis set unconditionally inside the layered path becausecaptureAlphaPnghangs under--enable-begin-frame-control.hdrPerfcollection is identical — sameaddHdrTimingcall sites, sameframes/domLayerCaptures/hdrVideoLayerBlits/hdrImageLayerBlitscounters.hdrDiagnosticsmutations (videoExtractionFailures, imageDecodeFailures) happen at the same code points.compositionandcompiledremain read-only — no surprise mutations on the orchestrator's owned objects.nativeHdrIdsis recomputed inside the stage fromnativeHdrVideoIds+nativeHdrImageIds(same expression as the sequencer used pre-extraction).Test plan
bunx oxlint+bunx oxfmt --check— clean.bun run --filter @hyperframes/producer typecheck+build— clean.bun test packages/producer/src/services/— 176 pass, same single pre-existing unrelated failure.Dockerfile.test— 3/3 PASS:hdr-regression— audio correlation 1.000hdr-hlg-regression— audio correlation 1.000vignelli-stacking— audio correlation 0.982 (exercises the shader-transition layered composite path)font-variant-numeric(1.000),many-cuts(0.994),sub-composition-video(0.947),gsap-letters-render-compat(1.000). All correlations identical to prior PRs in the stack.regressionworkflow.Known follow-up: import cycle
Same situation as PR 1.6 / PR 1.7 — the stage imports runtime helpers (
updateJobStatus, the six HDR helpers) and types fromrenderOrchestrator.ts, which importsrunCaptureHdrStageback. Safe in practice (deferred to runtime); a future PR will consolidate these helpers into a shared module once all 8 stages are extracted.🤖 Generated with Claude Code