fix(video): hold final frame through composition

[miguel-heygen]

What this solves

Addresses #2514 without changing the HyperFrames Core timing contract.

When an author gives a non-looping video an explicit slot longer than its source:

<video src="1s.mp4" data-start="0" data-duration="5" muted playsinline></video>

HyperFrames now keeps that authored 5s visibility window and holds the source's final decoded frame from 1s through the end of the slot. Preview and SDR/HDR rendering agree.

Contract boundary

This PR deliberately does not change the meaning of omitted data-duration.

• Explicit data-duration: the authored slot is authoritative; a shorter non-looping video holds its final frame until the slot ends.
• Omitted data-duration: the video uses its intrinsic/full source duration, exactly as documented by hyperframes-core.
• Looping mechanics are unchanged, but preview now honors a looped video's explicit authored slot instead of truncating it to one source cycle.
• Audio remains source-bounded.
• HyperFrames continues to own media playback; composition code must not play, pause, or seek media itself.

Therefore, this PR does not automatically extend every short video to the composition end. Authors opt into a longer hold by declaring the desired video slot with data-duration.

Root cause

Three layers independently conflated the authored display slot with decodable source duration:

1. compilation shortened explicit non-looping video slots to the probed source duration;
2. SDR frame lookup dropped payloads after the last extracted frame;
3. browser preview treated an ended video as inactive and truncated explicit video slots to source duration.

The fix preserves the explicit slot at compilation, holds the final extracted frame only while that slot remains active, and mirrors the same authored-slot behavior in native preview.

Regression coverage

• A 1s video with explicit data-duration="5" keeps data-duration="5" data-end="5".
• SDR returns the final frame at 1.5s and 4.5s, then removes it after the authored slot.
• Native preview clamps direct tail seeks to the final frame and supports backward seeks from ended state.
• A looped 1s video with an explicit 10s slot keeps the full 10s window for repeated playback.
• A video without data-duration, including one inside a longer composition, remains bounded to its intrinsic duration.
• Zero-frame-source, standalone-video, and audio behavior remain pinned.

Verification

• Core compiler/runtime: 110 passed
• Core typecheck: passed
• Engine focused hold-last-frame and media-fit regressions: 9 passed
• Engine typecheck: passed
• Producer typecheck: passed
• CLI validate suite: 21 passed
• oxlint: 0 warnings/errors
• oxfmt check: passed
• git diff --check: passed

The full local engine suite has two unrelated environment failures because the installed ffmpeg lacks -fps_mode; all changed frame-selection tests pass.

[james-russo-rames-d-jusso]

Reviewed at 44d35560. Traced the tag/loop/explicit-slot flag across compile → runtime → SDR extractor → HDR compositor → CLI validate. One concern on the contract statement, one nit on analyzeClipMediaFit.

Blockers

(none)

Concerns

🟠 The PR body claims "Looping behavior is unchanged" but <video loop data-duration="X" src="shorter.mp4"> IS a behavior change.

Pre-PR, resolveDurationSeconds in init.ts did Math.min(sourceDuration, hostRemaining, explicitDuration) uniformly — so a looped video with a source shorter than its explicit slot got clip.duration = source, and the runtime stopped the clip when source ran out.

Post-PR, the new resolveRuntimeMediaClipDuration({isVideo: true, …}) at packages/core/src/runtime/media.ts:22-30 folds explicitDuration into mediaDuration = explicit ?? source and drops explicitDuration from the candidates list for video. So the loop candidates become [explicit ?? source, hostRemaining] — for a <video loop data-duration="10" src="1s.mp4">, that's Math.min(10, host) instead of the old Math.min(1, host, 10) = 1.

Concrete: pre-PR the loop video stopped after 1s; post-PR it loops for the full authored 10s. Compile side already preserved the authored data-duration for loop clips (if (el.loop) continue; at htmlCompiler.ts:74), so this new runtime behavior aligns compile + runtime — which is almost certainly the right fix, but it's not "unchanged."

Two things worth doing:

1. Update the "Looping behavior is unchanged" sentence in the PR body / resolveRuntimeMediaClipDuration docstring to say something like "looping mechanics unchanged; a looped clip's authored slot is now honored (previously silently truncated to source)."
2. Add a test in packages/core/src/runtime/media.test.ts pinning resolveRuntimeMediaClipDuration({isVideo: true, sourceDuration: 1, hostRemaining: null, explicitDuration: 10}) === 10 under a comment naming the loop-with-explicit-slot case, so a future refactor doesn't accidentally regress back to the min-across-everything shape.

Not a blocker — the new behavior matches author intent and I couldn't find any test pinning the old truncation.

Nits

🟡 analyzeClipMediaFit docstring narrows to "audio source" but the signature is still tag-agnostic.

packages/engine/src/services/videoFrameExtractor.ts:1268-1273 — the docstring changed from "clip's source shorter" to "audio source shorter", but the function still takes {slotSeconds, mediaSeconds, loop} with no tagName param. A downstream @hyperframes/engine consumer passing video params will still receive a shortfall value; only the callers (validate.ts + checkBrowser.ts, both narrowed to audio[data-duration]) enforce the audio scope.

Either add a tagName param and guard, or rename to analyzeAudioClipMediaFit — pick whichever matches the intent. Otherwise the docstring is misleading about behavior. Non-blocking; the in-repo callers all work.

Green notes

🟢 HDR/SDR now genuinely agree. The HDR compositor at packages/producer/src/services/hdrCompositor.ts:181 already implemented hold-last-frame via const effectiveIndex = Math.min(videoFrameIndex, frameSource.frameCount) — "a composition that outlives the source clip freezes on the last frame". Pre-PR, SDR was the outlier; post-PR, SDR's getFrameIndexAtTime(…, holdLastFrame=true) catches up. HDR ignores loop and always holds, but that's a pre-existing quirk (looping HDR clips freeze rather than wrap) — orthogonal to this PR.

🟢 isHeldVideoTail / canSeekEndedVideoBackward gates are safe. Both require isNonLoopVideo, so looped video takes the existing wrap path unchanged. The relTime >= clip.mediaStart guard in canSeekEndedVideoBackward is provably safe: with playbackRate > 0 and the outer isActive's timeSeconds >= clip.start, relTime = (timeSeconds - clip.start) * rate + mediaStart >= mediaStart always holds. The [0, mediaStart) slice you might worry about is unreachable inside the active window.

🟢 if (el.loop) continue; preserved in compile phase 2 at both packages/core/src/compiler/htmlCompiler.ts:74 and packages/producer/src/services/htmlCompiler.ts:497. Loop clips' authored data-duration survives compile intact. Compile + runtime now agree on loop-video slot honoring.

🟢 audio branch of resolveRuntimeMediaClipDuration still uses Math.min(source, host, explicit). Audio remains source-bounded regardless of data-duration overshoot; the Audio "…" compile-time warning at packages/producer/src/services/htmlCompiler.ts:502-514 still fires for audio-only shortfalls. Audio contract genuinely unchanged.

🟢 getFrameAtTime (public SDR export) preserves the non-hold default. holdLastFrame = false is the default at the top-level function; only FrameLookupTable.getFrame / getActiveFramePayloads opt into true. Existing downstream users of getFrameAtTime see the same "return null past source end" semantics. New test at videoFrameExtractor.test.ts:1552-1554 pins this.

🟢 CLI validate narrowing is complete. No other in-repo caller expects video[data-duration] shortfall warnings — the producer's compile-side warning was already gated by shouldClampResolvedMediaDuration(r.tagName, ...) (audio-only), and lint has no video-shortfall check. No downstream path left dark.

🟢 tagName === "VIDEO" casing is safe workspace-wide. All comparisons uppercase, all selectors lowercase (HTML case-insensitive), no XHTML entry points. Runtime + init consistent.

🟢 Hostile counterexamples verified:

• Omitted data-duration on a standalone video: phase 1 probes and injects source duration. ✓ (pinned at htmlCompiler.test.ts:127-131)
• Omitted data-duration inside a longer composition: same phase-1 path. ✓ (pinned at :115-125)
• Source-duration > slot-duration: shouldClampMediaDuration(declared, max) returns false (declared < max), no clamp fires — slot is authoritative regardless. ✓
• Zero-frame source: sourceDuration filtered to null in refreshRuntimeMediaCache at media.ts:81, isHeldVideoTail guards on sourceDuration != null — no held-tail invented. ✓ (pinned at videoFrameExtractor.test.ts:559-577)
• loop=true + explicit slot: covered by concern above; behavior did change, just in the direction that matches author intent.

What I didn't verify

• The regression-shards / Tests on windows-latest / CLI: npx shim (windows-latest) checks are still in-progress at review time. If any go red, they're likely to be the recurring Windows/producer env-flake surface seen across recent PRs — flagging so you can confirm.
• I didn't run the local engine suite (-fps_mode env caveat noted in the PR body).

Verdict framing

Solid three-layer fix. Compile / runtime / extractor all funnel through the same tag-based split, and the HDR side already had this behavior — SDR was the outlier, now caught up. One meaningful contract-body concern (loop-video slot behavior IS changed, docs say it isn't) + one nit on analyzeClipMediaFit's docstring drift. LGTM from my side once the contract statement + a loop-pin test land.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Re-verified at b79f6477. Δ from 44d355600: 2 files / +16 / -3 — the loop-with-explicit-slot pin + analyzeClipMediaFit docstring rewrite.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 R1 concern closed. New test honors an explicit slot for a looping video instead of truncating it to one loop at packages/core/src/runtime/media.test.ts:215-225 pins resolveRuntimeMediaClipDuration({isVideo: true, sourceDuration: 1, hostRemaining: null, explicitDuration: 10}) === 10. The comment ("loop wrapping happens later in syncRuntimeMedia. Duration resolution must preserve the authored window that the loop fills.") documents the invariant so a future refactor to a single min-across-everything shape gets caught. PR body also updated: "Looped videos still loop, and preview now honors their full authored slot instead of stopping after one cycle." Wording matches the actual behavior.

🟢 R1 nit closed. analyzeClipMediaFit docstring at packages/engine/src/services/videoFrameExtractor.ts:1273-1277 now reads "Whether a media source is shorter than its data-duration slot by more than the compiler tolerance. The calculation stays tag-agnostic; current in-repo warnings call it for audio only because video slots may intentionally outlive their source and hold the final frame." Better than the two options I offered — it acknowledges the function IS tag-agnostic and names the caller-side (not signature-side) audio-only scope. Downstream @hyperframes/engine consumers passing video params still get shortfall values, but the docstring no longer misrepresents that.

🟢 Delta scope is clean. Only the two files above; no collateral changes to compile / runtime / SDR / HDR / CLI paths. All prior green notes carry through.

Verdict framing

Both R1 findings addressed with the right level of care — a real pin (not just a smoke-test) and a docstring that acknowledges the signature. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at b79f64774547569027c3f90a95a744d14bd34e29. Adversarial pass across the video-slot × source-duration × loop contract matrix; complementary to Rames's compile→runtime→SDR→HDR walk.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Contract matrix holds at all four quadrants.

Case	Runtime resolver	syncRuntimeMedia	SDR / compile	Pin
Omitted data-duration	mediaDuration = sourceDuration	normal play	phase-1 injects source	htmlCompiler.test.ts:24-41
Explicit == source	slot preserved	normal play	no clamp (declared ≤ max+ε)	implicit
Explicit > source, no loop	slot preserved, mediaDuration = explicit	isHeldVideoTail clamps relTime to sourceDuration, pauses element	holdLastFrame=true returns totalFrames-1	videoFrameExtractor.test.ts:392-521, media.test.ts:506-511
Explicit > source, loop	slot preserved	isNonLoopVideo=false bypasses hold; existing wrap at media.ts:220-224 fills window	HDR ignores loop (pre-existing quirk)	media.test.ts:215-225
Explicit < source	slot honored (author's shorter window wins)	end = start + explicit bounds isActive	shouldClampMediaDuration returns false when declared < max	implicit
Audio, any shape	[source, host, explicit].filter(...) — Math.min unchanged	source-bounded	shouldClampResolvedMediaDuration('audio', ...) still clamps + producer warning at producer/htmlCompiler.ts:502-514	htmlCompiler.test.ts:43-50, validate.test.ts:53-102

🟢 mediaStart interaction preserved. clip.sourceDuration at media.ts:81 is raw el.duration; relTime = (timeSeconds - clip.start) * playbackRate + mediaStart folds the trim offset in. Held-tail fires when relTime >= sourceDuration — i.e. at exactly (sourceDuration - mediaStart) / rate seconds into the slot regardless of trim depth. No off-by-mediaStart drift.

🟢 hostRemaining still bounds the video slot. init.ts:1796-1801 derives hostRemaining = inheritedStart + inheritedDuration - start from the nearest [data-composition-id] ancestor and passes it into resolveRuntimeMediaClipDuration. The video branch's candidates [mediaDuration, hostRemaining] mean a 10s authored slot inside a 3s composition still ends at 3s — the fix doesn't let an authored slot outlive its host.

🟢 Top-level getFrameAtTime export preserves non-hold default. Only FrameLookupTable.getFrame / getActiveFramePayloads opt in to holdLastFrame=true (videoFrameExtractor.ts:1343, 1412). Downstream @hyperframes/engine consumers of getFrameAtTime still see return null past source end. Pinned at videoFrameExtractor.test.ts:1552-1554, 1556-1558.

🟢 Zero-frame-source guard. refreshRuntimeMediaCache filters sourceDuration to null when el.duration <= 0; isHeldVideoTail guards on sourceDuration != null; FrameLookupTable.getFrame returns null when totalFrames <= 0. No held frame is invented when there's nothing to hold. Pinned at videoFrameExtractor.test.ts:416-434 and media.test.ts:559-577.

🟢 CLI validate + producer compile warning narrowed correctly. auditClipDurations at validate.ts:144 now selects audio[data-duration] only; the warning text ("the slot is shortened to the media length when rendered") is now audio-accurate. Producer's compile-side warning at producer/htmlCompiler.ts:499-515 is gated by shouldClampResolvedMediaDuration(r.tagName, ...) → audio-only. Video shortfall no longer surfaces as a warning — matches the intentional hold-final-frame behavior. Pinned at validate.test.ts:53-102.

🟢 Loop pin test carries the invariant forward. media.test.ts:215-225 explicitly documents that resolveRuntimeMediaClipDuration({isVideo: true, sourceDuration: 1, hostRemaining: null, explicitDuration: 10}) === 10, with the comment naming why ("loop wrapping happens later in syncRuntimeMedia. Duration resolution must preserve the authored window that the loop fills."). A future refactor to a uniform Math.min shape gets caught.

🟢 PR body wording accurate. "Looping mechanics are unchanged, but preview now honors a looped video's explicit authored slot instead of truncating it to one source cycle." That's the exact runtime delta — Rames's R1 caught the earlier phrasing.

Verdict framing

Solid three-layer fix. Every quadrant of the (video/audio × explicit/omitted × loop) contract matrix has a runtime path that agrees with the compile-time treatment, and every behavior-change edge has a pin test. HDR pre-existing agreement; SDR + preview now caught up. No blockers, no concerns, no nits from my lens — everything Rames flagged at R1 landed cleanly at R2. LGTM.

— Review by Via

[vanceingalls]

R2 at cdc3370ca — delta from b79f64774 is the single golden baseline update Magi described (style-15-prod authored slot 14s, prior baseline had the source-truncated 13.88s from the pre-fix behavior). Reviewed the compare diff: only the golden file changes, no production-code touch, no test-logic touch. This is the expected shape when a bug fix invalidates a snapshot that was capturing the buggy output.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Baseline update is the correct direction. The style-15-prod composition authored data-duration=14 on a source-shorter clip. Pre-PR, the compiler silently clamped the resolved duration to 13.88 (source length); the old baseline was capturing that buggy clamp as ground truth. Post-PR, the compiler honors the authored 14s slot. Updating the baseline to 14 is the mechanical follow-through — not a band-aid.

🟢 Delta is single-file, single-golden-line (no production-code changes, no test-logic changes, no cross-touch). Focused harness compilation reports 0 errors and resolves the composition to 14s.

🟢 Prior R1 verification still holds. Contract matrix at b79f64774 (video/audio × explicit/omitted × loop) was already verified; the golden bump doesn't affect any of those quadrants.

Verdict framing

APPROVE. Baseline update is the correct mechanical follow-through of the fix, not a symptom-silencer. Re-approving at cdc3370ca to restore the merge-gate after require_last_push_approval dismissed my prior stamp.

— Review by Via

[james-russo-rames-d-jusso]

Re-verified at cdc3370c. Δ from b79f6477: 1 file / +1 / -1 — the style-15-prod golden line update.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Golden flip is in the right direction and targeted. packages/producer/tests/style-15-prod/output/compiled.html:727 moves data-duration="13.88" data-end="13.88" → data-duration="14" data-end="14". The src file (packages/producer/tests/style-15-prod/src/index.html) has data-duration="14" authored on the aroll-video with a 13.88s source. Pre-PR that hit shouldClampMediaDuration(14, 13.88) = 14 > 13.88 + 0.05 → true, clamping the golden to 13.88 (the bug's output). Post-PR shouldClampResolvedMediaDuration("video", …) returns false, so the authored 14s slot survives — the new golden pins post-PR correct behavior, not a regression.

🟢 No sibling goldens left dark. Checked the other production-style baselines that carry 13.88 in their compiled video tags — style-11-prod, style-16-prod, style-17-prod. All three have src data-duration="13.88" authored on their aroll video against a 13.88s source (compositions also 13.88s). Authored slot = source length → no shortfall, nothing to clamp, no golden change needed. Only style-15's authored 14 vs source 13.88 hit the old clamp path.

🟢 Delta scope is clean. Only the one golden line; no code changes; all prior R1+R2 findings still closed.

Verdict framing

Correct golden update in the direction of the fix. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R3 at 22221fe01 — same pattern as the previous style-15-prod golden update. Delta from cdc3370ca is two golden baseline updates (style-2-prod + style-4-prod), both authoring 17s video slots against 16.043s sources. Only the goldens change; no production-code touch, no test-logic touch.

Blockers / Concerns / Nits

(none)

Green notes

🟢 Same mechanical follow-through as the previous golden bump. Two more compositions authored data-duration values greater than their source length; their goldens were capturing the pre-fix source-clamp behavior; updating the baselines to the authored slots is the correct direction, not a band-aid.

🟢 Delta scope-clean. Two files, both under the style-N-prod golden trees. No production code, no test logic. Focused harness compilation now passes both with 0 errors.

🟢 Prior R1+R2 verification still holds. Contract matrix at b79f6477 was already verified; each golden bump is a fresh mechanical follow-through of the same fix, not a scope expansion.

Verdict framing

APPROVE. Consistent with the R2 stamp on the style-15-prod update — same pattern, same direction, same "baseline was capturing the buggy pre-fix output" rationale.

— Review by Via

[james-russo-rames-d-jusso]

Re-verified at 22221fe0. Δ from cdc3370c: 2 files / +2 / -2. Style-2 (17 authored, 16.043 compiled → 17) + style-4 (same shape) golden updates.

Blockers

(none)

Concerns

🟠 Four more goldens still encode the old source-truncated behavior — will red the next CI run. Swept every packages/producer/tests/*/output/compiled.html for videos with a decimal data-duration and cross-checked against src-authored slots. Post-PR the compile preserves the authored slot for video, so any golden where src-authored > compiled is a stale pin. Confirmed:

Test	src authored	compiled at 22221fe0	needs
style-7-prod (composition main.html)	data-duration="16.7"	data-duration="16.043" (aroll)	flip to 16.7
style-12-prod (src/index.html)	data-duration="14"	data-duration="13.88" (a-roll-video)	flip to 14
style-13-prod (src/index.html)	data-duration="16"	data-duration="13.88" (aroll-video)	flip to 16
style-18-prod (src/index.html)	data-duration="14"	data-duration="13.88" (a-roll)	flip to 14

Also verified — the following do NOT need updates (compiled matches src-authored, no shortfall): style-3-prod (16.04 = 16.04), style-5-prod (16.04 = 16.04), style-8-prod (16.04 = 16.04), style-9-prod (16.04 = 16.04), style-11-prod (13.88 = 13.88), style-16-prod (13.88 = 13.88), style-17-prod (13.88 = 13.88), hdr-regression (1.5 = 1.5), hdr-hlg-regression (2.5 = 2.5), overlay-montage-prod (42.37 injected from source, no authored slot). And non-style-N-prod producer tests have no decimal video data-durations.

So R5 should be a single push updating those four golden lines to their authored values. That should close the cycle instead of another shard-by-shard discovery loop.

Nits

(none)

Green notes

🟢 R4 flips are correct. Both style-2-prod (src 17, compiled 16.043 → 17) and style-4-prod (same shape) — src files confirmed authored at 17, source is 16.043, pre-PR clamped (14 > 16.043 + 0.05 → true), post-PR preserves. Golden aligns to post-PR correct behavior, not a regression.

🟢 All R1+R2+R3 findings still closed — code delta empty since R3.

Verdict framing

Directionally correct; content-side sweep needs one more targeted pass. Once the four remaining style-N-prod goldens flip, the whole cycle should settle.

— Review by Rames D Jusso

[vanceingalls]

R4 at fb27035fe — scope reshape from Magi. Instead of continuing to bump goldens as more style fixtures surface with source-shorter videos, this reverts the three earlier golden flips (style-2-prod, style-4-prod, style-15-prod) and removes data-duration from the 7 fixture compositions with source-shorter videos. Rationale: these fixtures are visual smoke tests, not explicit-slot contract tests; the hold-last-frame invariant stays covered by dedicated regression tests. Verified below.

Blockers / Concerns / Nits

(none)

Green notes

🟢 Reshape is scope-clean. Delta from 22221fe01 → fb27035fe touches only fixture INPUT + OUTPUT files under packages/producer/tests/style-N-prod/. Zero production code, zero test-logic changes, zero runtime code touched. File-list: 7 INPUT edits (style-2/4/7/12/13/15/18) each removing exactly one data-duration attribute from a <video> tag, plus 3 OUTPUT golden reverts (style-2/4/15/output/compiled.html). Every INPUT patch is a single-line deletion of the data-duration attribute — no other attribute changes, no timing changes, no src changes.

🟢 Hold-last-frame regression tests are intact at the new head. packages/engine/src/services/videoFrameExtractor.test.ts still carries the four dedicated SDR hold-last-frame tests at lines 392, 451, 475, 496 ("holds the last frame for a non-looping clip until its authored slot ends", "holds the last frame at the inclusive clip end", "holds the last frame across the tail when the source is shorter than the window", "holds the last frame when the source is a sub-frame shorter than the slot"). packages/core/src/runtime/media.test.ts still has the runtime-side coverage: authored-hold-tail seek at :506 ("seeks a non-looping video to its final frame when entering an authored hold tail"), the loop-preserves-slot test at :215 ("honors an explicit slot for a looping video instead of truncating it to one loop"), and the top-level compositor test at :805 ("holds the final frame instead of looping a non-looping video"). Dedicated coverage carries the invariant forward independent of the visual fixtures.

🟢 The three previous golden flips reverted correctly. style-2-prod/output/compiled.html: 14/17 back to 16.043 (source length); style-4-prod/output/compiled.html: 17 back to 16.043 (source length); style-15-prod/output/compiled.html: 14 back to 13.88 (source length). Since the fixture INPUTs no longer author data-duration, the resolved OUTPUT correctly reflects intrinsic-source behavior, not authored-slot behavior. For style-7/12/13/18 the INPUT-authored data-duration matched the source integer exactly, so removing the attribute produces the same compiled numeric — no OUTPUT delta needed, expected and correct.

🟢 Rationale holds under audit. The style-N-prod fixtures are consumed by the PSNR-based regression harness in packages/producer/src/regression-harness.ts (compile source → render → PSNR-check against output.mp4 at checkpoint frames). compiled.html is a golden of the compiler output, not an explicit-slot contract. The hold-last-frame invariant lives in dedicated unit tests as noted above. Removing an incidental data-duration attribute from fixture INPUTs decouples the visual-example set from the hold-last-frame contract without weakening either.

Verdict framing

APPROVE. Reshape is well-scoped — fixture-only, contract tests intact, rationale sound. Stamp-flapping across 4 rounds (R1 APPROVE → R2 golden bump → R3 two more golden bumps → R4 reshape) has settled on a maintainable shape: visual fixtures no longer over-specify data-duration and the hold-last-frame contract is carried by dedicated tests, so future source-shorter fixtures won't force golden churn.

— Review by Via

[james-russo-rames-d-jusso]

Re-verified at fb27035f. Δ from 22221fe0: 10 files / +3 / -10 — design pivot on the fixture side.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 The pivot is coherent and rendered output is unchanged for all 7 fixtures. Removing the explicit data-duration from the seven <video> elements (style-2, -4, -7, -12, -13, -15-composition, -18) drops them onto the phase-1 injection path, which stamps the probed source duration. That produces the same compiled data-duration the old clamp used to produce, so:

Fixture	Pre-PR compiled (old clamp)	Post-R5 compiled (phase-1)	Match
style-2	16.043	16.043	✓
style-4	16.043	16.043	✓
style-7	16.043	16.043	✓
style-12	13.88	13.88	✓
style-13	13.88	13.88	✓
style-15	13.88	13.88	✓
style-18	13.88	13.88	✓

The three R3/R4 golden flips (style-2, -4, -15) revert cleanly to their intrinsic-source values, and the other four goldens (style-7, -12, -13, -18) are left untouched because they were already source-length under the old clamp path — nothing needed to change. output.mp4 blobs for style-12/13/15/18 are byte-identical to main; the drift on style-2/4/7 traces to unrelated already-merged PRs (#1869, #2490) landing on the PR branch, not this commit.

🟢 Composition-layer slots preserved. The outer composition-layer <div data-composition-id="aroll" ... data-duration="14"> in style-15-prod (and the equivalent wrappers in the other six) still carries the full authored composition duration — only the inner <video> lost its slot. So the visible composition timeline in each fixture is unchanged; the video simply plays through intrinsic length as it always did under the old clamp path, and no new hold-last-frame behavior kicks in for these (correctly, since none of them opt in via an authored slot).

🟢 R1 + R2 closures intact. The media.test.ts:215 pin honors an explicit slot for a looping video instead of truncating it to one loop survives, and the analyzeClipMediaFit docstring at videoFrameExtractor.ts:1273-1277 retains its tag-agnostic phrasing. No collateral damage to earlier commits.

🟢 Rationale is defensible. These fixtures were authored as visual composition examples, not as contract tests for the explicit-video-slot case — the data-duration="14"/17/16.7 on their aroll videos were incidental (someone eyeballed "this video is about 14 seconds"). Reserving explicit-slot coverage to the dedicated unit pins (core/media.test.ts, engine/videoFrameExtractor.test.ts around :1550) and letting the fixtures show intent-clean intrinsic behavior is the correct scope split. No producer-regression fixture now exercises "explicit slot > source" end-to-end, but the unit-level pins carry that load already and adding a dedicated fixture is a nice-to-have that would only add CI cost.

Verdict framing

Design pivot lands cleanly — chose to declassify the fixtures rather than pin them as contract tests, output is byte-equivalent, unit coverage unchanged. LGTM from my side. Waiting on regression-shards to confirm no shard I missed comes red on the new SRC edits, but structurally this should close the golden-drift loop.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Re-verified at 4cbde3830 on main@71d7be792. Rebase-only push — git range-diff fb27035f...4cbde3830 marks all four PR commits (2e8f871b, 31cc2fe8, 1284703a, 4cbde383) = patch-equivalent to their pre-rebase counterparts. The 14 new commits below them are main advancement (sdk attachSync re-sync, engine protocolTimeout, producer parity telemetry, core applyPositionEdits guard, etc), all orthogonal to the video-slot fix.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 R1–R5 findings all still closed. No delta in the 4 PR commits themselves; the loop-slot pin, docstring rewrite, style-15 golden, style-2/4 goldens, and the R5 intrinsic-duration pivot on 7 fixtures are byte-identical to their pre-rebase forms.

🟢 Rebase is coherent with the fix's contract. The main-advancement commits pulled in touch positionEdits, engine timeout escape hatches, and streaming-encode gating — none of them mutate the video-slot resolution path, resolveRuntimeMediaClipDuration, getFrameIndexAtTime, hdrCompositor.ts:181, or analyzeClipMediaFit. Nothing to re-verify on the fix side.

Verdict framing

Rebase-only. LGTM from my side stands.

— Review by Rames D Jusso

[vanceingalls]

R5 at 4cbde3830 — pure rebase onto main@71d7be792 (per Magi). Independently verified: per-commit file lists and counts identical to R4 (c1:15, c2:1, c3:2, c4:10 files; c4 = 7 fixture INPUTs + 3 OUTPUT golden reverts), per-file content byte-identical at spot-checks. Patch-equivalent to prior R4-stamped state. Only base advanced.

Blockers / Concerns / Nits

(none)

Green notes

🟢 Pure-rebase claim confirmed independently. Verified via gh api /repos/.../compare/<parent>...<commit> for each of the 4 PR commits at both R4 and R5 chains — file counts match per-commit (c1:15, c2:1, c3:2, c4:10). Per-file content read via gh api .../contents?ref=<sha> at fb27035fe (R4 head) and 4cbde3830 (R5 head) is byte-identical (sha256 match) for sample files across all three change regions: fixture INPUT (packages/producer/tests/style-2-prod/src/index.html), core runtime (packages/core/src/runtime/media.ts), engine service (packages/engine/src/services/videoFrameExtractor.ts). Per feedback_gh_compare_rebased_branches_caveat, I bypassed compare base...head (which shows base-advance noise) and used per-commit compare + per-file content reads.

🟢 Prior R4 verification carries forward. Scope-reshape (7 fixture INPUT data-duration removals + 3 OUTPUT golden reverts in style-15/2/4-prod/output/compiled.html) unchanged. Hold-last-frame contract tests intact.

🟢 Base advance is fresh main. 71d7be792 is the current main branch tip (Merge PR #2501, 2026-07-16T02:07:05Z). Standard restack, not a scope change.

Verdict framing

APPROVE. R5 in a stamp-flapping sequence (R1 contract, R2/R3 golden bumps, R4 scope reshape, R5 rebase). Scope reshape at R4 was the right settle-point; this rebase keeps it aligned with current main. mergeStateStatus=BLOCKED reflects in-flight required checks on the fresh push, not review state — expected and will resolve as CI completes.

— Review by Via