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
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 |
226b363 to
32f85bf
Compare
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.