feat: make creator media edits render-safe

[miguel-heygen]

What

• Makes creator-facing clip assembly, visual keyframes, and placed-audio editing explicit and copyable across the HyperFrames skills.
• Makes constant media playback rate render-safe across preview, WebAudio, final A/V, natural-duration readers, compiler injection, and zero-span handling.
• Preserves natural voice pitch and formants when Studio global playback speed is not 1x.
• Uses one strict literal timing-number contract across preview, compiler, and final readers so hand-authored values such as data-duration="0s" cannot make preview play while render drops the clip.

Why

HyperFrames already supports NLE-style clip assembly and seek-safe visual motion, but those capabilities were under-routed. Runtime and render paths also disagreed on several media timing boundaries. Studio additionally routed decoded PCM through AudioBufferSourceNode.playbackRate, which resampled voices at non-unit global speed and shifted pitch/formants.

How

• Core owns temporal source trim/splice/reorder through duplicated media clips using data-start, data-duration, and data-media-start; matching audio uses the same ranges.
• Keyframes own visual motion on inner wrappers: punch/zoom/pan/reframe/crop/mask/clip-path handoffs.
• Placed audio mixing, fades, automation, ducking, carve, and FX remain owned by hyperframes-audio.
• Strict finite timing values are normalized consistently across preview, compiler, and final readers.
• Studio keeps HTMLMediaElement as the pitch-preserving source-time/rate owner and routes its cached MediaElementAudioSourceNode through the existing FX, automation, element-gain, and master graph.
• Rerouted media stays unmuted and at unity upstream volume; downstream gain remains the single author × user volume/automation owner. Decoded fallback is never used at a non-unit effective rate where it would knowingly shift pitch.

Test plan

• Unit tests added/updated

• Manual A/V and Studio master-output proof performed

• Documentation updated

• focused Studio-audio runtime gate: 4 files / 244 tests

• runtime CI: 45 files / 941 tests

• core: 118 files / 2,391 tests

• full Studio and Studio-server tests/typechecks passed; producer typecheck passed

• exact full-Studio sanitized TTS matrix at 0.5x, 0.75x, 1x, 1.5x, and 2x: median F0 177.53–178.56 Hz versus 179.59 Hz source, zero hard discontinuities, and checkpoint drift under ~52 ms

• 0.1x boundary: timeline/audio advanced 0.99994 s over 10.02 wall seconds with source-like pitch and zero discontinuities

• pause/resume, live rate switch, seek, mute/unmute, trim, nested timing, cached-source reuse, FX, and automation checks passed

• matched 2x loudness control: one 0.5 volume lane plus one -6 dB gain measured -11.8 dB, consistent with the expected -12.02 dB and not double-applied volume

• full build, lint, format, fallow, typecheck, and hooks passed

[jrusso1020]

Reviewed at f763e6158f6 (full files, not just the diff). The retiming math is right in the places it is easy to get wrong, and the parity test is the strongest part of the PR. One finding I would want addressed before this lands, two nits, and a note on why I have not stamped.

1. The two halves read data-playback-start differently, and they disagree on the empty string

This is the one that matters, because the disagreement is between exactly the two readers that have to agree for audio and video to stay in sync.

// audioMixer.ts:475  — truthiness on the raw string
mediaStart: playbackStartAttr ? parseFloat(playbackStartAttr)
          : mediaStartAttr    ? parseFloat(mediaStartAttr)
          : 0,

// videoFrameExtractor.ts:547 — ?? on the raw string, which keeps ""
const mediaStartAttr =
  el.getAttribute("data-playback-start") ?? el.getAttribute("data-media-start");

getAttribute returns "" — not null — for a valueless attribute, so ?? does not fall through where the truthiness chain does. I ran both expressions against the same inputs rather than reasoning about it:

attributes	audio mediaStart	video mediaStart
data-media-start="5" (no playback-start)	5	5
data-playback-start="" data-media-start="5"	5	0
data-playback-start="0" data-media-start="5"	0	0
data-playback-start="2" data-media-start="5"	2	2
data-playback-start=" " data-media-start="5"	NaN	NaN

One divergent row, and its consequence is the full trim offset as A/V desync: the audio track starts 5s into the source while the frames start at 0. On a PR whose subject is render-safety for creator edits, that is the failure mode the change exists to prevent.

Two things keep me from calling it a blocker, and I want to be fair about both.

First, you did not introduce this inconsistency — it is inherited. The runtime already disagrees with itself on "": timeline.ts:19-23's parseNum explicitly returns null for "" (so its ?? chain falls through), while clipTree.ts:53-57's parseNum does not, and Number("") === 0, so it yields 0; init.ts:751 coalesces raw strings and also lands on 0. Your audio side matches timeline.ts and your video side matches clipTree.ts. Both have precedent.

Second, no tooling can emit an empty value — every writer stamps String(number) (compositionInsertion.ts:209, sourceMutation.ts:389-392, timelineEditingHelpers.ts), so this is reachable only from hand- or agent-authored HTML. That is a first-class authoring path here, and data-playback-start="" is a natural way for a generator to spell "unset", but it is not something the editor produces.

What makes it worth fixing here anyway is that this PR is where the consequence becomes A/V desync. Before it, both engine halves read only data-media-start through the same truthiness expression, so they could not disagree. Adding a second attribute name with different empty-string handling on each side is what turns an inherited cosmetic inconsistency into a sync bug.

The fix is also the PR's own thesis applied one level down — make the engine agree with the runtime by construction:

const mediaStart =
  parseNum(el.getAttribute("data-playback-start")) ??
  parseNum(el.getAttribute("data-media-start")) ??
  0;

using timeline.ts's parseNum semantics (reject null and "") in both files. Parse first, then coalesce on parse failure; coalescing raw strings is what admits "" as a value. That also fixes the " " row for free, which currently yields NaN on both sides and flows into -ss NaN — resolveVideoExtractionWindow's range guard cannot catch it, because every comparison against NaN is false.

Worth noting the new reader is also the one piece of this change with no test: the added cases cover data-playback-rate parsing on both sides, and nothing exercises data-playback-start at all.

2. Nit — the duplicated predicate in resolveVideoExtractionWindow is now load-bearing and unmarked

const resolvedDuration =
  Number.isFinite(requestedTimelineDuration) && requestedTimelineDuration > 0
    ? requestedTimelineDuration
    : resolveSegmentDuration(requestedTimelineDuration, video.mediaStart, playableDuration) /
      playbackRate;

resolveSegmentDuration opens with that same Number.isFinite(requested) && requested > 0 test and returns requested unchanged, so the inline branch is a deliberate duplicate: it exists so / playbackRate applies only to the source-derived fallback, an authored timeline duration already being in timeline units. That is correct, and it is also why the obvious cleanup is wrong — collapsing it to resolveSegmentDuration(...) / playbackRate would divide an explicit authored duration by the rate. One comment saying so would keep the next reader from tidying it into a bug. (I confirmed the rate-1 path is arithmetically unchanged from main, so this restructure carries no blast radius for existing compositions.)

3. Nit — the degenerate-duration fallback is no longer rate-aware

const effectiveDuration =
  (metadata.durationSeconds - element.mediaStart) / normalizePlaybackRate(element.playbackRate ?? 1);
element.end =
  element.start + (effectiveDuration > 0 ? effectiveDuration : metadata.durationSeconds);

The primary term is converted to timeline seconds; the metadata.durationSeconds fallback is left in source seconds. Only reachable when mediaStart >= durationSeconds, and the fallback was already there — but the two branches now carry different units, so a retimed clip in that state gets an end playbackRate× off. metadata.durationSeconds / rate would keep them consistent.

Checked and fine — recorded so nobody re-checks

• The atempo chain is correct at both clamp bounds. normalizePlaybackRate bounds input to [0.1, 5] and maps non-finite/non-positive to 1, which is what makes the Number.NaN sentinel for a missing attribute work and what bounds the two while loops. Worked the arithmetic at the extremes: 0.1 → [0.5, 0.5, 0.5, 0.8] and 5 → [2, 2, 1.25], both exact, and atempo is pitch-preserving as the docstring claims.
• The filter merge avoids a real footgun. preparedAudioOutputArgs folds atempo into the existing channel filter string and emits a single -af, rather than appending a second -af — which ffmpeg would silently resolve by dropping the first, taking the stereo normalization with it.
• Moving -ss/-t ahead of -i in extractAudioFromVideo is an optimization, not an accuracy regression. Input-side seek is fast, and -accurate_seek is on by default, so the start stays sample-accurate rather than snapping to a packet boundary. The input -t duration * rate paired with the output -t duration when rate ≠ 1 is right in both directions — checked 2× (read 2D source, compress to D) and 0.5× (read 0.5D, stretch to D).
• The held-tail branch gets the unit conversion right, which is the easiest line in the diff to get wrong: compositionStart: video.start + extractionOffset / playbackRate (timeline seconds) alongside mediaStart: video.mediaStart + extractionOffset (source seconds).
• getFrameIndexAtTime's unit chain is consistent — loopDuration divided into timeline seconds for the modulo, and the index taken from localTime * rate * fps.
• Automation × retiming is covered, which was my first guess at a gap: keeps automation on authored timeline time after constant retiming pins that a volume lane authored in timeline time stays in timeline time through a 2× retime.
• The parity test is the right construction for the claim it makes. Four colour bands crossed with four tones (440/660/880/1100 Hz), sampled at 0.25/0.75/1.25/1.75s, asserting both the dominant colour and the detected frequency at each point, plus output duration in [1.95, 2.05]. A test asserting only duration, or only frames, could not tell "both streams retimed" from "one retimed" or from "both retimed but mutually offset" — this one fails on any of those. Worth keeping as the template for the rest of the stack.

Why I have not approved

Nothing above is a merge blocker on its own, but Tests on windows-latest is still pending at this head and it is one of the eight required contexts on main — I do not stamp over a required check that has not reported. Non-required Perf: parity is also still running. The other seven required contexts are green, including Render on windows-latest and regression.

Once Windows reports and §1 is settled either way, ping me and I will re-check — this repo requires approval of the last push, so an approval posted now would not survive the fix commit anyway.

Note: /code-review max can't be invoked from my side, so this is that lens applied by hand rather than a lighter review silently substituted.

— Rames Jusso

[jrusso1020]

Re-reviewed from scratch at ffe375fc796, full files rather than the diff. I had said I would carry my earlier parser finding forward only if mediaTiming.ts was untouched — it was deleted in this round, so I re-derived that finding from zero rather than assuming it still held. It does hold, and the result is stronger than what I closed before.

The runtime-contract failure: root cause, and the assertion was not weakened

This was my main open question, since the previous head failed on a legacy WebAudio cleanup test seeing isActive() === false immediately after scheduling, and that file was also being changed for the playback-rate work. A green re-run would not have answered it.

The cause is in the test doubles, not the transport. The mocks were catch-alls:

const mockEl = { muted: false, getAttribute: () => "1" } as unknown as HTMLMediaElement;

getAttribute: () => "1" answers "1" to every attribute name. Once readMediaStart began reading data-playback-start, that stub returned 1 where a real element returns null, so the scheduled window shifted and the source was no longer active. The fix makes each stub name-dispatching (name === "data-playback-rate" ? "1" : null), and expect(transport.isActive()).toBe(true) is untouched — it appears in the diff only as context. So the assertion held and the harness was corrected, which is the right direction.

Worth naming because it generalises: a catch-all attribute stub is inert until production reads one more attribute, and then it lies. It cannot fail early, and it fails as a wrong value rather than an error.

Which is why the three that remain are worth a look — one of them is in this same file. webAudioTransport.test.ts:162 still has the original getAttribute: () => "1", in the same describe block, using the same withSource helper, as a stub you did fix eight lines above it. It is inert today: ownsElement reads no attributes and withSource hardcodes mediaStart: 0. But it is the identical landmine that produced the retracted head, left armed. Same shape at :494 (getAttribute: () => src) and audioFx.test.ts:110 (() => "{not json"). The () => null stubs elsewhere are fine — null is what an element without the attribute actually returns.

1. mediaTimelineWindow.ts parses with parseFloat where core parses with Number, and that now decides whether an element exists

This is the one I would want a decision on. The new helper is the right idea and its fail direction is right — it only drops on an explicit inactive window, so absent attributes keep the media. But:

// packages/engine/src/services/mediaTimelineWindow.ts
const parsed = Number.parseFloat(raw);

against core, which is Number(...) in all three places that read the same attributes — clipTree.ts:56, timeline.ts:22, init.ts:747. The two disagree on trailing garbage. I ran the drop decision both ways:

data-duration	engine (parseFloat)	core idiom (Number)
"0s"	DROP	keep
"0abc"	DROP	keep
"0px"	DROP	keep
"-1s"	DROP	keep
"0x10"	DROP	keep
"0"	DROP	DROP
"5s", "abc"	keep	keep

So data-duration="0s" means the runtime plays the clip and the render drops it — silently, with no diagnostic, which is the exact preview-versus-render class this PR exists to close.

Two things that argue this is important rather than blocking, and I want to be accurate about both. First, you did not introduce the split: htmlCompiler.ts:2174-2175 already uses parseFloat on these same attributes, so the engine/producer side was consistently parseFloat and core consistently Number before this PR. Second, it needs malformed authoring to bite, and machine-generated HTML emits String(number).

What changes here is the consequence. Previously that inconsistency perturbed a computed duration; now it gates whether the element is emitted at all. And the population this PR is named for — creator-authored media edits, with the skill docs in this same diff teaching people to write these attributes by hand — is precisely the population that writes "0s". One line (Number in place of Number.parseFloat, the trim() guard already handles the whitespace difference) makes the drop decision agree with the runtime.

2. Nit, carried forward unchanged — the duplicated predicate in resolveVideoExtractionWindow is still load-bearing and unmarked

Still at videoFrameExtractor.ts:1087-1092. resolveSegmentDuration opens with the same Number.isFinite(requested) && requested > 0 test, so the inline branch is a deliberate duplicate that keeps / playbackRate applying only to the source-derived fallback. Correct as written, and still the kind of thing a later cleanup collapses into a bug. One comment fixes it permanently.

3. Nit, carried forward unchanged — the degenerate-duration fallback is still not rate-aware

audioMixer.ts:939-943:

const effectiveDuration =
  (metadata.durationSeconds - element.mediaStart) / normalizePlaybackRate(element.playbackRate ?? 1);
element.end = element.start + (effectiveDuration > 0 ? effectiveDuration : metadata.durationSeconds);

Primary term in timeline seconds, fallback in source seconds. Only reachable when mediaStart >= durationSeconds, so low severity, but a retimed clip in that state still lands playbackRatex off. metadata.durationSeconds / rate keeps the branches in one unit.

Verified, so nobody re-checks

• The parser convergence is real and wider than before. mediaTiming.ts is gone; readMediaStart now lives in packages/core/src/runtime/playbackRate.ts and is exported from @hyperframes/core, with audioMixer.ts:474 and videoFrameExtractor.ts:578 both importing it, plus timingCompiler, media.ts and startResolver. One definition, six consumers. I extracted the real implementation and executed it against the pinned table: 10/10, including valueless/empty/whitespace falling through and "0" still resolving to zero. Both engine case tables are identical, case for case.
• The new value >= 0 guard is a genuine behaviour change, and it is pinned. Negative offsets now fall through to the next source instead of being accepted; negative playback-start and negative media-start both have rows.
• The reclassified test is not a red relocated to green. htmlCompiler.naturalDuration.test.ts is new in this PR (added by f9b117ee6), it is not on main, and it did not exist at the retracted head — so nothing moved out of a gating lane. The added assertion in test-classification.test.mjs pins the lane deliberately rather than leaving it implicit, which is the right way to do this.
• isKnownInactiveTimelineWindow is applied symmetrically — <audio>, <video data-has-audio="true">, and the video path all gate through it, so the audio and video sides cannot disagree about which elements exist.
• CI at this exact head, checked independently rather than taken from the summary. All 8 required contexts on main are present and green, collapsing to the latest run per name: Build, Test, Typecheck, Test: runtime contract, regression, Semantic PR title, Tests on windows-latest, Render on windows-latest. Both Windows lanes are green (one was pending when I last looked) and Test: runtime contract is green, which is the lane that failed before. Note regression-shards shows cancelled at the parent matrix level while all nine shards are individually green; it is not in the required set, so nothing hangs on it, but cancelled is inconclusive rather than green and it is worth not reading that row as a pass.

Verdict

COMMENTED. No merge was requested and I am not stamping. Section 1 is the one worth a decision before this lands; 2 and 3 are small and unchanged from my last pass.

The thing I most wanted to check came out well: the previous head's failure was diagnosed rather than papered over, and the fix corrected a lying test double instead of relaxing the assertion.

Note: /code-review max cannot be invoked from my side, so this is that lens applied by hand rather than a lighter review silently substituted.

— Rames Jusso

[jrusso1020]

Re-reviewed at e72f3184f88eb1893184e4a57c3f228e77d1de6a. My parseFloat/Number finding is closed, and I verified it rather than accepting "fixed by construction". No stamp requested, so this is a COMMENT.

The finding is genuinely closed

The winning idiom is Number, via parseNumeric in @hyperframes/parsers/composition-contract (compositionContract.ts:100-104), now surfaced as parseStrictFiniteTimingNumber from @hyperframes/core. Putting it in the parsers package rather than re-implementing it per consumer is the right home.

The regression guard is the part I care about most, and mediaTimelineWindow.test.ts pins the exact discriminating cases rather than round numbers: "0s", "0abc", "0px", "0x10", "-1s", "Infinity", "NaN", plus the ["-1s", false] / ["-1", true] pair. Those stubs are attribute-aware too, so they cannot rot the way the earlier ones did. Executing both idioms on the inputs that matter:

raw	strict (Number)	parseFloat
"0s"	null	0
"10s"	null	10
"-1s"	null	-1
"0x10"	16	0

The "0x10" row is why this needed one owner rather than two tolerant readers: the idioms do not just differ in strictness, they return different finite numbers for the same string.

Two things I checked that hold up well:

• The Puppeteer boundary is handled correctly. The in-page function returns endRaw / durationRaw / playbackStartRaw / mediaStartRaw as raw strings (htmlCompiler.ts:2179-2182) and Node parses them with the shared parser (:2211-2216, :2248-2249, :2507). Since a serialized evaluate callback cannot reference imports, returning raw and parsing outside is the only way to get one parser across that boundary. The synthetic getAttribute shim at :2213-2216 is attribute-aware and returns null for everything else, which is the discipline I asked for in the tests now showing up in production code.
• videoFrameExtractor.ts:566-574 and :625-633 fixed a bug beyond the unification. The old form was if (endAttr) { end = parseFloat(endAttr) }, a truthiness test on the raw string, so data-end="abc" became NaN and dropped the element. Falling through to the duration attribute and then to Infinity is strictly better behaviour.

The scope decision is right, and worth stating out loud

data-volume and data-playback-rate stay on Number.parseFloat, and I read that as deliberate rather than missed: parseFiniteDatasetNumber was renamed to parseVolumeNumber (mediaVolumeEnvelope.ts:99) while the three timing reads beside it moved to the strict parser, and htmlCompiler.naturalDuration.test.ts:62-65 pins data-playback-rate="0x2" to 10, which only holds under parseFloat (Number("0x2") is 2, which would make it 5). So the rule the PR actually implements is time-valued attributes are strict, scalar attributes stay lenient.

That is a defensible rule. My only ask is that it be stated somewhere durable, because the PR text reads as "one parser owns timing attributes" and a reader will reasonably conclude data-playback-rate="2x" is now rejected. It is not, and there is a test asserting it is not. On one element, data-playback-rate="2x" is honoured as 2 while data-duration="2s" means "absent" — fine as a decision, confusing as an accident.

New: one dormant second idiom on data-duration

packages/core/src/runtime/media.ts:84:

let duration = params?.resolveDurationSeconds?.(el) ?? Number.parseFloat(el.dataset.duration ?? "");

This is not a live divergence today and I want to be precise about why: the only production caller of refreshRuntimeMediaCache is init.ts:1981, and it always passes resolveDurationSeconds, whose own read is parseStrictFiniteTimingNumber(element.dataset.duration) at init.ts:2005. So the parseFloat branch is currently reachable only from tests.

It is still worth one line, because the trap is cheap to fall into: a second caller that omits the resolver (an innocuous-looking thing to add) silently restores parseFloat semantics for data-duration on the preview side while the render side stays strict, which is this PR's original bug with the polarity flipped. data-duration="0s" would zero-length the clip in preview and play it in the render. Since the file is already touched by this PR, routing that fallback through the shared parser closes the door for free.

New: the stub cleanup introduced a fourth catch-all

The three I named are all attribute-aware now (webAudioTransport.test.ts:156 / :164, :498, audioFx.test.ts:111). But htmlCompiler.test.ts:2017 is new in this PR and is the same shape:

querySelector: () => ({ getAttribute: () => "root" }),

That answers "root" to every attribute name, in the test for the media-volume path this revision added. Inert right now, because the code under test reads only one attribute (htmlCompiler.ts:2265-2266, querySelector("[data-composition-id]") then getAttribute("data-composition-id")). It is inert in exactly the way the earlier ones were before a second attribute read turned them into wrong answers, which is what cost this PR a revision. One line:

querySelector: () => ({ getAttribute: (name: string) => (name === "data-composition-id" ? "root" : null) }),

For the record the other surviving getAttribute: () => stubs are all () => null (fileServer.test.ts:972, gsapRuntimeBridge.test.ts:37, gsapDragCommit.test.ts:25), which is fine: that models a genuinely absent attribute. Non-null catch-alls are the only trap.

The two earlier nits, both still open

Nit A, sharper than I first put it — videoFrameExtractor.ts. resolveSegmentDuration (:856-864) opens with if (Number.isFinite(requested) && requested > 0) return requested;, and its only caller (:1093-1096, module-private, no other call sites) checks the identical predicate first and takes its own branch when true. So :861 is unreachable. The duplication is also load-bearing in a way that is easy to miss: the caller's true-branch returns requestedTimelineDuration undivided, while the else-branch divides by playbackRate. Anyone who "simplifies" the caller into resolveSegmentDuration(...) / playbackRate starts dividing authored durations by the rate. Either delete the dead early-return, or move the / playbackRate inside so both paths agree.

Nit B — audioMixer.ts:942-946 is unchanged:

const effectiveDuration =
  (metadata.durationSeconds - element.mediaStart) / normalizePlaybackRate(element.playbackRate ?? 1);
element.end = element.start + (effectiveDuration > 0 ? effectiveDuration : metadata.durationSeconds);

The fallback discards both mediaStart and playbackRate, so the degenerate case does not just lose precision, it contradicts the line above it. parseEnd next to it did get the strict-parser treatment (:453-455), so this is the last spot in the file still computing an end a different way.

State

At this exact head: 60 check runs, 57 success, 3 skipped (GCP BeginFrame image contract, Catalog: search index covers the registry, Mintlify Deployment), zero failing, zero pending, and all 8 contexts required on main are present in the rollup rather than merely not-failing. reviewDecision is REVIEW_REQUIRED and this comment leaves it there.

— Rames Jusso