feat(engine): render speed ramps with the same source-time mapping as the preview

[miguel-heygen]

No description provided.

[terencecho]

HOLDING at f1fc750d — code merit verified, waiting on non-code state

Not stamping APPROVE yet: this PR is still isDraft=true and stacked on feat/speed-ramp-core (#4129, also draft). A stamp here would either sit dormant (drafts can't merge, and the dispatch line's "loop presses merge" cannot fire) or, if the draft/base states change while my approval carries, land the engine work into #4129's branch out of order relative to what actually ships to main. Posting the review as a COMMENT with a re-stamp promise once (a) #4143 is marked ready-for-review and (b) #4129 is landed on main or at least approved and queued ahead of #4143.

The sourceTimeAt unification — verified

One canonical mapping in packages/core/src/speedRamp.ts (exported through @hyperframes/core at packages/core/src/index.ts:333), imported by every renderer surface that needs to translate playback time to source time:

• packages/engine/src/services/audioMixer.ts:47 + :405-406 — the ramp-baker slices duration into 0.25s chunks, computes sourceTimeAt(lane, i*step) / sourceTimeAt(lane, (i+1)*step) for each slice, and uses atempo=(to-from)/step (mean rate) with apad,asetpts=N/SR/TB,atrim=0:step to hold the exact slice length. Pitch preserved by atempo alone (never asetrate).
• packages/engine/src/services/videoFrameExtractor.ts:727 + :795,804,815,888 — frame extraction window (durationSeconds), preroll trim (trimmedSourcePreroll), coverage extent, and per-time frame lookup all use sourceTimeAt(playbackRate, …).
• packages/producer/src/services/render/videoFrameCoverage.ts:481 + :511 — the ffmpeg -t duration argument for both the ramp path and the normalized-constant-rate path go through sourceTimeAt.
• packages/producer/src/services/htmlCompiler.ts:568 + :618 — compile-time seek preview computes mediaStart + sourceTimeAt(playbackRate ?? 1, 2 - clip.start), so the compiled attribute reader agrees with the runtime.

No competing path that computes source-time from a lane by hand. That's the "the whole correctness argument rests on one function" property.

Blocker fixes — both pinned

1. Attribute reader now handles single-quoted + entity-encoded JSON. packages/core/src/compiler/timingCompiler.ts:82 — the regex is now (?:"([^"]+)"|'([^']+)') with per-quote groups (the prior ["']([^"']+)["'] truncated at the first inner quote), plus a tagAttrReader shim that decodes " / ' / < / > / & before the JSON parser sees data-automation. Pinned by the parameterised rate lane in compiled media describe at timingCompiler.test.ts:10 — runs the same lane through both encodings, asserts unresolved[0]?.playbackRate matches {target: "rate"}. This was the "silently dropped ramps" case.
2. shiftRateLane keeps the curve of a shaped segment cut mid-way. speedRamp.ts:15 — when the trim falls inside a segment whose left point carries curve or viaX, it resamples the residual curve with 16 samples (SHIFT_RESAMPLES = 16) and re-anchors at {t:0, v:rateAt(spec, dt)}. Pinned by shiftRateLane with a shaped segment at speedRamp.test.ts:38 — curve:0.8 segment cut at dt=1 out of a 4s ramp; asserts sourceTimeAt(shifted, 3) ≈ sourceTimeAt(original, 4) - sourceTimeAt(original, 1), i.e. the integral shape is preserved across the shift.

Audio mixer math — spot-checked

The two new tests cover both regimes: constant 2x → 4 slices of 0.5s source stretched to 0.25s output; geometric 1x→3x → total -t = (3-1)/ln(3) ≈ 1.8205 and quarter-0 mean tempo (∫₀^0.25 3^t dt)/0.25 ≈ 1.1508. Both close-to-2-decimal-place asserts match the log-interpolated integration I get by hand. MAX_RAMP_SLICES = 240 bounds a 60s clip at the intended 0.25s granularity.

Golden fixture — realistic and strict

packages/producer/tests/speed-ramp/:

• src/index.html — both a <video> and an <audio> element carry the same data-automation lane [{t:0,v:1},{t:2,v:4},{t:4,v:0.5}] — video and audio proving they follow the same integration.
• meta.json — minPsnr:30, maxFrameFailures:0, minAudioCorrelation:0.9, maxAudioLagWindows:120. maxFrameFailures:0 is the strictest a video regression fixture gets in this repo.
• Composition duration 4s / fps 30, renderConfig.fps=30. The 4s window at that lane integrates to what the PR body reports (t=1s → source 1.44s, t=2s → source 4.33s — matches ∫₀¹ 3^t dt where log-interpolated 1→4 over [0,2]).

require_last_push_approval=true behaviour note

Marking the PR ready-for-review is not a push, so my future APPROVE binds fine across that transition. But any commit push after my stamp invalidates it — including a merge of #4129 into feat/speed-ramp-core that this branch is then rebased onto. So the safest sequence is: land #4129 first → this PR auto-rebases to main → then I re-stamp at whatever the resulting head is (may or may not equal f1fc750d).

What I need to see to re-stamp

1. isDraft=false on this PR.
2. #4129 either landed on main OR approved-and-queued ahead of #4143 in a merge sequence.
3. Head unchanged (f1fc750d) OR — if a rebase happened — the delta is only whatever git rebase produced, no author changes to the reviewed files.

Ping me here (or via Home) once (1) and (2) hold, and I'll do the small re-verification and stamp.

— Review by tai (pr-review)

[jerrai-bot-heygen]

Reviewed exact head f1fc750d2e943344bf3ad0880ffc97b1be4a1533 against its core speed-ramp base.

The engine uses the same sourceTimeAt mapping as preview at extraction, preroll/rebase, frame lookup, loop wrapping, audio slicing, and coverage. The rebase preserves a trimmed lane’s source offset, and the regressions cover trimmed clips, source-space loops, natural duration, parser encodings, and audio tempo/duration. No blockers found.

The visible failed checks are WIP/preflight/regression workflow states; I did not treat them as positive test evidence. I did not run locally because this review checkout has no dependencies.

— Jerrai

[terencecho]

RE-APPROVING at 013e26dd — hold conditions cleared

My prior COMMENT HOLD 5253929448 at f1fc750d conditioned on three things; all now met:

1. isDraft=false ✓ (marked ready-for-review)
2. #4129 approved-and-queued ahead ✓ (Somu APPROVED 18837f45)
3. Delta since my hold: only a rebase (identical file scope) + a single-file golden regeneration — no author changes to the reviewed code files

Per-commit scope since f1fc750d

Two new commits:

• 728b8b07 — feat commit re-applied on top of updated feat/speed-ramp-core (18837f45). 20 files, identical to the scope I reviewed at my hold. This is the expected Graphite rebase after #4129 got its own updates. No new files, no scope creep.
• 013e26dd — 1 file only: packages/producer/tests/speed-ramp/output/output.mp4 (LFS pointer bump). Golden binary regeneration; zero code files touched.

Golden regeneration — direction check

Author's reason: the stored baseline predated the per-slice audio fix, so its audio drifted 1–3% in pitch from the expected source-time track and the CI render correlated at 0.61.

The reference this is measured against is mathematical, not code-output: the source sweep's frequency at the ramp-integrated source time (sourceTimeAt(lane, t) applied to the swept tone). Since the reference exists independent of any code, "golden moved toward the reference, not toward the code" is the right direction — a legitimate golden update, not a self-referential regen.

Old golden tracked the reference at 1.3% median / 4.0% worst; current render tracks at 0.2% median / 3.4% worst. Compare now reports 0 failed frames + audio correlation 1.000 (was 0.61). The compare uses correlation over sample-window lag windows (per meta.json: minAudioCorrelation:0.9, maxAudioLagWindows:120), not byte equality — correlation 1.000 = matches to sampling noise. Consistent with the mathematical reference.

Code merit — unchanged from my prior review

The whole sourceTimeAt unification story, both blocker fixes (attribute reader single-quote/entity-encoded parity + shiftRateLane shaped-segment preservation), the audio-mixer math (constant + geometric regimes matching hand integration), the fixture strictness (maxFrameFailures:0, minAudioCorrelation:0.9) — all carry from my prior review at f1fc750d. Nothing in this delta touches the code paths I verified.

CI evidence

Three regression workflow runs at exact head 013e26dd:

Run	Time	Conclusion
35432925944	08:46 UTC	SUCCESS
35433672686	09:03 UTC	SUCCESS
35435480422	09:43 UTC	FAILURE (shard-7 only)

Two fully green runs at this exact SHA is strong evidence the suite is passing here. The one failing shard in run 35435480422 is shard-7 (style-6-prod style-5-prod style-8-prod portrait-edge-bleed many-cuts …) — no speed-ramp coverage; speed-ramp (bin-packed at 20s, shard-schedule.json) landed on a different shard which was green in the two prior runs at this SHA. The shard-7 log has no meaningful test failure output, just artifact-upload / cleanup steps — looks like flaky infra, not a code regression. Read as a flake given the two immediately prior fully-green runs at identical SHA.

Concurrent-review posture

Jerrai's APPROVED at 6fd34f57 is at a stale SHA (one commit behind the golden regen). hf-oss require_last_push_approval=true means his approval doesn't satisfy the last-push gate at 013e26dd; a fresh approval at head is required. This re-APPROVE fills that.

Stamp mechanics

hf-oss require_last_push_approval=true → this APPROVE binds to 013e26dd only. Prior COMMENT 5253929448 at f1fc750d stays as-is; it was a comment, not an approval.

Base is still feat/speed-ramp-core (#4129) — Graphite stack. Merge order: #4129 first (Somu APPROVED, awaiting last-push gate lift on merge), then this PR auto-rebases to main + gets its own last-push approval.

— Review by tai (pr-review)

[terencecho]

RE-APPROVING at 8d6fc26b — pure Graphite restack, range-diff byte-identical

My APPROVE 5255265328 at 013e26dd was invalidated by hf-oss require_last_push_approval=true when Graphite restacked this branch onto rebased #4129 (21241b49). Author claim: "replayed on the rebased core PR, range-diff identical."

Verified byte-identical

Same commit pair pattern:

• 1e0ddaa5 (feat commit re-applied on 21241b49) — 20 files, file list identical to prior 728b8b07
• 8d6fc26b (golden regen re-applied) — 1 file: output.mp4, identical to prior 013e26dd

Spot-checked blob SHAs at 013e26dd vs 8d6fc26b across the reviewed files:

File	Blob SHA (both heads)
packages/producer/tests/speed-ramp/output/output.mp4	9372ed20…
packages/engine/src/services/audioMixer.ts	d4cc17aa…
packages/engine/src/services/videoFrameExtractor.ts	04ece12c…
packages/core/src/speedRamp.ts	257d4712…
packages/core/src/compiler/timingCompiler.ts	2874a451…
packages/producer/tests/shard-schedule.json	01a87918…

All six match exactly. The reviewed tree state is byte-identical to 013e26dd — the delta is purely parent-SHA references (728b8b07 was on 18837f45, 1e0ddaa5 is on 21241b49). All prior verification carries: sourceTimeAt unification, blocker fixes, audio-mixer math, fixture strictness, golden-regen direction check.

Stamp mechanics

hf-oss require_last_push_approval=true → this APPROVE binds to 8d6fc26b. Prior APPROVE 5255265328 at 013e26dd stays as-is but is superseded by the last-push gate.

— Review by tai (pr-review)

The base branch was changed.

[terencecho]

RE-APPROVING at 3f13e1e6 — rebase over merged main (#4129 + #4163 + #4164) plus a clean pitch-attr removal

My prior APPROVE at 8d6fc26b (5255283519) was dismissed by the rebase. Delta from 8d6fc26b → 3f13e1e6 is 6 commits ahead / 5 behind; the 5 "behind" are just merged main (#4163, #4129, #4164), and the 6 "ahead" are:

• 86667203ce — merge of #4163 (mine, stamped)
• 7f452ad276 — merge of #4129 (the speed-ramp core; Somu APPROVED at 21241b49)
• 35a4fe1da7 — merge of #4164 (Rames CR now at same head, but the merge landed against his earlier stamp; not this PR's concern)
• adb8989f70 — the original #4143 engine content, replayed on the new base with byte-identical author intent
• 26891657b4 — the speed-ramp golden regen (git blob SHA 9372ed20ae64… — same blob as what I verified in my 013e26dd/8d6fc26b review; correlation 0.61 → 1.000, 1.3% → 0.2% pitch drift, direction pinned to mathematical reference)
• 3f13e1e627 — new: drop data-preserve-pitch

Pitch-attr removal (3f13e1e627) — 4 files, 24 deletions / 2 additions

Commit rationale (quoted): "The engine always time-stretches audio with atempo, so an opt-out read only by the preview made preview and render disagree. Remove the attribute and its reader so both paths keep pitch."

Verified against the diff:

• packages/core/src/speedRamp.ts: PRESERVE_PITCH_ATTR = "data-preserve-pitch" constant and readPreservePitch(el) function both deleted.
• packages/core/src/speedRamp.test.ts: import + it("preserves pitch unless the clip opts out", …) block deleted; the enclosing describe renamed "presets and pitch" → "presets".
• packages/core/src/runtime/media.ts: import readPreservePitch dropped; call site el.preservesPitch = readPreservePitch(el) inside syncRuntimeMedia deleted.
• packages/core/src/runtime/media.test.ts: expect(clip.el.preservesPitch).toBe(true) line removed from the rate-lane test.

Reachability check — grepped data-preserve-pitch, PRESERVE_PITCH, preservesPitch, readPreservePitch at head 3f13e1e6 across the surfaces where a dangling ref would matter (Studio Timeline/TimelineCanvas, engine audioMixer, producer htmlCompiler, CLI render, lint rules, runtime init) — zero hits. The Studio-side toggle removal Miguel is landing in #4153 stacks on this — so the UI that would have written the attribute is going away in the same series.

Behavioural claim held: engine still preserves pitch by construction — audioMixer.ts builds every tempo change through buildAtempoFilter (atempo=... chain), FFmpeg's atempo is pitch-preserving by definition, and buildRampFilterComplex composes per-slice atempo stages the same way. Preview side: syncRuntimeMedia no longer sets el.preservesPitch; browsers default HTMLMediaElement.preservesPitch = true, which is what the attribute-defaulted-to-true previously read to anyway. Net effect: preview + render now agree on always-preserved pitch, no user-visible regression for anyone who wasn't already opting out (and for those who were, the render already ignored them — this fix stops silently disagreeing with them).

Everything else — replayed on merged main, structurally same as 8d6fc26b

Cross-checked the fresh gh pr diff (22 files, 1120 lines) against my prior review at 8d6fc26b. The centralization I verified before still holds at the new head:

• packages/core/src/speedRamp.ts shiftRateLane: unchanged shape — resample a shaped curve/viaX segment cut in the middle at SHIFT_RESAMPLES=16, integrate from the new origin. sourceTimeAt/timeAtSourceTime inverse pair still round-trips.
• audioMixer.ts buildRampFilterComplex: per-slice atempo = (sourceTimeAt(i+1)-sourceTimeAt(i))/step (mean rate across the slice), RAMP_SLICE_SECONDS=0.25, MAX_RAMP_SLICES=240. Tests pin the exact ffmpeg graph shape (asplit=N, per-slice atrim … atempo … apad … atrim=0:step, concat=n=N) at 2x constant and at 1→3x geometric.
• videoFrameExtractor.ts rebaseVideoToWindow: shifts video.playbackRate = shiftRateLane(rate, start - video.start) before moving video.start. Float-noise guard (Math.abs(window.compositionStart) < 1e-9 ? 0 : …) preserves first-frame lookup.
• getFrameIndexAtTime ramped loop wrap: source-space modulo (sourceTime %= cycle when typeof rate === "object" && loop), so a lane keeps running across cycles instead of restarting from lane t=0. Test-pinned (4s→3x lane, source(3)=4.6586 wraps to 0.6586, frame 16).
• resolveTimelineExtractionWindow visible-source-duration: for a lane, uses sourceTimeAt(rate, trimmedPreroll + visibleDuration) - trimmedSourcePreroll (integrated span across the visible window), not duration * rate (which would multiply by a single point). Preserves timeline lookup semantics under trim.
• Golden fixture (packages/producer/tests/speed-ramp/*): identical to what I verified before. meta.json demands minPsnr=30, maxFrameFailures=0, minAudioCorrelation=0.9, maxAudioLagWindows=120. Reference sweep is mathematical (frequency at ramp-integrated source time), so a matching baseline confirms the engine, not the code.

CI

Running at post — SETTLED green expected via the polling script.

Stamp mechanics

hf-oss require_last_push_approval=true → this re-APPROVE binds to 3f13e1e6 only. Prior reviews (5255283519 at 8d6fc26b, 5255265328 at 013e26dd) remain dismissed.

Miguel miguel-heygen on trusted stamp list; stamping to merge per dispatch.

— Review by tai (pr-review)