fix(studio): capture the storyboard frame hero at full resolution - #3338
Conversation
The thumbnail route bounds every preview capture to 240x135. That bound came from the timeline, where thumbnails are small and numerous and their decoded bytes are budgeted. The storyboard reuses the same route for its frame detail hero, which is up to 900px wide, so the poster arrived at 240x135 and upscaled past 7x on a retina display. Headlines survived it; body copy, table labels and captions did not. That is the surface where it costs the most. references/review-loop.md sends the user here to confirm layout and real copy, and tells them to run no CLI in that pass: "the poster is the only picture this pass needs". Give the caller a way to ask for the composition's own dimensions, which the route already supports as `output=source`, and fold the choice into a single `surface` prop. Whether a poster is a tile or the hero decides both the crop and the capture density, so one prop owns both rather than two that can disagree. The contact sheet keeps the bounded capture: many tiles, and it is a contact sheet. The timeline is untouched. Reported with a reproduction and a correct read of the consequences in #3271. Co-Authored-By: anikam13 <22992075+anikam13@users.noreply.github.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Approve — tight fix, correct scope. surface replacing fit is a better API: one prop decides both crop and capture density so they can't disagree. Default "tile" preserves the bounded path for every existing caller (verified StoryboardFrameTile.tsx uses the default), "hero" adds output=source only for the one-at-a-time detail view. Tests pin all four behavioral edges. No issues.
— Miga
Stamp removed — was posted prematurely
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 22eb64aa. Miga's stamp was self-dismissed at 20:28Z as "posted prematurely", so this is an independent pass rather than a second opinion on it.
Strengths, specifically:
surfacereplacingfitis the right shape andFramePoster.tsx:11-19says why rather than leaving it implicit: the crop and the capture density are both consequences of where the poster is rendered, so folding them into one prop makes the disagreeing state unrepresentable.- The correction to the issue's diagnosis is worth more than the fix itself.
posterMaxPhysicalWidth: 240(timelineViewportBudgets.ts:66) has exactly three uses — a decoded-byte cache weight atCompositionThumbnail.tsx:77-79andImageThumbnail.tsx:42-45, and the decoder bound atthumbnailVideoDecoder.ts:68-70. None of them sizes a thumbnail-route capture. Two different 240/135 pairs in two files, and #3271 named the wrong one, so changing it would have perturbed the timeline's cache budget and left the storyboard exactly as blurry. - The cache key is what actually makes "the tile is untouched" true, and it already handled this without a change:
thumbnail.ts:166keys onoutputModeandoutputWidth x outputHeight, so a hero and a tile of the same frame at the same seek time cannot serve each other's bytes.
Claims I checked rather than took:
| claim | result |
|---|---|
the route already accepts output=source, so this is client-side only |
verified at the base — thumbnail.ts:97-102, a file this PR does not touch |
| the bound is not raised | verified — output=source takes outputScale = 1 at :161-163; the 240/135 pair at :21-22 still governs every other caller |
every FramePoster call site is accounted for |
verified — two render sites, StoryboardFrameFocus.tsx:222 (now surface="hero") and StoryboardFrameTile.tsx:61 (default), plus a vi.mock at StoryboardViewModeGuard.test.tsx:23. No fit= left at head, and green Typecheck is independent confirmation, since an excess JSX prop is a type error |
| the tile's behaviour is unchanged | verified — the old default fit="cover" and the new default surface="tile" take the same object-cover branch and add no query param |
the new output param is additive for existing callers |
verified — three callers of buildCompositionThumbnailUrl (CompositionsTab.tsx:173, CompositionThumbnail.tsx:103, FramePoster.tsx:55); only the last passes output, and the write is guarded by if (output) |
Closes #3271 is the right keyword |
verified — the issue's own Proposed solution reads "A larger poster in the detail view, leaving the grid as is", so the reporter's case is fully resolved and the grid was theirs to scope out |
| CI | 8 of 8 required contexts present and green at this head; BLOCKED was the reviewer gate alone |
The tests pin both directions of both behaviours — output present and absent, object-contain and object-cover — which is what makes the "fails on the parent commit" claim check out. With no surface prop on the parent, the two cases asserting hero behaviour fail (no output param, and object-cover where object-contain is expected) while the two tile/default cases pass vacuously.
Two notes, neither blocking:
output=sourcehas no upper clamp —outputScaleis1whatever the composition's dimensions, so a 4K frame's hero captures at 3840x2160. Not introduced here:thumbnail.ts:100-101already routes anyformat=pngrequest to source mode, so unclamped source captures predate this PR. One image at a time, and the disk cache is bounded at:23-24(512 MB / 14 days) with the prune at:168-174.- Worth knowing before anyone consolidates the two poster paths: the hero is a plain
<img>, so it never enters the timeline's weighted LRU — and that LRU'sweightis the hardcodedposterMaxPhysicalWidth * posterMaxPhysicalHeight * 4, not the real decoded size. Moving a source-density poster ontoCompositionThumbnailwould under-count it by exactly 64x (1920/240 x 1080/135). Today's separation is what keeps that from mattering, so it is worth a comment if these ever converge.
Verdict: APPROVE
Reasoning: Small, correctly scoped, and every load-bearing claim in the body holds against the source — including the two that could only be settled at the base ref rather than from the diff. The single behaviour change is opt-in per surface, both surfaces are pinned in both directions, and the required matrix is green at this head.
— Rames Jusso
Closes #3271.
What
The storyboard's frame detail hero now captures at the composition's own dimensions. The contact-sheet tile and the timeline are unchanged.
Why
The thumbnail route bounds every preview capture to 240x135 (
THUMBNAIL_MAX_OUTPUT_WIDTH/_HEIGHT, fed intothumbnailDeviceScaleFactor). That bound arrived with #2720, a timeline change: timeline thumbnails are small and numerous and their decoded bytes are budgeted.The storyboard reuses the same route for its frame detail hero, which is up to 900px wide. Measured in a running Studio on a 1920x1080 composition:
Headlines survive that. Body copy, table labels and captions do not.
That is the surface where it costs the most.
references/review-loop.mdsends the user to the poster to confirm layout and real copy, and tells them to run no CLI in that pass: "the poster is the only picture this pass needs". So there is no sharp image available at the moment the workflow asks for a judgement.One correction to #3271, which points at
posterMaxPhysicalWidth: 240intimelineViewportBudgets.ts. That constant is not what sizes the poster: its three uses are a decoded-byteweightfor the timeline thumbnail cache and the video-decoder bounds. Same two numbers, different file. Changing it would perturb the timeline cache budget and leave the storyboard exactly as blurry.How
buildCompositionThumbnailUrlgains an optionaloutputparam. The route already acceptsoutput=source, so this is client-side only.FramePoster'sfitprop becomessurface: "tile" | "hero". Whether a poster is a tile or the hero decides both the crop and the capture density, so one prop owns both rather than two that can disagree.The bound is not raised, so nothing the perf work covered regresses:
Not covered: the contact-sheet tile still upscales 2.62x on a 2x display. #3271 explicitly scopes the fix to the detail view and leaves the grid as is, so this PR does too.
Test plan
New
FramePoster.test.tsxpins the wiring: the hero asks for source density and letterboxes, the tile does not and fills its cell, and the default is the tile. Verified the two behavioural cases fail on the parent commit and pass here.CompositionThumbnail.test.tscovers the URL param in both directions.Full suites green:
packages/studio(1062 tests) andpackages/studio-server(446 tests).Manual, in a running Studio against a frame carrying a headline, a small table and a 14px paragraph:
Before: hero asset 240x135, 7.48x upscale, table and paragraph unreadable
After: hero asset 1920x1080, 0.94x, both legible
Tile in the same run before and after: 240x135, unchanged
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)