Repository navigation
fix(core): a split sub-composition plays its videos from its in-point in preview and timeline - #5413
Conversation
Edit accuracy: accurate 2061 (base branch 2061), smooth 1622 of thoseThe gate passes. Quarantined, measured but not gated (0) |
c7f7fcc to
d848c42
Compare
48406e4 to
f785461
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes at f7854617. The origin model (hostStart + (local - inPoint)) and the slot cut look right to me. I checked the straddling clip, the loop, the speed ramp, the fade-in, nested hosts and the no-id host against the code and against the new tests, and the no-in-point parity guard holds up when run against main's code. One problem blocks: the audio clock still sees the uncut window, and that can freeze playback where main plays on. Details below.
Blocker
1. The playhead can stop before an in-point host starts, because the audio clock picker uses the uncut window. packages/core/src/runtime/init.ts:4570-4588 (followedOrLongestRunningAudio)
This PR places a clip from the host's origin, so a clip the in-point cuts at its head now has a resolved start before the host begins. followedOrLongestRunningAudio still builds the window from resolveAbsoluteMediaStartSeconds plus data-duration and never applies the host slots. So during [clip's own start, host start) it can pick an audio element that syncRuntimeMedia will never play there. If that element has readyState < HAVE_FUTURE_DATA, line 4618 treats it as buffering and pins the clock to state.currentTime. The element is never asked to play, so the hold only ends if something else gets it to buffer.
Repro, in the same harness audioClockSource.test.ts uses:
<div data-composition-id="main" data-root="true" data-start="0" data-duration="10">
<div id="half" class="clip" data-composition-id="half" data-composition-file="compositions/scene.html"
data-start="5" data-duration="5" data-playback-start="5">
<div><audio id="a" data-start="2" data-duration="8" src="/assets/a.mp3"></audio></div>
</div>
</div>With the audio paused at readyState 0, I pressed play from 0 and stepped 4 s and then 10 s of frames:
- this branch:
getTime()stays at2.0both times, anda.playis never called - main: the clip resolves to 7 s, nothing holds the clock, and the playhead moves on (passes
> 3.5at 4 s) - with
readyState4 the branch does not stall (the picker detaches)
You can reach this shape in Studio by splitting a scene and deleting the first half without closing the gap, or with any host given an in-point by hand. When both halves are present, the first half's copy of the same element usually wins the pick and is playing, which may be why the split fixtures don't show it.
Suggested fix: cut the picker's window the same way the media cache does, and keep the uncut start as compositionStart for the clock mapping:
const start = resolveAbsoluteMediaStartSeconds(el);
const durAttr = parseStrictFiniteTimingNumber(el.dataset.duration);
const kept = cutToHostSlots(
{ start, end: durAttr != null && durAttr > 0 ? start + durAttr : Infinity },
resolveMediaHostSlots(el),
);
if (!Number.isFinite(start) || kept.end <= kept.start) continue;
if (!isInClipWindow(state.currentTime, kept.start, kept.end)) continue;hardSyncAllMedia (line 4788) reads the same uncut window. That one only seeks a paused element, so it is harmless, but cutting it the same way would make every reader agree.
Worth a look
2. Studio's clip list and hyperframes timeline disagree for an in-point host, and Studio's rows can go negative. packages/core/src/runtime/timeline.ts:434-437 vs packages/cli/src/timeline/describeProject.ts:286-290, 377-381
collectRuntimeTimelinePayload places nested rows from the origin but doesn't cut them. The CLI cuts and drops them. Same fixture in both: host data-start="0" data-duration="7" data-playback-start="5" holding v1 (local 0 to 4), t (a div, local 1 to 3) and v3 (local 8 to 12):
- Studio payload:
half 0+7,v1 -5+4,t -4+2,v3 3+4 hyperframes timeline:v3 3..7,half 0..7.v1andtare dropped.
The body says the Studio rows are placed from the origin on purpose, so the placement may be intended. Negative starts in that payload are new, though: main clamps at 0. Is the Studio timeline fine with a nested row at -5 s? If not, cut them (or drop the fully cut-away ones) the same way the CLI does.
Test gaps (mutants that survived)
I ran 8 single-line mutants against the core runtime tests plus the CLI timeline tests. 6 were caught. These 2 survived:
media.ts:461: changing the fade length back toclip.duration, so a head-cut clip's fade-out lands early by the cut amount, passes everything. Only the fade-in is pinned. A test with a straddling clip that hasdata-fade-outwould cover it.init.ts:4840: deletingif (kept.end <= kept.start) continue;passes everything. The decoded path still refuses on its own math, but the media-element route then callsscheduleMediaElementPlaybackfor a clip that never plays. Nothing pins the "never scheduled" claim for that route.
Nits
media.ts:345:startsEarlyreadsrateAt(clipRate, 0). For a head-cut audio clip with a speed ramp, the rate at the cut point israteAt(clipRate, clip.start - origin). It only affects the cue-ahead margin.- In the CLI, a cut row keeps its uncut
start/end/duration, sodurationno longer equalsabsEnd - absStart(above:v3isabsStart 3, absEnd 7, start 8, duration 4). The text output reads fine ("3-7 (local 8-12)"). A--jsonconsumer that assumesabsEnd - absStart == durationwould be wrong, so a short note on the field might help.
What I verified
- Parity guard against main: I checked out main's six source files under this branch's tests.
init.noInPointParity.test.tspasses 13/13. The new placement, resolver, audio and CLI tests fail on main (10 core, 1 CLI), as the body says. - No-in-point behavior of the
startResolver.tsrefactor, checked by reading the code: the duration chain only ever yields a positive number or null, so??matches the old> 0checks. The inherited-host branch is unchanged.hostOffset + max(0, v)stays non-negative without an in-point, so dropping the outer clamp changes nothing. The relative-start floor is 0 whenever the origin is not negative.hostStart !== 0only differs from> 0for a negative origin. - Preview vs CLI on the split fixture (12 s scene split at 5 and 9): the runtime source times (
scene-splitv3 at 8.5 s gives 2.5,scene-split-splitv3 at 10 s gives 4) match the CLI windows ([8, 9],[9, 12]). - Decoded audio with a head cut:
from = max(elapsed, headCut, 0), offsetfrom * mediaRate + mediaStart, delayed by(from - elapsed) / globalRate. Ramped clips never reach this path, so a constant rate is correct here. WithheadCut = 0it reduces to main's two branches. - Test runs: the PR's core test files pass (367). The full core suite passes 4552, with 1 skipped. The CLI timeline suite passes 106.
- CI at this head: 59 checks passed and 7 were skipped. The Studio edit-accuracy shards and one regression shard were still running. No failures so far.
All of this was verified with unit tests and local runs in the test harness, not with a production render or a real browser. The clock stall in particular is reproduced in the runtime harness. In a real browser, how long it lasts depends on when that element buffers.
— Rames
|
Thanks, all addressed in d69e608:
One thing not exercised: a row that straddles the cut now shows in Studio's clip list from the host's start. I haven't driven a drag of such a row in Studio, so whether a drag writes its start from the listed position is unverified. |
…review-timeline # Conflicts: # packages/core/src/runtime/startResolver.ts
jrusso1020
left a comment
There was a problem hiding this comment.
Approve at 23c153fe. This clears my change request from f7854617.
The blocker is fixed. I re-ran my original repro (audio in an in-point host, readyState 0, play from 0). getTime() now reaches 4.0 at 4 s, where it used to freeze at 2.0. It holds at 5.0 only once the audio enters its played window, and that is real buffering. The new audioClockSource test is that repro, and it fails with the fix commits reverted (as do the cue-ahead test and both timeline.inPoint tests).
Claims checked:
playedMediaWindow(init.ts:985-989) drives the clock picker and hard sync. Both still map time with the uncut start, which is correct. With a host at 5 s, in-point 5 and audio atdata-start="2": pausing at t=3 leaves the element alone, and pausing at t=6 lands at 4.- Studio's clip list (
timeline.ts:453-460) now matcheshyperframes timelineon my earlier fixture (half 0+7, v3 3+4), with no negative starts. I probed no in-point, in-point 0, content past the host's end, and two-deep nesting, and all came out right. - A nested host cut away entirely stays as a zero-length row, so its clips stay nested.
- The main merge's only real conflict was in
startResolver.ts. It keeps the??chain and adopts main's duration helper, which is equivalent. Git's automatic merge differs from the merge commit only in that hunk. - Core passes 4568 tests and typecheck is clean. The two mutants that survived last round (the zero-length kept window and the fade length) are now killed. All 11 required checks are green.
Should-fix (non-blocking): the hard-sync change (init.ts:4803-4804) has no test. Reverting it to the uncut start keeps the runtime suite green. A test with the setup above (pause at t=3: currentTime untouched; pause at t=6: currentTime 4) would pin it.
Nits:
- A nested host cut away at its tail gets its zero-length row at its own uncut start, which can fall after the parent's end. Example: a host at 0 s, 5 s long, in-point 2, holding a host at local 8 puts the row at 6. That row also feeds
maxEnd. Clamping it into the parent's slot would fix it. It also means the test name's "ashyperframes timelinedoes" is not quite true here, since that command drops such rows. - The picker's
runsUntil(init.ts:4591) still uses the uncut length. A clip cut at its tail can win "longest" over a root clip that actually plays longer, and the clock then switches source when the host ends. I haven't reproduced this.
Checked with unit tests and the runtime harness, not in a real browser.
— Rames
…review-timeline # Conflicts: # packages/core/src/runtime/media.ts
vanceingalls
left a comment
There was a problem hiding this comment.
Approve at 9a57c6ed.
Verified:
- The delta since @jrusso1020's approval at
23c153feis only the merge of main (#5434, #5431, #5440). The one overlap ismedia.ts, where #5440'sMIN_NATIVE_PLAYBACK_RATEfloor readsbaseRate = rateAt(clipRate, t - origin). With the in-point origin, a ramped clip under a split half gets its floor check at the correct rate. - Without an in-point nothing changes.
webAudioTransport.startBoundedSourcewithheadCut=0reduces exactly to main's two branches (start offset, delay and remaining length).startResolver.computeStartkeeps a non-negative result whenever the origin is >= 0.hostInPointSecondsreturns 0, so no host slots are built andcutToHostSlotsis a no-op in the runtime and the CLI.init.noInPointParity.test.tspins this. - Host slots are found by composition attributes only (
isCompositionHost). A plain<video data-playback-start>trim never becomes a slot, and an inner root that takes its timing from its host is skipped, so the in-point counts once. - All required checks pass. The Studio edit-accuracy shards are still running and aren't required.
Non-blocking: Rames's earlier should-fix still applies. The hard-sync playedMediaWindow change (init.ts:4804) has no test.
Verdict: APPROVE
Reasoning: The origin and slot-cut model is consistent across preview, audio scheduling and the CLI timeline. Projects without an in-point resolve as on main, and the merge from main brings in no new interaction.
— Via
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at 9a57c6ed, which merges main (#5431, #5440) into 23c153fe, the head I approved.
I compared the merge against a plain git merge-tree of the two parents. The only conflict is the baseRate hunk in media.ts, and both sides survive it:
- The PR's
rateAt(clipRate, params.timeSeconds - origin)is kept, withorigin = clip.origin ?? clip.startin the same per-clip scope (:340). - #5440's floor gate
const playing = params.playing && baseRate >= MIN_NATIVE_PLAYBACK_RATEis kept too. - Everything downstream of #5440 (
heldBelowFloor, theMath.max(MIN_NATIVE_PLAYBACK_RATE, …)clamp, the!playingbranches) merged cleanly. The floor now uses the in-point-aware rate, which is what it should do. media.test.tsauto-merged.
All 11 required checks are green at this head. My earlier non-blocking notes (hard sync untested; tail-cut nested row position) still apply.
— Rames
What changes for a user
After splitting a sub-composition clip in Studio, the second half now plays the videos inside it from where the split happened, in the preview and in
hyperframes timeline. Before, Studio's split wrote each half's in-point (data-playback-start) correctly, but only the scene's animations honoured it: the videos inside started over as if the scene began at the second half's start.Example from the test fixture: a 12 s scene with trimmed videos, split at 5 s. Inside the second half, a video authored at 9 to 13 s of the scene:
Studio preview at 7 s, with a generated 12 s clip (red, then green, then blue, its own time burned in) inside one sub-composition split at 5 s:
Before
After
This is the first part of making a sub-composition's in-point shift and cut the media inside it (issue #3245). Snapshot and the renderer follow in their own PRs.
How
Only a sub-composition host with an in-point above 0 changes anything. Its local time 0 sits at the host's start minus that in-point (its origin), so everything timed inside it (media, images, text, nested scenes, and the clip rows Studio lists) is placed from that origin, the same way the scene's animations already were. The media inside plays only within the host's slot: what starts before the host is cut at its head, and the host's end cuts the tail. A clip that straddles the in-point starts at the host's start, further into its file, while its loop point, speed ramp, volume lanes and fades stay timed from the clip's own start. Nested compositions inside a split half are cut by every enclosing in-point host. Hosts are found by their composition attributes, so a host without an id counts.
Every host without an in-point, and every project without one, resolves exactly as on main on every path: start, end, audio scheduling (same-origin and cross-origin) and
hyperframes timeline. A guard test pins that.packages/core/src/mediaTiming.ts:hostInPointSeconds,compositionOriginSecondsandcutToHostSlots, shared by preview and the CLI timeline.startResolver.ts: host offsets come from the origin; enclosing in-point hosts are collected by composition attributes.media.ts,init.ts,webAudioTransport.ts: preview and audio scheduling cut to in-point hosts' slots; a clip an in-point host cuts away entirely is never scheduled.describeProject.ts:hyperframes timelineplaces and cuts rows the same way, only for hosts with an in-point above 0.Media marked as root time (
data-hf-media-start-basis="global") keeps its authored time. This revives the composition-origin model from an earlier attempt (#3826, closed as stale):hostStart + (local − inPoint), with root-time media left alone. Its renderer, snapshot and engine parts are left for the follow-up PRs.Not changed here, and kept as on main: a split's first half (in-point 0) behaves exactly like any unsplit host. So a clip with fades that straddles the split still fits both fades into the first half's remainder, and splitting a host that has no id still lets the first half's copy run past the split. Both come from how main treats any host without an in-point, and changing them is a separate decision. A host's playback rate still doesn't scale the media inside it. Render and snapshot run the same runtime, so they pick up part of this; their own placement code follows in the next PRs.
Checked
expected [ 9, 13 ] to deeply equal [ 4, 8 ]) and pass three runs in a row on Linux.init.noInPointParity.test.ts: 13 unsplit fixtures (1 to 3 levels of nesting, root length from its animation,data-end, relative starts, a host without an id, a host under a different id, root-time media, looping and faded clips, same-origin and cross-origin audio, a scene trimmed shorter than its file) record each element's start, its source time and volume at 12 seek points, and what both audio routes are asked to play. The expected values were produced by running the fixtures against main's code; the test passes on main and on this branch. A few other no-in-point tests in this PR also pass on main by design.