Skip to content

perf(studio): long clips mount only the picture tiles in view - #4993

Merged
miguel-heygen merged 16 commits into
mainfrom
feat/studio-visible-tiles
Oct 4, 2026
Merged

miguel-heygen merged 16 commits into
mainfrom
feat/studio-visible-tiles

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

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_FRAMES caps 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

  • Only long clips. A strip up to 4,096 px wide (about two screens) keeps every tile, as before, and is never measured or re-rendered on a scroll; it still shares the one scroll listener and is skipped in each frame's loop. Longer strips mount only the tiles near the screen.
  • The span. useThumbnailStripSize is 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.
  • One shared tile row. The new ThumbnailTiles mounts 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.
  • Refreshing on a scroll, in the same frame. All strips share one scroll listener, one ResizeObserver, two IntersectionObservers and one animation frame. Each long strip remembers its last measured box. On a scroll frame, the hook reads each scroller's offset once and shifts every remembered box by the change. It measures the strips whose shifted box lands within 256 px of the screen, and every strip already showing tiles, since a move without a scroll (rows removed above it, a dock resize) leaves its box stale. Every read happens before one flushSync commit. 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.
  • Moves without a scroll. Two invisible elements cover each strip's unmounted ends. An IntersectionObserver re-measures when either nears the screen, whatever moved the strip: a drag (the drag copy moves through a wrapper), undo, a typed start time, a dock divider, a window resize. A second IntersectionObserver measures a strip as soon as it comes within 256 px of the screen. Both use rootMargin plus scrollMargin, so the band also applies inside the timeline's own scroller.
  • Why the window. I measure against the window rather than the timeline's scroll snapshot because that snapshot is not published while scrolling when row virtualization is off (useTimelineScrollViewport).

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)
  • Comments follow CONTRIBUTING.md "Comments"

What I measured

Unit tests (VideoThumbnail.test.tsx). Each new test fails with its piece of the fix removed:

  • A 10-minute clip at full zoom mounts fewer than 60 tiles, and they cover the window before and after each scroll. On main this fails with expected 12170 to be less than 60; without the scroll refresh it fails with expected 431041 to be less than or equal to 100000.
  • A strip moved by something other than a scroll, as a drag moves its copy, still covers the window. Without the edge watch it fails with expected 431041 to be less than or equal to 430500.
  • A clip wholly off screen mounts no tile.
  • In useThumbnailStripSize.test.tsx, each fails on the head before its fix:
    • near strips are re-measured in one shared frame;
    • a jump measures the strips it brings on screen in that same frame;
    • a strip measured between a scroll and that scroll's frame is not shifted by the scroll twice;
    • strips carried from one side of the screen to the other do not re-render;
    • a strip wholly on screen does not re-render when it moves;
    • a strip on screen keeps its tiles after a move without a scroll followed by a scroll (from review);
    • a vertical scroll measures the strip it brings on screen in that frame;
    • a strip ending just off screen keeps its edge tiles (the 256 px band);
    • a short clip keeps every tile up to exactly 4,096 px, and is never re-measured on a scroll;
    • a strip far above or below the screen mounts nothing;
    • a strip is measured as it comes near, without waiting for a frame;
    • a scroll reads no strip far from the screen.
  • Removing the left padding fails both tests, and in ThumbnailTiles.test.tsx the first mounted tile sits exactly where the padding ends.
  • The changed tests pass 3 runs in a row.

Suites and checks. The Studio player, hooks and components suites pass (5,809 tests). oxlint, tsc --noEmit and 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:

  • 1,000 rows: interaction p95 48.5 ms (limit 75), frame p95 33.3 ms (limit 75), with 20,929 timeline elements on the first run against main's 20,900. Main's recent runs of this arm are 43 to 61 ms and 17 to 33 ms; the p95 moves between runs with frame timing (the previous head measured 33.0 ms), so the element count is the steadier comparison.
  • virtualized: interaction p95 33.2 ms (limit 58.3), frame p95 16.8 ms (limit 25), with 1,230 timeline elements, as on main.

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-open fixture 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:

  • Elements on the page: 37,154 to 1,030.
  • Tiles in the clip: 18,000 to 43.
  • Zoom to full: 7.1 s to 1.9 s, and the slowest single press from 1.8 s to 0.5 s.
  • Main-thread work over 20 scroll frames: 1,973 ms to 843 ms, with layout going from 105 ms to 27 ms and paint from 200 ms to 42 ms.

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

  • Scroll frame time on a machine with a GPU. The box I measured on has no GPU. Each scroll frame costs about 500 ms there on both sides, almost all of it Chrome's software rasterizer (the trace shows the main thread waiting on DisplayItemList::Raster). So I report main-thread work, not wall-clock frame time.
  • A strip moved by a layout change, not a scroll. Zoom, a row insert or a dock resize moves strips without a scroll. The observers repair a strip as it nears the screen, after the frame that showed it, so such a move can show one frame without pictures on a long clip.
  • The long-clip numbers on the final code. They were measured at 7567661; the later commits change only when strips re-measure.
  • Browsers without 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.
  • A clip growing while it scrolls. If a trim or zoom widens a clip and a scroll frame reads it before the resize report, one layout pass can hold its old last tile off screen. The review found the resize report corrects it before paint in that frame; I did not watch it in a browser.
  • The audio waveform canvas. It is untouched; it draws one canvas the width of the clip.
  • Desktop. It picks this up with its next Studio bump.

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.

Before: 18,000 tiles mounted

After

The same view with 64 tiles mounted. Below that: a dragged long clip that keeps its picture, and the numbers.

After: 64 tiles mounted, same view

After: the dragged clip keeps its picture

Before and after numbers on the final code

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.
@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 1562 (base branch 1562), smooth 1235 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 4, 2026 10:05
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 somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and showing is 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) * frameW survives (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 at index * frameW would 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. frameW is an integer (thumbnailUtils.ts:75), so first * frameW matches 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 plain setState. 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 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 applies NOTHING_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 calls scheduleRefresh. The saved position hasn't been re-read, and showing is 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):

  1. Mount one 1,000,000 px strip at left = 0. Its span is "0-1536".
  2. Move it to left = -3000 with no event.
  3. Scroll the scroller by 2000 the other way, so the strip is really at -1000, still covering the screen.
  4. Run the frame: the span becomes "0-0".
  5. 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.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

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). refresh() now reads every strip that is showing tiles, plus any strip whose shifted box lands near the screen (useThumbnailStripSize.ts:115). The "far, so clear it" branch is gone: a real read of a far box already returns 0-0. Rames's repro is now the test "keeps a strip on screen measured after a move without a scroll, then a scroll", and it fails with the old refresh() (['0-0'] instead of ['0-2560']). A strip that isn't showing tiles is repaired by the presence observer when it comes on screen. Its band (256 px left and right, none vertically) lies inside the 256 px band on every side that a read uses, so a strip read as far always produces an entry when it arrives. Only long strips that are showing get the extra read; short strips are skipped first. On CI's gate at this head, the 1,000-row arm measures 48.5/75 ms with 20,929 timeline elements (main: 20,900), and the virtualized arm 33.2/58.3 ms with 1,230 (as on main).

Low 2 (survived mutations). Each now has a test that fails under its mutation:

  • vertical scroll shift ignored → "measures a strip a vertical scroll brings on screen in that same frame";
  • NEAR_PX = 0 → "keeps the tiles of a strip that ends just off screen";
  • < instead of <= at 4,096 px → the short-clip test now uses a strip of exactly 4,096 px;
  • paddingLeft: (first + 1) * frameW → new ThumbnailTiles.test.tsx: the first mounted tile's index × frameW equals the padding.

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 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 * frameW is the left padding, and the start gap is the same width. The end gap covers the whole strip when the span is 0-0, so a blank strip still has something to observe. The index-to-URL mapping in VideoThumbnail is unchanged.
  • watchGap passes 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 in refresh(), and merge pins 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 showing from the read condition;
  • always reading every strip;
  • forcing showing to 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

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit 1bdb5ed Oct 4, 2026
169 checks passed
@miguel-heygen
miguel-heygen deleted the feat/studio-visible-tiles branch October 4, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants