fix(runtime): honor render fps for deterministic seeks#1739
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 40fb2bcfda9d4e6ccb4e7a409e4c7be161e4a978. Layering on @viavoyageragent and the HF-runtime-interop lane — I'm focusing on cross-PR coherence with the render-reliability arc (#1725, #1726), producer entry-point parity, telemetry, and the test-environment shape. Wiring + algorithm choice both look solid; flagging one likely-out-of-scope-but-worth-deciding parity gap in the catalog/template preview scripts, plus a few smaller observations.
⚠️ scripts/generate-catalog-previews.ts:241 + scripts/generate-template-previews.ts:144 — preview scripts call createFileServer({ projectDir, port: 0 }) with no fps, so they silently fall back to 30
Both preview generators import createFileServer from @hyperframes/producer (same code path you wired) but don't pass fps. Catalog previews pass fps: { num: 30, den: 1 } to createCaptureSession and template previews pass fps: 30, but those are the capture-session output fps, not the file-server config injection. If the wrapper's data-fps (or the underlying compositions) ever target anything other than 30, the same DOM-on-30-grid-inside-60-container duplicate-frame artifact you fixed for production renders reappears in the marketing-surface thumbnails. Today they look 30fps-only based on hardcoded constants, so this is probably fine — but per feedback_dispatch_map_silent_drop_default_types the silent fallback is the same shape as the bug you just removed for the producer path. Worth either passing fps explicitly or adding a sibling unit test that asserts the preview path's __renderFps matches the wrapper's authored fps. Question: intentional out-of-scope (since preview renders are 30fps by convention) or a follow-up?
⚠️ Telemetry: snap-applied / snap-amount is not surfaced
init.ts:51 does state.canonicalFps = resolveExportRenderFps() ?? state.canonicalFps; silently. If a future producer caller forgets to thread fps (the catalog scripts above are one example today), the runtime silently falls back to 30 again — and we won't know until the YDIF probe catches it post-render. Per feedback_observability_pr_failure_path_coverage, this PR has a free-leverage moment to emit one signal: "render-mode runtime initialized — canonicalFps=N, source=export-config|default". One console.info / postMessage on the failure path (config absent, finite-but-zero, NaN) tells SRE which producer entry point is mis-wired without needing a YDIF round-trip. Cheap; saves the next debug arc.
🟡 Test shape — init.test.ts:150 asserts the wired path, not the originally-broken behavior
The new test sets fps: 60, calls renderSeek(1/60), and asserts timeline.time() is close to 1/60. That validates the config is being read. It does NOT pin the original bug: without the wire-up, renderSeek(1/60) at canonicalFps=30 would quantizeTimeToFrame(1/60, 30) = floor(0.5 + 1e-9)/30 = 0. A second case asserting timeline.time() is 0 when __HF_EXPORT_RENDER_SEEK_CONFIG is unset (i.e. proving the pre-fix behavior the test would regress to) makes this a true pin. Per feedback_parity_test_reflexivity_tautology — and worth noting parity-harness.ts:240-260 pre-quantizes externally so the existing parity harness wouldn't have caught the original bug either (it bypasses state.canonicalFps entirely). Not blocking, but the asymmetry between "what the parity harness tests" and "what real renders do" is worth a note in the test file.
🟡 init.ts:51 — ?? state.canonicalFps falls back to 30 silently when the config has fps: 0 / fps: NaN
resolveExportRenderFps() returns null on non-finite or ≤0 fps, and the fallback chain lands at the constructor default 30. This is the safe behavior — but combined with the no-telemetry above, a producer caller that wires fps: { num: 0, den: 1 } (or a future fpsToNumber that returns 0 on some new rational input) gets the silent-30 bug back with no signal. Same feedback_dispatch_map_silent_drop_default_types shape. The producer side (fileServer.ts:319-322 normalizeRenderFps) also defaults to 30 on missing/invalid Fps, so the silent fallback is two-deep. One emit on the producer-side fallback path would catch most of it.
💭 Cross-PR coherence with #1725 / #1726 — verified clean
I checked both directions:
- #1725 (PREEMPT zero-progress retries): bails on
findMissingFrameRangesreturning a nonzero count. Frame-range detection is based on extracted PNG/MP4 frames at the capture-session level, not onstate.canonicalFps. The snap-direction (Math.floor + 1e-9epsilon) doesn't shift the disk-frame index either way; #1725's gate is orthogonal. - #1726 (hold-last-frame on short media): trigger is per-clip on
media.duration < slot.duration, evaluated in the media-clip subsystem — not viaquantizeTimeToFrame. Even if the snap shifts the global seek time by<1/fps, the freeze trigger comparison is on clip-internal time, so no over/under-cross of the freeze threshold.
So the three render-reliability PRs compose cleanly: #1725 prevents wasted retries, #1726 prevents the trailing-black-flash, this one fixes the every-other-frame-duplicate on non-30 outputs. Good arc.
💭 Snap algorithm — feedback_inverse_operation_tolerance_asymmetry
quantizeTimeToFrame at parityContract.ts:35 is Math.floor(t * fps + 1e-9) / fps — consistent floor with epsilon. No inverse-op (seek-out / loop-wrap / pause-resume) on the renderSeek path, so the asymmetry concern doesn't apply here. Negative/NaN time clamps to 0 via the safeTime > 0 guard. Multi-fps composition resolves to container fps (the right call — we're rendering INTO the container grid, not the clip grid).
✅ Confirmed solid
resolveExportRenderFpsvalidates withNumber.isFinite+> 0before adopting.- All three producer entry points (
renderOrchestrator.ts:1230,probeStage.ts:169,renderChunk.ts:503) threadfpsthrough. Probe stage matching the main render is the right call — keeps probe-vs-render parity (the probe's per-frame DOM samples now match what real capture will see). - Engine package's
createFileServeris a separate code path (packages/engine/src/services/fileServer.ts) that doesn't injectRENDER_MODE_SCRIPTat all — not a parity gap, just a different surface. window.d.tsdeclaration is properly typed for the newfpsfield on the export-config object.
What I didn't verify
- Whether the catalog/template preview compositions actually have non-30fps wrappers in production today (would convert
⚠️ to 🛑 if yes; converts to 💭 if confirmed 30-only forever). - Whether
cli/src/commands/snapshot.ts:294(the otherrenderSeekconsumer) usesserveStaticProjectHtmlinstead ofcreateFileServer— eyeballed at lines 9, 105 and yes it does, so out of scope of this PR. Worth a separate parity audit if non-30 snapshots matter.
— Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Fixes the duplicate-every-other-frame bug from #1737 by closing the producer→runtime fps coordination gap: the producer injects the actual render fps into window.__HF_EXPORT_RENDER_SEEK_CONFIG.fps, and the runtime adopts it as state.canonicalFps during init so renderSeek's quantization grid matches the container fps.
The mechanism:
createFileServer({ fps })→buildRenderModeScript(fps)interpolatesvar __renderFps = ${renderFps}and setswindow.__HF_EXPORT_RENDER_SEEK_CONFIG.fps = __renderFpsin the injected body script (fileServer.ts:312-336).- Three producer call sites pass fps:
renderOrchestrator.ts:1227(in-process capture),probeStage.ts:166(probe),distributed/renderChunk.ts:500(distributed chunks). - Runtime reads it inside
initSandboxRuntimeModular(init.ts:43-51):state.canonicalFps = resolveExportRenderFps() ?? state.canonicalFps— keeps the 30 default when the config is absent or invalid.
Verdict: LGTM at 40fb2bcfda9d4e6ccb4e7a409e4c7be161e4a978. Rames also reviewed in parallel (~30s before me); he caught the preview-script silent-fallback shape and the observability angle, both worth addressing. My findings below complement his — distinct angles on chunk-path consistency, dead __HF_FPS field, and broader-scope consequences — plus a precision on his catalog/template finding.
Boot-order check (showing the work, not a finding)
The whole fix depends on the producer body-script setting __HF_EXPORT_RENDER_SEEK_CONFIG before initSandboxRuntimeModular reads it. Traced:
entry.ts:bootstrapHyperframeRuntimeis gated onDOMContentLoaded:if (document.readyState === "loading") addEventListener("DOMContentLoaded", ..., { once: true }); else bootstrap().- Producer-served HTML injects the runtime in
headScripts(inline in<head>) andRENDER_MODE_SCRIPTinbodyScripts(before</body>). - During HEAD parsing, runtime-source executes synchronously, sees
readyState === "loading", registers the DOMContentLoaded listener — does NOT yet call init. - During BODY parsing,
RENDER_MODE_SCRIPTruns synchronously and setswindow.__HF_EXPORT_RENDER_SEEK_CONFIG = { ..., fps: __renderFps, owner: "runtime" }. - After
</body>, DOMContentLoaded fires →bootstrapHyperframeRuntime()→initSandboxRuntimeModular()→ reads__HF_EXPORT_RENDER_SEEK_CONFIG.fps. The config IS set at this point.
The else bootstrap() immediate path (runtime loaded post-DOMContentLoaded) would NOT pick up a later-set config, but in the producer context this branch is never taken because runtime is always inline in head. In preview/studio contexts there's no __HF_EXPORT_RENDER_SEEK_CONFIG at all, so resolveExportRenderFps() returns null and the default 30 stays. No regression path.
Scope is broader than the PR title suggests (informational, not a finding)
state.canonicalFps has more consumers than just player.renderSeek. The grep at 40fb2bcf:
init.ts:1683—postState: bridgeframe = round(currentTime * canonicalFps)init.ts:1735—collectRuntimeTimelinePayload({ canonicalFps })init.ts:1754— clipTreerootDuration: payload.durationInFrames / canonicalFpsinit.ts:1930—getCanonicalFps()accessorinit.ts:1985— bridgetime = frame / canonicalFpsinit.ts:2646—player.seekquantization (non-render path)init.ts:2668—player.renderSeekquantization (the targeted one)
All of these previously assumed 30fps when the output container was 60fps. This PR fixes them all in one motion — a correctness improvement across the runtime, not just renderSeek. The self-consistency at the clipTree-rootDuration site holds because durationInFrames = ceil(safeDuration * canonicalFps) in timeline.ts:663-665, so durationInFrames / canonicalFps ≈ safeDuration at any fps. Worth a one-line note in the PR description (or a follow-up doc update) so future readers tracing canonicalFps consumers understand the widened scope.
NIT findings (non-blocking)
-
NIT (precision on Rames's catalog/template preview finding) — Verified both
scripts/generate-catalog-previews.ts:241andscripts/generate-template-previews.ts:144callcreateFileServer({ projectDir, port: 0 })with nofps. The structural concern Rames raised holds — the silent default-to-30 is the same shape as the bug this PR removes. But it does NOT reproduce as a bug today: both files use thatcreateFileServeronly for the thumbnail (single-frame) path, where the surroundingcreateCaptureSessionis hard-coded to 30fps (lines 246 + 149). The 24fps video paths (lines 279, 176) go throughcreateRenderJob+executeRenderJob, which routes throughrenderOrchestrator.ts:1227— that IS wired in this PR. So the catalog/template preview thumbnails are accidentally-correct at 30fps but the call-site is one constant-edit away from regressing. Two cheap fixes:- Pass
fps: { num: 30, den: 1 }explicitly at both call sites (makes the invariant explicit). - Or factor a
createPreviewFileServer({ projectDir, fps })helper that requires fps as a non-optional parameter.
Either closes the silent-default vector without expanding this PR's scope. Concur with Rames on raising it; out-of-scope for this PR, in-scope for a small follow-up.
- Pass
-
NIT (test coverage gap — fractional fps) — The producer's
normalizeRenderFpsusesfpsToNumber({ num, den })which produces irrational values for NTSC rates:{ num: 24000, den: 1001 }→23.976023976023978. Template-interpolated into JS source asvar __renderFps = 23.976023976023978.quantizeTimeToFrame(time, 23.976...)works but is untested —init.test.tsonly exercises the integer-60 case. Add a test with a fractional fps to lock in the contract; otherwise a future tightening ofnormalizeRenderFps(e.g., rounding to integer) would silently regress NTSC renders. -
NIT (test coverage gap — fallback paths; convergent with Rames's #3 + #4) —
fileServer.test.ts:226verifiesfps: { num: 60, den: 1 }injects the right tokens, but no test pins the fallback shapes:fps: undefined→__renderFps = 30fps: { num: 0, den: 1 }→__renderFps = 30fps: { num: 60, den: 0 }→__renderFps = 30(div-by-zero infpsToNumber)
Similarly
init.test.tsonly covers the happy path; no test pins "missing__HF_EXPORT_RENDER_SEEK_CONFIG→ canonicalFps stays 30". This converges with Rames's "test asymmetry" point — his framing (assert pre-fix behavior under the unset config to make it a real regression pin) is the cleaner way to express it. Either framing closes the same gap. -
NIT (chunk path consistency — distinct from Rames's reads) —
renderChunk.ts:500constructs the fps inline as{ num: plan.dimensions.fpsNum, den: plan.dimensions.fpsDen }, while the orchestrator (1227) and probe (166) passjob.config.fpsdirectly. Worth confirmingplan.dimensions.fpsNum/fpsDenis sourced fromjob.config.fpsupstream — if the distributed plan dimensions could ever drift from the in-process config fps (e.g., if a planner downsamples/upsamples for chunk scheduling), the runtime's canonicalFps would diverge between in-process and distributed paths. If they're always equivalent, a one-line// invariant: plan.dimensions.fps === job.config.fpscomment or a shared helper avoids divergence-by-edit risk. -
NIT (relationship to existing
__HF_FPS) —window.d.ts:42already declares__HF_FPS?: numberandskills/website-to-video/references/capabilities.mddocuments it as "Render FPS hint." Repo-wide grep: only consumer ispackages/core/scripts/debug-timeline.ts(read viaextractWindowNumber(html, "window.__HF_FPS")); nothing in production code sets it. This PR adds a parallel mechanism (__HF_EXPORT_RENDER_SEEK_CONFIG.fps) instead of extending__HF_FPS. Reasonable choice (the seek-config is more cohesive), but the duplication will confuse readers. Either:- Add a short comment near the new code clarifying
__HF_FPSis debug-only and__HF_EXPORT_RENDER_SEEK_CONFIG.fpsis the canonical runtime channel, OR - Follow up with a removal of
__HF_FPSfromwindow.d.ts+ the capabilities doc +debug-timeline.ts.
- Add a short comment near the new code clarifying
-
NIT (
ownerfield readers) —RENDER_MODE_SCRIPTsetsowner: "runtime"in the config object. No consumer grep-matches__HF_EXPORT_RENDER_SEEK_CONFIG.*ownerin the runtime or producer. If it's intended for telemetry/debugging tag, fine; if it's load-bearing somewhere I missed, worth confirming. Otherwise it could be dropped from the type and the injected object to reduce noise. -
NIT (typing strictness) —
__HF_EXPORT_RENDER_SEEK_CONFIGis typed withmode?: string(not a union of valid modes like"strict-boundary" | "default"),step?: number(no positivity constraint), etc. The producer's env-var validation is stricter than the type. Trivially correct as-is; tightening the type would catch malformed config at compile time on future readers.
On Rames's observability ask
Concur. The fallback chain is two-deep (producer-side normalizeRenderFps → 30; runtime-side resolveExportRenderFps() ?? state.canonicalFps → 30), and both silently swallow malformed config. One console.info("[hf-runtime] canonicalFps=N, source=export-config|default") at init.ts:51 and a sibling at the producer's normalizeRenderFps fallback path would surface a mis-wired entry point (e.g., the preview-script silent-default shape above) without needing a YDIF post-render probe. Cheap; the next debugging arc on this code is going to ask for it.
CI state
CI on 40fb2bcf (read-time freshness): all Preflight / Format / Lint / Typecheck / Build / Test / SDK / Studio smoke / runtime contract / CLI smoke / Fallow / CodeQL / perf passes are GREEN. 8 regression-shards are still in flight (no flake history this session). CLI: npx shim SKIPPED (path-filtered, no CLI changes). No blockers visible.
Prior reviewer state
Rames's COMMENTED review landed at 15:36 UTC (pullrequestreview-4580705016). Convergent on solid mechanism + concur on observability + test-pin gaps; he holds the preview-script-silent-default and observability angles; I hold the chunk-path-consistency, __HF_FPS dead-field, broader-canonicalFps-consumers, and boot-order angles. No conflicts on findings.
Review by Via
40fb2bc to
1cbe6b4
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
R2 follow-up to my R1 + Rames's R1 at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Magi's push addresses every actionable finding from both R1 reads. Audited the delta against the original asks:
✅ R1 items closed at 1cbe6b48
-
Catalog/template preview generators thread explicit fps (R1 Rames#1 + my N1) —
scripts/generate-catalog-previews.ts:241andscripts/generate-template-previews.ts:144now passfps: { num: 30, den: 1 }. The previously accidentally-correct silent-default is now an explicit invariant; future fps drift will fail loudly rather than silently re-introducing the original bug. -
Observability for source/fallback (R1 Rames#2 + #4) —
resolveRenderFpsConfigreturns{ value, source: "render-options" | "default", fallbackReason?: "missing" | "invalid" }; producer-side emitsconsole.info("[hyperframes] render fps defaulted", ...)when source is"default"(fileServer.ts:299-302); runtime-side emitsconsole.info("[hyperframes] render runtime fps", ...)when the config exists (init.ts:117-122). Both ends now log on the failure path so a mis-wired entry point shows up in producer logs without needing a YDIF post-render probe. -
Test pins (R1 Rames#3 test-asymmetry + my N2/N3) —
init.test.ts:188-208adds the "keeps the default 30fps renderSeek grid when export render fps is absent" test assertingtimeline.time()).toBe(0)(pins the originally broken behavior — the pre-fix grid would have roundedrenderSeek(1/60)to 0 at canonicalFps=30, exactly the regression-pin shape Rames asked for).fileServer.test.ts:226-296adds four cases via a sharedexpectInjectedRenderFpshelper:{ num: 60, den: 1 }(existing happy path, retained){ num: 24000, den: 1001 }fractional →"23.976023976023978"(my N2 NTSC pin)undefined→"30"source"default"reason"missing"(my N3 missing case){ num: 60, den: 0 }→"30"source"default"reason"invalid"(my N3 div-by-zero case)
The fractional test's expected value (
"23.976023976023978") matches the literalString(24000/1001)interpolation, locking in the floating-point pinning so a futureMath.round()offpsToNumberwould fail this test. -
Distributed chunk invariant comment (my N4) —
renderChunk.ts:500-502adds// These dimensions are frozen by the controller from the render job, so // chunk runtime seek quantization stays on the same fps grid as capture.Documents the producer-job → plan-dimensions → chunk-fps pipe so a future planner refactor that re-derives fps from a different source flips a known invariant. -
__HF_FPSannotated legacy (my N5) —window.d.ts:48adds/** Legacy debug-only fps hint. Render-mode runtime fps uses __HF_EXPORT_RENDER_SEEK_CONFIG.fps. */. Future readers tracing fps consumers will land on the canonical channel.
Items deferred (not blocking, low-priority)
owner: "runtime"field in the injected config object (my N6) — unchanged. Acceptable; no consumer grep-matches it but it's harmless metadata.- Typing strictness on
mode/step/offsetFraction(my N7) — unchanged. Same posture; the env-var validation is stricter than the type, and tightening is a follow-up nicety.
Verdict
LGTM at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Every actionable R1 finding addressed; the test pins close the regression-detection gap (both the floor-to-zero pre-fix behavior AND the NTSC fractional path) without expanding scope. The two-deep silent-fallback chain Rames flagged now emits at both layers. Clean push.
CI state
Re-running on 1cbe6b48: Analyze / Build / Detect changes / Fallow audit GREEN; regression-shards in flight. No reds at read-time.
Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 verification at 1cbe6b48f6d234b064af5349c165e85f3c110b44 — commit by @miguel-heygen, Slack-side R2 driver was the Magi peer bot. Layering on Via's R2; convergent independent verification with one distinct angle on observability reach.
Walked the R2 delta against the R1 ask list. The PR description's 5-point follow-up summary is accurate; every actionable R1 item from my R1 and Via's R1 is addressed at HEAD.
R1 finding-by-finding verification
Both scripts/generate-catalog-previews.ts:241 and scripts/generate-template-previews.ts:144 now pass fps: { num: 30, den: 1 } explicitly. The accidentally-correct silent-default at the preview thumbnails is now an explicit invariant — a future drift in createCaptureSession's output fps would no longer paper over a producer-side mis-wire. Concur with Via on the precision (the bug didn't reproduce today, the structural concern was the right shape to close). Clean.
Both layers of the silent-fallback chain now emit:
- Producer body-script (
fileServer.ts:349-354) firesconsole.info("[hyperframes] render fps defaulted", { canonicalFps, fallbackReason })on__renderFpsSource === "default". - Runtime (
init.ts:72-79) firesconsole.info("[hyperframes] render runtime fps", { canonicalFps, source, rawFps, fallbackReason })whenever the config object exists at all (covers success path AND parsed-invalid fallback — the runtime sees the wired-but-invalid case and logssource: "default"+fallbackReason: "invalid").
Caveat below as NEW #1 — both console.info calls execute in the headless browser's page context, and producer's browserManager.ts doesn't forward Runtime.consoleAPICalled to producer-side logs. The signal exists at the right semantic layer but doesn't surface in producer Node-side logs that SRE would tail. Still useful (DevTools attach catches it, regression-shard CI captures browser console), but a future hardening pass should pipe these into observability proper.
🟡 R1#3 — test asserts wired path, doesn't pin pre-fix behavior → ✅ resolved.
init.test.ts:187-207 adds the exact regression-pin shape I asked for: expect(timeline.time()).toBe(0) when __HF_EXPORT_RENDER_SEEK_CONFIG is unset, with an inline comment spelling out the pre-fix math (floor((1/60) * 30) / 30 = 0). A revert of state.canonicalFps = exportRenderFps.fps ?? state.canonicalFps would now fail this test, not just the wired-path test. Plus the delete window.__HF_EXPORT_RENDER_SEEK_CONFIG in afterEach (line 112) keeps the two tests independent. Right shape.
(My parity-harness aside — that parity-harness.ts pre-quantizes externally and would have missed the original bug — isn't addressed in this PR, but it's a separate harness-design concern; out of scope for this PR's R1 close.)
🟡 R1#4 — two-deep silent-30 fallback at producer + runtime → ✅ resolved-differently.
Original ask was "one emit on producer-side fallback would catch most cases." The PR ships emits on BOTH sides — producer-side on default-only, runtime-side on every config-present. That's stronger than my ask (catches the wired-but-invalid case too), with the page-context observability caveat above. Per feedback_open_item_alternative_resolution, crediting as resolved-differently rather than partial.
Magi's claimed-fix verification (5 items)
- Preview generators explicit
fps: { num: 30, den: 1 }— verified at both files (generate-catalog-previews.ts:238-242,generate-template-previews.ts:141-145). ✅ fpsSource+fpsFallbackReasonin config + runtime/producer logs — verified field shape atwindow.d.ts:48-49, producer setter atfileServer.ts:344-346, runtime reader atinit.ts:50-66. Producer log at:349-354, runtime log at:72-79. ✅- Tests for missing config, missing/invalid producer fps, fractional
24000/1001injection —init.test.ts:187-207covers absent config;fileServer.test.ts:268-296covers missing/invalid/fractional via the sharedexpectInjectedRenderFpshelper. Fractional test asserts the literal string"23.976023976023978"(String(24000/1001)), so any futureMath.roundinnormalizeRenderFpswould fail loudly. ✅ - Distributed chunk frozen-fps invariant comment —
renderChunk.ts:503-504adds the two-line invariant note. Reads cleanly; documents thecontroller render job → plan.dimensions.fps* → chunk fileServer fpschain. ✅ __HF_FPSmarked legacy/debug-only —window.d.ts:53adds/** Legacy debug-only fps hint. Render-mode runtime fps uses __HF_EXPORT_RENDER_SEEK_CONFIG.fps. */. Sole consumer atpackages/core/scripts/debug-timeline.ts:220still reads__HF_FPSfor backwards compat; the comment correctly steers new readers to the canonical channel without breaking debug tooling. ✅
NEW findings at R2
🟡 NEW #1 — page-context console.info not forwarded to producer Node logs.
fileServer.ts:349-354 and init.ts:72-79 both execute in the headless browser page context. Grepped packages/producer for Runtime.consoleAPICalled / page.on('console') / equivalent CDP wiring; nothing forwards browser console events into producer-side logs. So the new "log on silent fallback" signal lands in the discarded headless console, not in producer-job logs SRE tails. DevTools attach + regression-shard CI's browser-console capture both still see it, so it's useful for interactive debug + CI, but a producer-job emitting [hyperframes] render fps defaulted won't show up in kubectl logs producer-pod. Cheap follow-up: a one-liner in browserManager.ts to forward console.info with the [hyperframes] prefix into the producer logger. Doesn't block this PR.
🟡 NEW #2 — double-log on the producer-default → runtime-init path.
When the producer wires fpsSource: "default" (e.g. preview-script that doesn't pass fps, were that ever to happen post-PR), the body-script logs "[hyperframes] render fps defaulted" AND the runtime then logs "[hyperframes] render runtime fps" with source: "default". Two log lines for one fallback event. Both are useful (producer-side names the entry point; runtime-side reports the consumed value), but worth ensuring downstream log-grep / Datadog parsing distinguishes the two messages. Not a behavior bug; a noise-economy nit.
💭 NEW #3 — fpsSource field is read as a closed enum but typed loosely.
init.ts:62 does config.fpsSource === "default" ? "default" : "export-config" — any value other than the literal "default" is treated as "export-config". window.d.ts:48 correctly types it as "render-options" | "default", but the runtime resolver doesn't distinguish undefined / "render-options" / arbitrary-string from each other (all collapse to "export-config" in the telemetry). Per feedback_dispatch_map_silent_drop_default_types, a future enum extension (say fpsSource: "user-override") would silently surface as "export-config" in logs. Two cheap options: (a) explicit if (config.fpsSource === "render-options") ... else if (config.fpsSource === "default") ... else { warn; treat as default }, or (b) just pass config.fpsSource ?? "export-config" through as-is and let observability see the raw value. Either keeps the dispatch honest under enum growth. Non-blocking.
💭 NEW #4 — runtime log gate condition.
init.ts:72 gates the log on if (window.__HF_EXPORT_RENDER_SEEK_CONFIG) — present at all → log. Means the success path emits a log every render. Aligned with my R1 ask (every-render is fine for a startup-cardinality log), but if Datadog log-volume is sensitive, a log-sampling guard or env-var gate would defang the future "this is too chatty" complaint. Pure noise-economy, not a bug.
Confirmed clean
- Producer-side
Fpstype import +fpsToNumberreuse correctly handles fractional rationals; the producer'sresolveRenderFpsConfigatfileServer.ts:312-323mirrors the runtime'sresolveExportRenderFpsshape for symmetric reasoning. No drift. - Runtime's
resolveExportRenderFpscorrectly disambiguatesmissing(config absent orrawFps == null) frominvalid(non-finite or ≤0). Both feed the same fallback path but thefallbackReasonkeeps the failure mode identifiable in logs. afterEachcleanup atinit.test.ts:112properly deletes__HF_EXPORT_RENDER_SEEK_CONFIGso test order doesn't leak state.- Render orchestrator, probe, and distributed chunk paths all thread
fpsthrough tocreateFileServer. Engine package's separatecreateFileServercorrectly stays untouched (not the same code path, noRENDER_MODE_SCRIPTinjection). - Wider
state.canonicalFpsconsumers Via enumerated in R1 (postState bridge, clipTree rootDuration,player.seeknon-render path, etc.) all benefit from the wire-up. The fix is broader than the PR title implies, in a good way.
Cross-PR coherence
No changes to the #1725 / #1726 boundaries Via verified in R1. Snap algorithm at parityContract.ts untouched. Composes cleanly with the render-reliability arc.
CI state
At read-time on 1cbe6b48: Build / Lint / Test / Typecheck / Format / Preflight / SDK / Studio smoke / Test:runtime contract / Perf:fps / Perf:parity / Perf:scrub / Render catalog previews / Preview parity GREEN. Regression-shards 1-8 + windows-latest renders pending (no flake history this session). No reds. CodeQL skipped (path-filtered).
Verdict
LGTM at 1cbe6b48f6d234b064af5349c165e85f3c110b44. Every actionable R1 finding from both reviewers is closed; the test pins lock in the regression-detection gap (pre-fix toBe(0) regression-pin AND the NTSC fractional pin); the silent-fallback chain emits at both layers. NEW findings above (page-context log forwarding, double-log nuance, enum dispatch tightening, log-volume gate) are all follow-up-worthy but non-blocking — none rise to changes-requested at this scope. Concur with Via on the LGTM.
Stamp call stays with @miguel-heygen / James per protocol.
— Rames D Jusso
1cbe6b4 to
f1cadf2
Compare
What
Fixes #1737 by wiring the requested render fps into the page-side runtime before deterministic render seeks.
Why
renderSeek()quantizes DOM/GSAP timelines tostate.canonicalFps, but the browser runtime only defaulted that value to 30fps. Rendering at--fps 60therefore sampled DOM motion on a 30fps grid inside a 60fps output container, producing duplicate every-other frames.How
window.__HF_EXPORT_RENDER_SEEK_CONFIGfrom the producer file server.state.canonicalFpsduring initialization. This also keepsplayer.seek, bridge frame/time conversion, and clip-tree root-duration payloads on the same render fps grid.R1 follow-up
render-optionsvsdefault,missingvsinvalid) and runtime observability for the chosen canonical fps.24000/1001fps injection without rounding.plan.dimensions.fps*is frozen from the controller render job.window.__HF_FPSis legacy/debug-only; render-mode runtime uses__HF_EXPORT_RENDER_SEEK_CONFIG.fps.R2 follow-up
[hyperframes]page-console messages through the existing capture-session browser-console bridge with a dedicated[HyperFrames]host prefix, so runtime fps observability is visible in producer stdout/diagnostics.render-optionsanddefaultare accepted as known sources, and unexpected future values surface asunknownwithrawFpsSourcepreserved.Test plan
bun run --filter @hyperframes/core test -- src/runtime/init.test.tsbun run --filter @hyperframes/engine test -- src/services/frameCapture.test.tsbun test packages/producer/src/services/fileServer.test.tsbun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/engine typecheckbun run --filter @hyperframes/producer typecheck--fps 60renders GSAP/DOM motion at 30fps (render seek re-quantizes to hardcoded canonicalFps=30) #1737 repro onhyperframes@0.7.10: 60fps output had 240 frames / 93 exact consecutive duplicates; YDIF alternated with zero-motion frames.bunx oxfmt --check packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.ts packages/producer/src/services/fileServer.ts packages/producer/src/services/fileServer.test.ts packages/engine/src/services/frameCapture.ts packages/engine/src/services/frameCapture.test.tsgit diff --check