perf(studio): long clips mount only the picture tiles in view - #4993
Conversation
A clip's picture strip mounted one tile per frame width, so a 10-minute clip at full zoom mounted 18,000 tiles. The strip-size hook now also reports the strip's on-screen span (refreshed on scroll, resize and clip moves), and the video, image and composition strips mount only the tiles inside it.
The strip refreshed its span only on scroll, resize or a style change on its own clip, but a drag moves the ghost through a wrapper, so a long clip's ghost went blank past about 600 px of drag. Each strip now marks its unmounted ends and re-measures when either nears the screen, whatever moved it, and commits before paint. The tile row is one shared ThumbnailTiles for the video, image and composition strips.
Edit accuracy: accurate 1562 (base branch 1562), smooth 1235 of thoseThe gate passes. Quarantined, measured but not gated (0) |
With row virtualization off the timeline mounts every clip, and each strip had its own scroll listener and frame that read layout and then committed. A scroll jump changed every strip's span, so a 1,000-clip timeline laid out about once per clip per frame (the viewport gate's 1,000-row arm went to ~3 s frames). All strips now share one scroll listener, observers and frame: every position is read first, then every update commits in one batch.
…creen edge A strip's span is clamped to its own measured width, and each strip keeps its current size outside React so an unchanged size never calls setState. A clip wholly on or wholly off screen then does no work on a scroll; only strips straddling the edge re-render.
The shared frame read every mounted strip's position, so a timeline of 1,000 unvirtualized clips forced about 1,000 style-and-layout passes per scroll frame. An IntersectionObserver now tracks which strips are within 256 px of the screen; a scroll re-measures only those, and a strip is measured as it comes near. A clip wholly off screen mounts no tile.
…the same frame Each frame moves every strip's last measured box by its scroller's offset change and reads only the boxes that land within 256 px of the screen. A jump therefore measures exactly the strips it brings into view, with no blank frame, at the cost of a short scroll; strips far away get their span from the moved box without a layout read.
A strip up to 4,096 px wide keeps all its tiles, as before this change, and a scroll frame skips it; only long clips mount the tiles in view. Short clips gain nothing from virtualizing and paid a re-render each time they crossed the screen's edge.
Each strip remembers the scroll offset its box was read at, so a strip read between a scroll and that scroll's frame is not shifted again. The empty-span guards are removed: a near strip always starts inside itself and a far one already gets nothing.
somanshreddy
left a comment
There was a problem hiding this comment.
Review at 1ed54857 (full PR).
This is a comment, not an approval. The span math, tile placement and lifecycle hold up. I found one blank-strip path that persists past a single frame, plus some test gaps.
1. Medium: a visible long strip can go blank and stay blank after a move that isn't a scroll, followed by a scroll.
refresh() decides whether to read a strip from its remembered box shifted by the scroll delta (useThumbnailStripSize.ts:109-116). If that predicted box is far and the strip is showing, it gets NOTHING_IN_VIEW without a fresh read (:116). The remembered box only updates on mount, on a resize (RO), when presence IO reports a transition, or on a predicted-near read. A vertical move that keeps the strip's size and keeps it on screen updates none of these: tracks removed or collapsed above it, or the timeline panel resized. That leaves the box stale. The next scroll can then predict "far" for a strip that is on screen. Neither recovery path catches it:
- presence IO reports only transitions, and the strip never left (
:133-134); - the end sentinel now spans the emptied strip and fires, but that goes through
scheduleRefresh→refresh()again (:155). It trusts the same stale box, andshowingis now false, so nothing happens.
The strip stays blank until a later scroll brings the prediction near, or the strip leaves the screen and comes back.
Repro (scratch happy-dom test using the PR's own harness pattern): a 100,000 px strip is read at top=300. It moves to top=800 with no scroll and no resize. The timeline then scrolls down 600 px, so the strip's real top is 200 (on screen). The span becomes 0-0 and stays 0-0 after the gap callback plus its frame. Control: a presence-IO entry for the same strip (a fresh read) restores 0-2560.
Suggested fix: in refresh(), read a showing strip instead of evicting it on the prediction. That costs one read per strip with tiles mounted, which is bounded. Alternatively, have the gap observer measure its own strip rather than calling refresh().
2. Low: these survived mutation, so the tests don't pin them.
- Ignoring the vertical scroll shift (
top: box.top,:111) survives. The vertical-scroll path that row virtualization OFF relies on has no test. paddingLeft: (first + 1) * frameWsurvives (ThumbnailTiles.tsx:25). The tests prove the padding exists, not its value. A one-tile misplacement would ship green. An assertion that the first mounted tile sits atindex * frameWwould close this.NEAR_PX = 0(the 256 px band,:54) and<vs<=at the 4,096 px threshold (:29) both survive.
3. Nit: short strips don't re-render on a scroll, but they still do some work.
Every thumbnail, including sidebar CompositionsTab cards, calls acquire(). That installs a window capture-phase scroll listener, which fires for any scroll in the document, not only the timeline. Each strip also gets one presence observation and two gap observations. refresh() loops over every strip each frame, though short ones continue (:105). It's cheap, but "does no work on a scroll" in the body overstates it.
Checked and holding:
- Alignment.
frameWis an integer (thumbnailUtils.ts:75), sofirst * frameWmatches the full-strip positions exactly. The index-to-frame mapping is unchanged (VideoThumbnail.tsx:133). The last partial tile is clipped as before. - Same-frame updates. Zoom, a trim and a strip crossing the 4,096 px threshold all go through RO →
measure, which runs before paint. flushSync. It's called only from rAF and from RO/IO callbacks, never during render or in an effect. The ref attach uses a plainsetState. Commits happen after the loop, so a strip unmounted mid-commit can't be measured.- StrictMode. In a scratch test, the double-mount tears down and rebuilds the shared observers and listener with adds and removes balanced. The last unmount cancels the pending rAF, and running that stale frame afterwards doesn't crash.
Mutations: I ran 14 against the 3 targeted files (29 tests). 9 were caught: chunk floor/ceil (both ends), the start margin, the end clamp, threshold ×2, shift sign, the gap callback as a no-op, never evicting, and readAt not updated. 5 survived: the vertical shift, padding +1 tile, first ceil (benign, since the 512 px margin absorbs it), NEAR_PX = 0, and <=→<. The tree was restored and verified clean after each one.
CI at head: 93 pass, 1 skipping, 0 pending, 0 failed. I read the Before/After in the body as text; I didn't open the attached screenshots.
What I ran: I read the full diff. Baseline vitest on useThumbnailStripSize.test.tsx, VideoThumbnail.test.tsx and CompositionsTab.thumbnails.test.tsx gave 29/29 passing. I also ran the two scratch tests above (the repro and StrictMode, not committed) and the mutation pass.
— Somu
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes. The medium finding in the comment review at this head is real. I reproduced it, and I'm treating it as the blocker. Credit to that review.
Blocker: a long strip that's still on screen can go blank and stay blank.
- Each strip keeps its last measured position.
refresh()(useThumbnailStripSize.ts:109-116) shifts that saved position by the scroll since it was measured, and only re-reads the strip if the shifted position lands near the screen. Otherwise it appliesNOTHING_IN_VIEW. - A move that isn't a scroll changes the strip's real position without updating the saved one: rows removed above it, a dock divider, a window resize. None of these fire the ResizeObserver, since the size doesn't change. The presence observer doesn't fire either, since the strip stays in its band.
- On the next scroll, the shifted saved position can be off screen while the real strip is on screen. The strip then drops all its tiles.
- Recovery goes through the same
refresh(): the end gap now covers the whole strip and fires its observer, which callsscheduleRefresh. The saved position hasn't been re-read, andshowingis now false, so nothing changes. The strip stays blank until a later scroll happens to bring the stale position back near the screen.
Repro (in the PR's own "on a scroll" harness in useThumbnailStripSize.test.tsx):
- Mount one 1,000,000 px strip at
left = 0. Its span is"0-1536". - Move it to
left = -3000with no event. - Scroll the scroller by 2000 the other way, so the strip is really at
-1000, still covering the screen. - Run the frame: the span becomes
"0-0". - Run a second refresh frame, which is the path the gap sentinel takes: it stays
"0-0".
Fix: in refresh(), re-read every strip that is currently showing, rather than trusting its shifted box. Only strips near the screen are showing, so this adds a read for on-screen strips only and doesn't bring back the 1,000-row read storm the gate caught. Please add the repro above as a test.
Simplicity. Most of the code's complexity is the cached box with readAt arithmetic, and that's where this bug came from. A smaller design would keep a set of near strips, updated by the presence IntersectionObserver (it already has the 256 px band and scrollMargin). A scroll frame would then read only the strips in that set. That handles moves that aren't scrolls for free, but a long jump would get its pictures one frame later. If keeping same-frame pictures on a jump matters, the fix above is the smaller change. Your call.
Reuse: the PR extends useThumbnailStripSize, the hook all three strips already measure through, and shares a single ThumbnailTiles across them. That's the right place for it.
Non-blocking, from the same comment review (I haven't checked these myself): there's no test for the vertical-scroll shift, the left-padding value, the 256 px band, or the 4,096 px boundary.
This review covers that blocker. I'll do a full pass on the head with the fix.
— Rames
…croll A strip showing tiles is re-read on every scroll frame instead of trusting its last box shifted by the scroll, which a move without a scroll (rows removed above it, a dock resize) leaves stale. Tests now pin the vertical shift, the 256 px band, the 4,096 px short-clip limit and the tile padding.
|
Fixed at 32d5ffd. Thanks to both of you for the repro. Blocker / Medium 1 (blank strip after a move that isn't a scroll, then a scroll). Low 2 (survived mutations). Each now has a test that fails under its mutation:
Nit 3 (short strips still do some work). Agreed. The body now says a short strip is never measured or re-rendered on a scroll, but still shares the one scroll listener and is skipped in each frame's loop. Simplicity (Rames). I kept the same-frame design, since Desktop asked that a jump not show a blank frame. With the fix above, it takes one more read per strip leaving the screen in a frame. Known, unchanged. A far strip brought on screen by a move that isn't a scroll (a track collapsing, a dock resize) still gets its tiles one frame late, through the presence observer. That is listed under "What I did NOT exercise". |
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 32d5ffd6. The blocker from my last review is fixed, and this is the full pass I owed.
Blocker: fixed. refresh() now re-reads every strip that is showing tiles instead of trusting its shifted box (useThumbnailStripSize.ts:115), and the branch that cleared a strip without reading it is gone. The repro is now the test "keeps a strip on screen measured after a move without a scroll, then a scroll". With the old refresh() restored it fails, and at this head it passes. The extra reads only hit strips with tiles mounted, and isNear limits those to strips on screen. The viewport gate passes both arms at this head.
A strip that isn't showing. I checked the claim that the presence observer always repairs it. The observer's band is 256 px left and right with no vertical margin, and scrollMargin only applies inside the scroller. That band sits inside the 256 px band isNear checks on every side. So whenever the observer reports a strip, the read that follows returns a non-empty span and sets showing. A strip can't be stuck "intersecting but blank". The one-frame-late case (a far strip brought on screen by a move that isn't a scroll) is real and listed under "What I did NOT exercise". That's fine.
Rest of the PR.
ThumbnailTiles:first * frameWis the left padding, and the start gap is the same width. The end gap covers the whole strip when the span is0-0, so a blank strip still has something to observe. The index-to-URL mapping inVideoThumbnailis unchanged.watchGappasses one stable function to two refs, and each mount acquires and releases the shared observers in balance (React 19 ref cleanup).- Short strips (
<= 4096) are skipped before any work inrefresh(), andmergepins their span to the full width. The sidebar cards keep every tile, as before.
Reuse: this extends useThumbnailStripSize, the hook all three strips already measure through, and all three share one ThumbnailTiles. The only other IntersectionObserver in studio is the sidebar's lazy card loader, and the body explains why useTimelineScrollViewport doesn't fit (its snapshot isn't published while scrolling when row virtualization is off). Nothing to reuse that's missing.
Simplicity: keeping the same-frame design was your call, and this fix is the small version of it: one condition instead of two branches. The cached box with readAt is still the hardest part to follow, but each piece now has a test that fails without it.
Tests and mutations. The 4 thumbnail test files pass (33/33). I ran 11 mutations, and 10 were caught:
- reverting the fix;
- dropping
showingfrom the read condition; - always reading every strip;
- forcing
showingto true, and to false; - ignoring the vertical shift;
NEAR_PX = 0;<at 4,096 px;- making the gap observer a no-op;
- padding off by one tile.
Only one survived: the end gap starting one tile late. That's harmless while a tile is narrower than the 256 px band. The four survivors from the earlier review are all caught now.
CI: 57 pass and nothing failed when I checked. The viewport gate passes; edit accuracy, Windows and regression shards were still running.
— Rames
What
A clip's picture strip on the timeline now mounts only the tiles near the screen instead of one tile per frame width across the whole clip. This covers video, image and composition clips.
Why
At full zoom (48 px per frame), a 10-minute clip is 864,000 px wide. Its strip mounted 18,000 tiles, about 36,000 elements, for that one clip.
MAX_VISIBLE_THUMBNAIL_FRAMEScaps how many frames get decoded, not how many tiles are mounted. The cost lands on every zoom step and on every scroll frame that has to lay out and paint the timeline.Related work
Refs the timeline virtualization work (
useTimelineRowVirtualization,TIMELINE_VIEWPORT_BUDGETS). Rows are virtualized there, but a single clip's tiles were not.How
useThumbnailStripSizeis the hook all three strips already measure through. For a long strip it also reports the strip's on-screen span against the window, rounded outward to 512 px chunks, and clamped to the strip, so a strip re-renders only when it crosses a chunk. A strip more than 256 px off screen in either direction gets one empty span, the same on either side.ThumbnailTilesmounts the tiles inside that span and pads the skipped width on the left, so every tile sits exactly where it did before. All three strips use it.flushSynccommit. A jump to anywhere in a long timeline therefore measures exactly the strips it brings on screen, in that frame, at the cost of a short scroll; far strips cost arithmetic only.rootMarginplusscrollMargin, so the band also applies inside the timeline's own scroller.useTimelineScrollViewport).Test plan
What I measured
Unit tests (
VideoThumbnail.test.tsx). Each new test fails with its piece of the fix removed:expected 12170 to be less than 60; without the scroll refresh it fails withexpected 431041 to be less than or equal to 100000.expected 431041 to be less than or equal to 430500.useThumbnailStripSize.test.tsx, each fails on the head before its fix:ThumbnailTiles.test.tsxthe first mounted tile sits exactly where the padding ends.Suites and checks. The Studio player, hooks and components suites pass (5,809 tests). oxlint,
tsc --noEmitand the comment ratchet are clean.Timeline viewport gate. The first version made the gate's 1,000-row arm (row virtualization off) take about 3.3 s per interaction. A profile of that arm showed every scroll frame reading the position of all 1,000 strips (14,331 forced style-and-layout passes against 134 on main). CI's gate at 32d5ffd passes both arms:
Browser run, long clip (measured at 7567661, before the gate commits; those change when strips re-measure, not what a long clip mounts). Studio dev server and headless Chrome, 1440 x 900, on a project built from the repo's
studio-openfixture plus a generated 10-minute test-pattern video. The script zooms from fit to full (14 presses), then scrolls the timeline. Runs on main and on that head alternated, 3 each, on a shared Linux box under load (10 to 15 on 8 cores). Medians:What is on screen. Screenshots taken 1 minute into the clip are byte-identical before and after.
Drag. In the browser, I dragged the long clip 1,000 px at full zoom. With this head, its picture covers all 1,432 visible pixels at every step. An independent review of this PR's first version found its drag copy ran out of picture: 1,192 of 1,432 px covered, and the gap stayed. That led to the edge watch above.
What I did NOT exercise
DisplayItemList::Raster). So I report main-thread work, not wall-clock frame time.scrollMargin. They catch an unmounted end only once it is on screen, so one frame there can show a blank strip. Chrome and Electron support it.Before
The timeline at full zoom, a minute into the 10-minute clip. The badge shows counts read from the page at capture time: 18,000 tiles mounted for this one clip.
After
The same view with 64 tiles mounted. Below that: a dragged long clip that keeps its picture, and the numbers.