Repository navigation
perf(studio): zooming a long timeline re-decodes and redraws less - #5142
Conversation
A zoom that changed a strip's frame count decoded the whole strip again on the main thread, and showed the clip's single poster frame until it landed. Frames are now sampled at each slice's left edge, so a strip twice as long holds every frame of the shorter one; decoded frames are shared per source and time, so zooming out decodes nothing and zooming in decodes only the new half. The strip on screen stays until the new width is ready.
… zoom A zoom resizes every mounted clip, and every mounted waveform redrew in that same frame, on screen or not. A waveform now draws only while it is on or near the screen (one shared observer, a screen of margin each side) and draws when it comes near if a zoom changed it meanwhile.
Analyzing a long music track runs for over a second on the main thread, and when it started late it landed inside a zoom. The analysis now awaits an optional pause before each long stage; Studio's waits for the timeline to rest and the browser to go idle. Results are unchanged. The tempo detector's stage is one call, so a zoom that starts inside it still waits for it.
…how it A shared frame was keyed by the time a strip asked for, not the time it was decoded at, so a coarse strip's keyframe-early frame could land in a finer strip and run its frames backwards. A frame now carries its decoded time and is reused only when the strip would have accepted that keyframe itself. The frame-count ladder now tops out at a power of two (32), so its top step shares frames with the one below instead of none.
Left-edge sampling never showed a clip's final slice, so an end card or a fade-out was missing from its strip. Each strip now also decodes the clip's last frame (at its own time, never an earlier keyframe), shared across zoom steps like the others, and the last tile shows it. The strip on screen is now held per clip and range, not per props: a clip crossing between on and off screen keeps it, and a clip narrowed to one frame lets it go, so widening again never re-decodes a dropped width.
The comment ratchet allows no file a higher share of comment lines than on main; the added explanations move to the PR description.
Edit accuracy: accurate 2059 (base branch 2059), smooth 1538 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…t it mid-zoom The hold leased the strip on screen only once a new width was requested, so for one commit it had no lease; with the cache over budget it was evicted, the poster showed, and the old width decoded again. The strip on screen is now leased for as long as it is the clip's, and shown only while held. A frame that fails to decode takes the frame before it, so every slot keeps its time and the last tile still shows the end.
The hold checked the clip twice, in its lease and again before showing it; the lease alone decides now, and the cross-clip test pins it. A test now also pins that a filled frame is not charged to the cache.
terencecho
left a comment
There was a problem hiding this comment.
Review at 6910922aa2228e48a9586776abbca2d2aebef22e — requesting changes on the new beat-analysis idle contract.
Recheck idle after the lazy tempo-detector import. packages/core/src/beats/beatDetection.ts:234-237 awaits pause, then awaits loadBpmDetective() before entering its synchronous detect(audioBuffer) stage. On a cold import, a zoom can start while that module is loading; when it resolves, the expensive detector starts mid-zoom without consulting whenTimelineIdle again. I reproduced this deterministically by holding the lazy import, starting markTimelineMotion() after the first idle pause, then releasing the import: detect observed isTimelineMoving() === true. On a long track this is the ~1.2-second stage the PR is intended to keep out of zoom interactions. Please recheck pause after the import and before detect, and pin the delayed-import case in a test. This is an uncovered case in the newly introduced idle guarantee, not a claim that baseline analysis was already idle-aware.
I found no separate issue in the filmstrip frame-sharing/lease cleanup or the waveform visibility gate. Focused beat/waveform/motion tests and the deterministic race repro passed (18/18 in a disposable checkout); the thumbnail audit was read-only because dependencies were not installed there. Required checks passed. The optional Studio viewport gate failed at 69.0/58.3 ms interaction p95; its dense-short fixture contains div clips without media sources, so it does not exercise the changed thumbnail, waveform, or beat paths. A separate open #5151 addresses the wider resting overscan introduced by #5109; I did not count that optional failure as this PR's defect.
— tai
terencecho
left a comment
There was a problem hiding this comment.
Review at 2065b15fa91605fb67c4048c09b8b441015db9b1 — requesting changes for a separate filmstrip cache regression.
The delayed-import beat fix is correct: the timeline-idle pause now follows loadBpmDetective() and immediately precedes synchronous detection. The focused beat regression passes (6/6), and focused Studio motion/waveform/thumbnail tests pass (73/73). My earlier suggestion that the filmstrip path was clean was too broad; a base-versus-head reproduction exposed the issue below.
A new timeline session can reuse video frames decoded for an earlier version of the same asset. packages/studio/src/player/lib/thumbnailVideoDecoder.ts:242-250 keys sourceInfos and sharedFrames by source URL, fit and timestamp, then returns an all-shared result without reopening the source. The scheduler's request identity includes sessionEpoch, and its bounded cache retains released results, but the decoder never receives that epoch or an asset revision. On project A → B → A, when A's clip bytes change at the same /api/projects/A/preview/assets/clip.mp4 URL while B is open, the new session can return A's old object URLs and aspect. A deterministic scheduler+decoder probe with the same stable URL and changed dimensions (640×360 → 1280×360) reopened A on the PR base (04736006b1f448c0136fbe147c4bad7e66016b70) and returned the new frame/aspect; at this head it did not reopen A and returned the old frame/aspect. The source URL passed by useRenderClipContent.ts:215-240 is not revisioned in this path, and no production caller invalidates the project cache. Please scope the shared decoder entries to the session/asset revision or explicitly invalidate them on session change, with an A → B → A same-URL test.
All currently listed required checks pass. The optional Comments check fails its comment-share ratchet; that CI signal is not the reason for this changes request.
— tai
terencecho
left a comment
There was a problem hiding this comment.
Review at f800a70fc9f6723f1b59d6623e9f894be33803d8 — approved.
Both prior blocking findings are resolved. Beat detection waits for the lazy detector import, then waits for timeline idle immediately before synchronous detection; its focused tests pass 6/6. Video thumbnail decoding now scopes shared frames to the project/session content version and accepts cached source metadata only when that version matches. The same deterministic A → B → A test that failed on 2065b15fa91605fb67c4048c09b8b441015db9b1 passes here: after A's video changes at the same URL, it opens A again and returns the fresh frame/aspect. The new same-version sharing and disposal test passes; focused Studio tests pass 74/74.
Nonblocking follow-up: sourceInfos keeps one latest metadata entry per distinct source URL without a global bound or eviction. This is not a recurrence of the stale-version bug, but consider bounding it for long-lived sessions with many assets. Required Build/Windows checks were still pending at review time; the optional Comments check was red. Neither timing nor that comment-share ratchet is the basis for this code verdict.
— tai
terencecho
left a comment
There was a problem hiding this comment.
Review at 341337d5fa5d2a50d68d32066d28467d48db2bad — approved after the comment-only follow-up to my prior substantive review.
The entire delta from f800a70fc9f6723f1b59d6623e9f894be33803d8 removes two comments in packages/core/src/beats/beatDetection.ts; the delayed import, immediate idle pause, thumbnail content-version cache, and tests are byte-identical. No runtime path changed, so the earlier A → B → A cache reproduction and focused test evidence still apply. I did not rerun them for this comment-only head. The removed import/pause comment explained a subtle timing rationale; retaining that context elsewhere would be useful, but its absence is not a code blocker. The current viewport-gate failure is outside this comment delta and does not change the code verdict.
— tai
Verdict: APPROVE
Reasoning: The prior correctness fixes remain unchanged, and this follow-up only removes two comments to satisfy the comment-share check.
What
Zooming the timeline of a long project does less work each time a zoom lands:
Why
Profiling slider zooms on a 60-minute project showed what still ran on the main thread each time a zoom landed: filmstrip frames decoded and scaled again at the new width (the largest cost), every mounted waveform redrawn, and beat analysis of the music landing inside a zoom when it started late (over a second on long audio).
Numbers
Headless Chromium on a Linux box, no GPU, the 60-minute project. Frames per second during each input, main | before this PR (#5109 merged) | this PR, four alternated rounds:
Paired round by round, this PR was faster in 16 of the 20 runs, including all four pinch runs. The machine's load moved between 6 and 20 across rounds, so single runs are noisy. In the per-zoom profile, the zoom's own commit dropped from about 970 ms to about 490 ms over six zooms, and waveform drawing from about 330 ms to about 200 ms (same session, same build flags).
On this project most frames decoded during a zoom are posters for clips that just came into view (159 different clips over six zooms, two decoded twice), so frame reuse pays most on long clips at deep zoom.
How
thumbnailVideoDecoder.tssamples each slice's left edge (start + duration * i / n) and then the clip's last frame. Left edges are what let strips nest: each step of the power-of-two frame ladder contains every time of the one below it. Decoded frames are shared per source, fit and time, with a user count, and revoked when the last strip holding them is released.VideoThumbnailkeeps a lease on the strip on screen for as long as it belongs to the clip, and shows it while a new width decodes, so a cache over its budget cannot evict it mid-zoom. The hold is per clip and range: a change in on-screen priority keeps it, and narrowing the clip to one frame lets it go.AudioWaveformgates its draw on one shared IntersectionObserver (a screen of margin each side).analyzeMusicFromBuffer/analyzeMusicFromUrltake an optionalpauseawaited before each long stage. Studio passeswhenTimelineIdle, which resolves at an idle moment with no zoom moving the timeline.Known limits
Test plan
useMusicBeatAnalysispassing the pause has no hook-level test (no test renders that hook today); the core pause and the idle wait are tested.Before
main (with #5109): a pinch in and out, the slider, Ctrl+wheel and the zoom buttons on the 60-minute project.
before-zoom.mp4
After
This PR, the same zooms.
after-zoom.mp4