fix: improve auto worker handling for webgl-heavy renders#411
fix: improve auto worker handling for webgl-heavy renders#411miguel-heygen wants to merge 2 commits intomainfrom
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Staff-level review — overall a reasonable, well-tested change. Delegating auto resolution to the producer (one source of truth) is a clear win. A few points, in priority order.
1. Does this actually fix #410?
The detection in detectRenderModeHints (packages/producer/src/services/htmlCompiler.ts:94-107) only fires on raw requestAnimationFrame(...) inside inline <script> tags — external src= is explicitly skipped.
Issue #410 describes "one GSAP timeline to orchestrate text staggers, burst rings, and camera orbits." GSAP drives its own render loop through its ticker and Three.js is loaded as a module, so the motivating composition almost certainly doesn't trigger this hint. A user reproducing that scene still gets auto = 6 and still times out.
Two paths forward:
- Narrow the framing. Rename the constant, update the PR title, log message, timeout-retry string, and docs from "WebGL-heavy" → "raw rAF screenshot-mode renders." Then follow up with real WebGL detection (scan compiled HTML for
canvas.getContext('webgl'/WebGLRenderingContext). - Rethink the default. There's a separate conversation about lowering the
autodefault globally (e.g.default = 2, opt-in to more via--workers N/HYPERFRAMES_WORKERS) given user complaints about CPU/fan noise. If that lands, this compat path becomes dead weight — the default would already sit at the cap.
My preference: land this as the narrower fix (rename + scope), and open a separate issue for whether calculateOptimalWorkers is the right shape of default for a dev tool running on laptops.
2. Naming over-promises — renderOrchestrator.ts:399
const SCREENSHOT_MODE_AUTO_WORKER_CAP = 2;This gates on requestAnimationFrame only — iframe-triggered screenshot mode is not capped, as your third test case confirms. Rename → RAF_SCREENSHOT_AUTO_WORKER_CAP (or similar) so the name matches behavior. Otherwise the next person reading this will expect iframe-induced screenshot mode to hit the same cap.
3. Magic 2 needs a comment
No justification anywhere for why 2 specifically. Was this benchmarked, or taken from OP's "--workers 2 ran cleanly" observation? A one-line comment (e.g. "2 is the highest count that reliably avoids Chrome compositor starvation on GPU-heavy scenes — benchmarked / observed in #410") helps future tuners.
4. Docs regressed in specificity — docs/guides/rendering.mdx
The old table ("MacBook Air M1 → 4 workers") was concrete and testable. The replacement bullets ("longer renders scale up based on CPU, available memory, and total frame count") don't give a user any way to predict what auto will pick. Users troubleshooting now see "workers auto (frame-aware heuristic)" in the render plan with no number until the render actually starts.
Consider either (a) keeping rough guidance (~3 cores per worker, cap 6), or (b) resolving the worker count at plan-print time so the banner shows "auto → 4 workers".
Smaller nits
- Surprise-factor log (
renderOrchestrator.ts:417) —log.info("Reduced auto worker count...")for a user who opted intoautois a user-visible behavior change. Worth surfacing in the render-plan banner, not just structured log output. A user seeing "auto picked 6, reduced to 2 (rAF screenshot-mode)" in the banner will understand immediately; a log line they may never read won't explain why their render is slower than expected. - Timeout-retry message (
renderOrchestrator.ts:2393) — advising--workers 2first means video-heavy users pay an extra failed retry before being nudged to--workers 1. Acceptable trade-off, but worth noting in release notes. - Test helper duplication —
createCompiledCompositionis now defined in bothdescribeblocks inrenderOrchestrator.test.ts. Hoist it to module scope. - CLI
"auto"string is dead text —render.ts:145treats--workers autoidentically to no arg (both leaveworkers = undefined). Consider dropping"auto"from--helpand the CLI surface; it's a no-op option that only adds documentation surface. (Out of scope for this PR, but trivially doable in a follow-up.)
Risk is low — workers?: number is additive, blast radius is limited to users whose auto currently resolves to >2 on rAF scenes (who now get slower-but-more-reliable renders). Nothing here is blocking.
|
Addressed the main review concern in |
miguel-heygen
changed the title
miguel-heygen
force-pushed
the
codex/issue-410-worker-auto
branch
from
226b363 to
32f85bf
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving — the rewrite addressed the main concern (WebGL/Three.js are now detected via real getContext('webgl') + Three identifiers + external script sources, and the motivating CDN-loaded Three composition from #410 will now trigger the cap). Magic 2 documented, function name aligned, test helper hoisted.
One ask before merge: please rebase onto current main (your branch is ~156 commits behind). The real diff is small and shouldn't conflict, but the GitHub PR view is currently showing a misleading delete-everything view against current main.
Three small follow-up nits inline — non-blocking.
| }); | ||
| } | ||
|
|
||
| // Issue #410 reported that 2 workers stayed stable for GPU-heavy scenes where |
There was a problem hiding this comment.
Minor: now that the cap also applies to requestAnimationFrame hints (not just WebGL), the constant name WEBGL_AUTO_WORKER_CAP is slightly misleading — a future reader looking at this line will think the cap only fires for WebGL.
No strong preference, but consider either:
COMPAT_HINT_AUTO_WORKER_CAP(matches the function nameapplyAutoWorkerCompatibilityHints), orGPU_HEAVY_AUTO_WORKER_CAP(covers both Chrome compositor cases).
Same applies to the log message at L417 (Reduced auto worker count for WebGL-heavy composition) — when the trigger was requestAnimationFrame only, calling it "WebGL-heavy" is inaccurate. Including reasonCodes in the log payload helps, but the headline string still misleads. Could be: Reduced auto worker count for compatibility-flagged composition and let reasonCodes describe the why.
| log.warn( | ||
| `Parallel capture timed out with ${job.config.workers ?? "auto"} workers. ` + | ||
| `Video-heavy compositions often need sequential capture. Retry with --workers 1`, | ||
| `WebGL-heavy scenes often need lower parallelism. Retry with --workers 2 first; ` + |
There was a problem hiding this comment.
Same naming nit: "WebGL-heavy scenes often need lower parallelism" can fire on rAF-only timeouts where no WebGL was involved. Worth softening to e.g. GPU- or rAF-heavy scenes often need lower parallelism. Retry with --workers 2 first… so users with raw rAF compositions don't go looking for nonexistent WebGL.
| - very short renders stay single-worker, because parallel startup overhead outweighs the gain | ||
| - longer renders scale up based on CPU, available memory, and total frame count | ||
| - very large renders are capped again to avoid Chrome/FFmpeg contention | ||
|
|
There was a problem hiding this comment.
Soft nit, not blocking: dropping the concrete MacBook Air M1 → 4 table for vague bullets ("longer renders scale up based on CPU…") loses something for users trying to predict what auto will pick before they hit start. The render plan banner now reads workers auto (frame-aware heuristic) with no number.
Cheaper-than-table option: resolve the worker count at plan-print time and show auto → 4 workers (rAF-heavy: 2) in the banner. Producer-side already has the math; CLI just needs to call it. Worth a follow-up issue if you don't want to scope-creep this PR.
| const workers = applyAutoWorkerCompatibilityHints( | ||
| 6, | ||
| { fps: 30, quality: "standard" }, | ||
| createCompiledComposition(["iframe"]), |
There was a problem hiding this comment.
Tiny: this test description still says unrelated to requestAnimationFrame, but the function now also caps for webgl. Update to unrelated to requestAnimationFrame or webgl (or just compatibility hints to match the function name).
2 participants
Summary
--workers autothrough the CLI and Docker paths so the producer/container resolves worker count instead of the CLI forcing a CPU-only defaultrequestAnimationFrame, then cap auto-selected workers at2for those GPU-heavy cases unless the user explicitly overrides workersFixes #410
Verification
bun run --filter @hyperframes/cli typecheckbun run --filter @hyperframes/producer typecheckbun run --filter @hyperframes/cli test src/utils/dockerRunArgs.test.tsbun test packages/producer/src/services/htmlCompiler.test.tsbunx vitest run packages/producer/src/services/renderOrchestrator.test.tsbunx oxlint packages/cli/src/commands/render.ts packages/cli/src/utils/dockerRunArgs.ts packages/cli/src/utils/dockerRunArgs.test.ts packages/cli/src/telemetry/events.ts packages/producer/src/services/htmlCompiler.ts packages/producer/src/services/htmlCompiler.test.ts packages/producer/src/services/renderOrchestrator.ts packages/producer/src/services/renderOrchestrator.test.tsrequestAnimationFramecomposition:render --workers autologged reduction from6to2workers and captured at2 workersreasonCodes:["webgl"], thenrender --workers autologgedReduced auto worker count for WebGL-heavy composition {"from":6,"to":2}and captured at2 workersagent-browseragainst local studio preview athttp://localhost:5191/#project/demo, including recording/tmp/hf-410-vXuYvI/preview-pass.webmand screenshot/tmp/hf-410-vXuYvI/preview-shot.pngNotes
bun run packages/cli/src/cli.ts validate /tmp/hf-410-vXuYvI/democurrently fails in this checkout withMissing 'default' export in .../packages/cli/src/commands/contrast-audit.browser.js; that appears unrelated to this patch and did not block the render/browser verification above.