Repository navigation
perf(core): a jump starts its video first and rebuilds the clip list after the frame lands - #5761
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at d7f172d9, only this PR's own diff against perf/playback-scales-with-screen.
Media first. syncMediaForCurrentState() now runs before seekTimelineAndAdapters on a paused jump, and stays skipped on the keepPlaying path as before. mediaForceSyncNextTick is still set, so media in a scene that the page seek mounts gets synced on the next tick.
Clip-list hold.
postTimelineholds only when all of these are true: a paused jump is landing, render capture hasn't started, and a completion is pending. Load-time posts and render capture are never held, and the third test pins the "no jump" case.- Holds made during one jump collapse into one post.
seekCompletionsSettledre-reads the pending set on each pass, so a second jump that lands mid-hold is covered by the same post. registerSeekCompletionwraps every promise so that it can't reject. That meansPromise.allinseekCompletionsSettledalways settles, and_pendingFailureis still left forwaitForSeekCompletionto report.- The post after the hold re-checks
state.tornDownand goes back throughpostTimeline. If playback started in the meantime, the playback hold still applies.
Non-blocking. The hold lasts as long as the slowest pending completion. The media and proxy completions have caps (media.ts, mediaProxy.ts HOLD_CAP_MS), but frameSources.ts:114/143 registers source.ready and source.seek() as they are. A frame source that never settles would therefore hold the clip list until some later post finds nothing pending. Before this change that list posted at once. A cap at the hold, or on those registrations, would bound it.
The tests cover each change. No live run; I checked the source and CI only.
— Rames
d7f172d to
9183736
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at 91837362. git range-diff shows the patch is identical to d7f172d9, which I approved. It's only rebased onto #5754's fix (73de4fd8).
No live run.
— Rames
73de4fd to
d7824e1
Compare
9183736 to
816b8d5
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Holding at 816b8d50. The code is fine; the problem is CI.
git range-diff shows the patch is identical to the one I approved at 91837362 (91837362 = 816b8d50), so the code review stands.
The hold: the base is perf/playback-scales-with-screen. At this head only Preflight and the Perf jobs run, and the required contexts (Typecheck, Build, Test, runtime contract, the Windows jobs, Studio captures, the edit accuracy gate) haven't run. I'll approve once this PR runs them green against main.
— Rames
d7824e1 to
24cfc0f
Compare
816b8d5 to
f12f0f1
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at f12f0f1c. Range-diff against 816b8d50: this PR's own commit is unchanged; the stack below it gained #5741's c7d2f864 and #5754's matching isDiscoveryStamped carry into the cached scene clock (reviewed on #5754).
Holding with a comment because the base is still perf/playback-scales-with-screen (#5754, open), so the required contexts haven't run at this head. I'll approve once it targets main with required CI green.
No live run.
— Rames
24cfc0f to
9165514
Compare
f12f0f1 to
41e3850
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 41e3850e. Range-diff against f12f0f1c: this PR's commit is unchanged, and the stack below it is #5754 rebased onto main now that #5741 has merged.
Still holding with a comment: the base is still perf/playback-scales-with-screen (#5754, open), so required CI hasn't run at this head. For the record, my review at f12f0f1c was a COMMENT, not an approval. I'll approve once #5754 merges and this PR is on main with required CI green.
No live run.
— Rames
…after the frame lands
41e3850 to
4ec9d26
Compare
fdc40d8
into
perf/playback-scales-with-screen
What changes for the user
Jumping around a long film costs less work: in the 12-jump test, the page now does about 18% less main-thread work per jump, and the jump median improves about 5% (843-874 ms -> 801-859 ms on a 9-minute test film). This is the first of three seek steps; jumps are still bound by video decoding and the page's layout, which the next PRs address (preview proxies, then an instant still).
What this PR does
postTimelinenow holds the post and sends one after they settle and the frame paints. The hold applies only after a jump: at load, posts (andwindow.__clipManifest) land at once, and render capture is never held.seek-dispatchgains two read-only helpers (hasPendingSeekCompletion,seekCompletionsSettled), so the hold never consumes a failure thatwaitForSeekCompletionreports.Tests
init.seekOrder.test.ts. Each test fails without its change; I checked each by removing the change and rerunning:Core runtime suite: 2099 tests pass. Typecheck, lint and the comment ratchet pass. The new tests pass 3 runs in a row.
Measurement
Setup:
One base run was lost to a bug in my test rig: it clicked Play before Studio enabled the button (Play stays disabled for 0.2-1.9 s after the preview reports its duration). Fixed in the rig; not a Studio issue.
What a jump still spends, per profile:
Stacking
Stacked on #5754, which is stacked on #5741. Measured on a build that also includes the held-scenes stack (#5719, #5722, #5726): the hold covers the post at the end of
nestRanSceneswithout changing that code.