Repository navigation
fix(core): stop small previews dropping layers on heavy compositions - #5268
Conversation
Edit accuracy: accurate 2061 (base branch 2061), smooth 1506 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
GSAP infers a -50% CSS translate from the rendered pixel matrix. At a fractional preview zoom that matrix is snapped, so a centered layer lost its percentage and shifted by half its height after reload, render and redo. The runtime now restores the authored percentage before GSAP's first transform parse. Also restores the original thumbnail ceilings.
resize-scale-px-r30-{root,nested}-z50-after measured 0.53 px tracking on
main and 0.49 px here. Also drops a comment line the runtime name covers.
The centering repair now also reads percentages from a stylesheet transform (the common translate(-50%, -50%) pattern), runs for every property that makes GSAP parse a transform (transformOrigin first, too), resolves against the border box, and leaves elements without a layout box alone instead of throwing. The browser check now runs zoomed.
GSAP folds every other CSS percentage translate into pixels, and compositions rely on that (a translateX(-100%) title tweened to x: 0 slides in). The repair now gives back only a -50% that zoom snapping lost, and treats a size that is not in px (a hidden element sized in percent) as no box.
A pixel part in the translate means GSAP never infers -50%, so the repair leaves it alone, and a layer without a layout box is left to GSAP whatever size its computed style reports (that differs by Chrome build for hidden percent-sized layers).
The zoom was applied on the first resize callback, after the source was assigned, so a fast-loading composition could run its scripts and lay out unzoomed and then reflow. Fitting on connect, before any source, makes the first layout use the preview zoom.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Request changes at b44a30e3. The fix does make layers raster at the size they're shown. But the GSAP adapter changes the final render, which the body says it doesn't. And fitting with zoom makes the preview wrap text differently from the export.
Blocker 1: the GSAP adapter changes renders at native scale (core/src/runtime/init.ts:346).
installFlatGsapTransformsruns fromruntime/entry.ts:44. That is the runtime the producer injects into every render page, and the producer's early GSAP stub can be chained, so the adapter installs during renders.- The body says "At native scale GSAP already infers centering, so nothing changes." GSAP's own rule (CSSPlugin.js ~1003) compares
Math.round(offsetWidth/2)withMath.round(-x), so it doesn't infer centering for widths between about 2k+0.5 and 2k+1 px. The adapter forces-50%there. - Repro: Chrome, DPR 1, no zoom, producer early stub + runtime + GSAP 3.15.
- Setup:
left:50%; transform:translate(-50%,-50%); width:226.6px, thengsap.from(el,{x:-400}). - Without the adapter, frame 0 is at
left=560. With it, frame 0 is atleft=446.7, a jump of half the element's width. - Widths of 227px and 226.4px don't change.
- Removing
init.ts:346and rebuilding the runtime brings back 560.
- Setup:
- No producer golden covers this, so the regression shards stay green.
- Fix: apply GSAP's native rule with the unzoomed size, so the render is unchanged. Or keep the change on purpose and own it with a body note, a release note and a producer fixture.
Blocker 2: preview text no longer wraps like the render.
- I laid out 120 paragraphs three ways:
- the old transform fit;
- the new zoom fit;
- a native 1920px page at DPR 1, matching render conditions.
- Transform fit vs native: 0/120 differ in line count.
- Zoom fit vs native:
- 1–2/120 wrap to a different number of lines. For example, a 23.1px sans paragraph in a 295px box is 6 lines in the render and 5 in the preview.
- 106–113/120 differ in height, by up to a full line.
- It held at DPR 1 and 2, for 240, 528 and 1100px players, with font hinting on and off.
- Zoom shapes text at the zoomed font size, so this follows from the approach itself. The GSAP adapter patches one symptom of the changed layout, and text wrapping is another.
Should-fix
- Resizing re-lays-out the whole composition.
-
The ResizeObserver (
hyperframes-player.ts:222) calls_rescale, which writesiframe.style.zoom(iframe-dom.ts:76) on every tick with no debounce. -
40 resize steps on a 3000-node composition:
Fit Layout Style recalc Transform 1.9 ms 2.6 ms Zoom 794 ms 418 ms -
That's about 30 ms per step, so dragging a panel janks on a heavy composition.
-
Option: keep the transform during the drag and apply the zoom once resizing settles.
-
- Canvases sized from
devicePixelRatiostay at fit resolution when the editor zooms in.- The editor zooms the stage with a transform (up to 400%,
NLEPreview.tsx:246), which doesn't change the frame's DPR. - DOM text re-rasters crisp, but VFX canvases size from DPR with no floor (
core/src/runtime/vfx.ts:709) and go soft. - At 400% on a 2x screen, the share of blurry edge pixels was 0.064 with zoom against 0.017 before.
- The existing
set-display-scalemessage could size those canvases.
- The editor zooms the stage with a transform (up to 400%,
- One hidden player can switch the fix off for the whole page.
zoomsFramesWhole(iframe-dom.ts:84-101) is decided once, by the first player that connects.- If that player sits in a
display:nonecontainer, the probe records "no zoom". The next, visible player then getstransform: … scale(0.275)with no zoom. - Fix: treat a 0×0 frame as unknown and probe again later.
Checked and fine
- Player code is preview-only. The producer and engine don't import the player, and the CLI
play/presentcommands are previews. - Geometry is right at high DPR, for small and large players:
- the frame's DPR equals host DPR × zoom;
- the inner viewport stays 1920×1080;
- the iframe sits at its expected rect, so picking math is unaffected (edit-accuracy 20/20 green).
- Body claims that check out:
frameDisplayScaledivides out the zoom (protocol.ts:76); a mutant removing it is caught.- The probe reuses the initial frame: one frame, one navigation.
- Renders don't use the player.
- Tests: core 1920/1920, player 540/540,
teleport.test21/21, andtest:browser-rasterpasses on Chrome.
Nits
baseline.jsonis fully re-recorded, not two cases. All 2061 cases moved, each by at most 0.13px against a 0.5px limit. That still shifts what the gate compares against, so the body should say so.- Mutants that survive unit tests:
- Caught only by the browser test: removing the adapter install, dropping
initializeIframeScalingfromconnectedCallback, and reverting the_connectedguard. - Survive everything:
- dropping the probe's DPR check (
iframe-dom.ts:100); - applying the
-50%rule per axis, since mixed centering liketranslate(-50%, -100%)is untested.
- dropping the probe's DPR check (
- Caught only by the browser test: removing the adapter install, dropping
- Simpler shape, which I haven't tested: keep the transform fit and cap oversized
will-changerasters only in the preview. Layout would stay identical to the render, with no GSAP patch.
CI: 77 pass and 8 skipped across 85 distinct checks, 0 failures.
— Rames
…ayout CSS zoom shaped text at the zoomed size, so paragraph heights in the preview differed from the render by up to a line. The transform fit stays; the centering adapter and zoom-only bench changes go with it.
Chromium never rasters a will-change: transform layer below device scale, so a preview shown at a quarter size rastered every hinted layer at full size and could run out of tile memory, dropping layers. While the player shows the composition below 1x, the runtime swaps the hint for an identity filter, which keeps its stacking context and containing block; preserve-3d containers keep the hint.
…n save The identity filter that replaced will-change: transform showed up in computed filter, so Studio's blur slider saved it, GSAP filter tweens and clearProps picked it up, and it cut backdrop-filter off from what is behind it. A far perspective keeps the same stacking context and containing block, is never composited for flat content, and nothing reads or writes it. Also: the runtime guard lint (realm-safe element checks), a class change that drops the hint undoes the swap, and the browser check measures raster scale with the swap off and on at one shown size and compares frosted glass and 3D pixels.
The perspective that stood in for will-change: transform could still reach a saved project: Studio records perspective keyframes from GSAP's computed value, and copying a generated element serialised its inline styles. It also went stale when a parent class or a later stylesheet changed the element. Now the runtime writes no inline style. It marks a hinted layer with a preview attribute and one injected rule sets its will-change to auto. Only layers whose hint changes nothing are marked: positioned, with no fixed descendants, and no z-indexed or blending descendants unless the layer has its own z-index. Every class or tree change re-checks the whole page. Studio strips the mark from copied markup, as it does the other preview marks.
…ine styles need A layer's ::before or ::after can sit behind it, blend with it or be fixed to it, so those count like children. A layer that also hints opacity or filter keeps every hint, since those make their own boundaries. Inline style changes, which scripts make every frame, are watched cheaply: they only give the hint back to the layers holding an element that now depends on it, or to a layer that turned static or changed its z-index. Text changes no longer trigger a full pass; a new stylesheet still does.
…e or late sheet A marked layer now has its mark lifted while its hints are read, so a class or inline value that adds opacity or filter gives every hint back. Any attribute change re-checks its subtree, so attribute selectors are followed, and a linked stylesheet re-runs the pass once it loads.
Lifting a mark to read a layer's own hints repaints it, and a change on a scene container lifted every marked layer under it (about 6 ms and a re-raster per event). Now only the changed element is lifted, and a full pass clears all marks at once so the page restyles a single time. Tests cover leaving marked layers alone, ignoring our own marks, and the linked-sheet load pass.
A full pass re-marked layers one at a time, so each read restyled the page again. Every mark is now decided first and written after. The browser check holds off the player's own scale messages while it turns the swap off and on, since a late resize tick could turn it back on.
…s the swap off Turning the swap off restyles the page, and the player answers by resending its real scale, which turned the swap back on within milliseconds, so the off half compared the swapped page with itself. The check now holds the player's sends on the host page, asserts the swap is off before the native shots, and removes the layers that keep their hint before either trace.
Stripes a few pixels wide alias differently at each raster scale, so the probe measured texture resampling rather than the 3D shape it is there for. A solid fill keeps the shape and position.
miguel-heygen
changed the title
|
Thanks, Rames. Reworked at Blocker 1 (GSAP adapter changes renders). The adapter is deleted; nothing in this PR touches GSAP. The new code runs only after a Blocker 2 (zoom changes text wrapping). Zoom is gone; the preview fits with Should-fix: resizing re-lays-out the composition. Gone with zoom. Resizing only changes the transform. The swap runs once when the shown scale first drops below 1, and later messages below 1 return early. Coming back to 1x restores once. Should-fix: canvases stay at fit resolution when the editor zooms in. That came from zoom changing the frame's device pixel ratio. With the transform fit the frame's DPR is the host's again, as on main, so this PR no longer changes canvas sizing. Should-fix: one hidden player switches the fix off. The zoom probe is deleted. Each document swaps from its own Nits. How it keeps layout: the runtime writes no inline style. Below 1x it adds one rule ( Numbers on the 48-layer fixture (528px, DPR 2, 80 MB tiles, full Chrome 154, 3 paired runs vs the merge base): largest raster over shown size 3.64 to 1.00, |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Request changes at aca81c35, narrowly. The rework fixes every finding from my last review. Renders are byte-identical to main, and layout doesn't move. Two small Studio-side defects remain, each a few lines plus a test. I'll approve a head that fixes both.
My earlier findings
Finding at b44a30e3 |
Now |
|---|---|
GSAP adapter changed renders (init.ts:346) |
Resolved. init.ts differs from main only by the import and the onSetDisplayScale wrapper. A 2614-node composition with GSAP-animated will-change layers rendered to PNG through the CLI gives 20/20 frames byte-identical, head against merge-base. |
| Zoom changed text wrap | Resolved. Zoom is gone. In Chrome 153, toggling the swap moved 0 of 2614 boxes and changed the line count of 0 of 120 paragraphs. The browser test asserts boxes within 0.01 px. |
| 794 ms resize layout | No longer applies. A resize only changes the transform. The swap runs when the shown scale crosses 1x: 9.6 ms the first time, then about 7–8 ms to re-mark and 2 ms to restore. |
| VFX soft under editor zoom | No longer applies: no zoom, so the frame's DPR is the host's again. |
| One hidden player switched the fix off | No longer applies: the probe is gone. Each document swaps on its own set-display-scale. |
Preview-only, as claimed: setPreviewRasterScale is reachable only through the bridge's set-display-scale message. Only the player (hyperframes-player.ts:1013) and Studio (runtimeProtocol.ts:40) send it. The producer, engine and CLI render never do, and the module has no load-time side effects. A mark that ends up in a saved file does nothing in a render, because the CSS rule that reads it is injected only in a preview.
Should-fix 1: Studio's stacking model reads the hint the swap drops (core/src/runtime/stackingContext.ts:59-65)
- The cause:
stackingContext.tsdecides stacking contexts from computedwill-change, and a marked layer now readsauto. That feedsstackingContextIdinruntime/timeline.ts:483/594/651andstudio/.../timelineSyncHydration.ts:106, andcomputeStackingPatches(timelineStackingSync.ts:178-180) compares only clips with the same id. - Scenario: clip A sits inside
<div style="position:absolute; will-change:transform">and clip C sits at the root. Once the preview is small, A's id changes fromcss:1.0.0tocss:root. Dragging A's row above C then returns[{a, zIndex: 1}]instead of[]. - The effect: Studio writes a
z-indexinto the source that does nothing in the export, because A is still inside the wrapper's stacking context. The timeline order no longer matches the render. - Fix: treat
[data-hf-preview-raster]as still hinted instackingContext.ts, or read the authored hint.
Should-fix 2: a text edit saves the mark into the source (domEditingLayers.ts:72-77, :144-160)
buildTextFieldcopies every live attribute exceptstyle, and the serializer writes them out.- Repro: editing "Hello" in
<h1>Hello <span class="word" data-hf-preview-raster>world</span></h1>produces the patchHi<span class="word" data-hf-preview-raster="" data-hf-text-key="child:1:span">world</span>. data-hf-upcomingalready has the same gap. But positioned per-wordwill-changespans are exactly what this PR marks, so this path will be hit far more often.- The body's "Studio strips the mark from copied markup" holds only for copy and ask-agent.
- Fix: keep one shared list of preview marks, used by both
liveMarkupWithoutPreviewMarksandbuildTextField.
Nits
- Nothing pins "no message, page untouched". Turning the swap on at module load with no message still passes the browser test, and fails one unit test only through a leftover timer. A unit test asserting that the runtime injects no style element and no marks without a
set-display-scalewould pin the render guarantee. - Per-frame cost is a bit above the body's 1–2 ms. With 450 GSAP-animated words in 30 marked lines (Chrome 153), script time per frame is 3.90 ms with the swap against 1.11 ms without, and adding a stylesheet costs 15.3 ms against 0.6 ms. Toggling a class on a container is cheaper with the swap (5.3 against 11.5 ms). None of this blocks.
- "Pixels unchanged" is slightly strong. Paint order is identical, and static text and overlays match. The GSAP-scaled words re-raster sharper, which the body does mention.
Tests:
- Core 4408 pass (runtime 1942/1942,
previewRasterHints10/10), Studio 7814 pass (authoredSource1/1), Player 553/553. test:browser-rasterpasses 3/3 on chrome-headless-shell 152.- Typecheck, oxlint and oxfmt are clean.
- Mutants:
- restore only above 1x: 6 unit tests fail;
- dropping the layer's own
::before/::aftercheck: the browser test fails; - removing the
authoredSourcestrip: the studio test fails; - dropping
unmarkHolders: 1 test fails; - turning the swap on with no message survives (nit above).
CI: regression on the latest run failed because shard 9 hit a Docker Hub 429 while pulling the node image, and the other shards were cancelled. That's infrastructure. The earlier run at this same head had regression and all 10 shards green. A rerun should clear it.
— Rames
…ext edits Studio's stacking model read a marked layer's will-change as auto, so a timeline drag could write a z-index that does nothing in the render; a marked element now still counts as a stacking context. A text edit copied every live attribute but style, so it could save the mark; the preview-only attributes are now one list, used by both the copy and the text-edit paths.
The runtime keeps a video across a scene swap only when its live attributes match the fresh import, and the preview raster mark made them differ, so a hinted video in a small preview was rebuilt on every edit. The comparison now ignores every preview-only attribute, which also fixes the same rebuild for data-hf-upcoming. The shared list is renamed PREVIEW_ONLY_ATTRS, and the lazy-image strip in copied markup gets a test.
|
Thanks, Rames. Both should-fixes and the nit are in at Should-fix 1 (stacking model reads the dropped hint). Should-fix 2 (text edit saves the mark). The preview-only attributes are now one list, Also from a review of that delta: the runtime's keep-unchanged-media check compared live attributes, so a marked video was rebuilt on every scene swap in a small preview. Nit (nothing pins "no message, page untouched"). Nit (per-frame cost). Thanks for the numbers; the body now cites the review's measurements rather than a single figure. "Pixels unchanged": the body says fine detail in a swapped layer rasters at the size shown and can look a little different, while layout, shapes and positions stay. CI at Checks at |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approve at 46e70977. Both should-fixes from my aca81c35 review are fixed, and each fix has a test that fails without it. The nit about the untouched page is pinned too.
Should-fix 1, stacking: fixed (stackingContext.ts:72)
- A marked element now counts as a stacking context.
- That's exact, not just safe. The runtime marks a layer only when every
will-changehint istransform,translate,rotateorscale(previewRasterHints.ts:15-18). All four are inSTACKING_CONTEXT_WILL_CHANGE_PROPERTIES, so the authored layer was always a stacking context. The id is the same path-basedcss:…either way, socomputeStackingPatchessees the same id with or without the mark. stackingContext.test.tscovers it. Deleting the line fails that test.
Should-fix 2, saved text edits: fixed (domEditingLayers.ts:59, :76)
buildTextFieldnow skipsstyleplus every entry of the new sharedPREVIEW_ONLY_ATTRS.liveMarkupWithoutPreviewMarksuses the same list, so the two can't drift.- The test round-trips
<span class="word" data-hf-preview-raster data-hf-upcoming>and checks the saved patch carries neither mark. Reverting the filter fails it. - The lazy mark's
loading="lazy"isn't stripped on this path. That can't happen here, becauseisEditableTextLeafonly takes text tags with no children, so an<img>never becomes a text field.
Also in this head
- Scene swaps (
init.ts:295):RUNTIME_MEDIA_ATTRSnow includes the preview marks. A marked video is no longer read as an authored change during a scene swap. Removing the spread fails 2 of the new swap tests. authoredSource.ts: the strip loop is rewritten around the shared list and behaves the same as before. The new lazy-image test fails ifloadingis no longer dropped.- Untouched page:
previewRasterHints.test.tsnow asserts no style element and no marks before anyset-display-scale. That covers my nit.
Tests
- Core runtime 1946/1946 (76 files), Studio editor + utils 2801 pass (16 todo).
- Mutants, 4 of 4 caught:
- dropping the stacking line;
- reverting the text-field filter;
- removing the marks from
RUNTIME_MEDIA_ATTRS; - keeping
loadingon a lazily previewed image.
- This approval waits for the required checks at this head.
— Rames
What
A small preview no longer rasters a composition's
will-change: transformlayers at full size. While the player shows a composition smaller than its own size, the runtime drops that hint on every layer where nothing depends on it, so those layers raster at the size they are shown. Heavy compositions stop running out of tile memory and dropping layers. Layout, paint order and pixels stay as they are in the export, and the runtime writes no inline style.Why
Heavy compositions dropped layers in a small preview: text, labels and overlay controls vanished for single frames while WebGL canvases stayed drawn.
Chromium never rasters a layer under a
will-change: transformhint below the native scale (device scale x page scale), unless the shown scale is under 25% of native (MinimumRasterContentsScaleForWillChangeTransformincc/layers/picture_layer_impl.cc). The player fits a 1920x1080 composition into its box withtransform: scale(), so in a 528px preview on a 2x screen every hinted layer rasters 3.6x larger than it is shown. A composition with many such layers exhausts the tile budget, and the compositor draws frames with missing tiles.Earlier heads tried CSS
zoom(it changed text wrapping against the render), then a stand-in property on each element (filter: opacity(1), then a farperspective). Each stand-in was visible to the editor or GSAP, so it could end up saved in a project, and went stale when classes changed. Both are gone; the transform fit stays.How
setPreviewRasterScale(packages/core/src/runtime/previewRasterHints.ts) runs from the runtime's existingset-display-scalehandler, which only a preview host sends (shown width / layout width). Renders never send it.[data-hf-preview-raster] { will-change: auto !important }, and puts that attribute on each layer whose computedwill-changeholds only transform hints and whose hint changes nothing:::before/::afterof the layer or a descendant, isposition: fixed(the hint is their containing block);opacityorfilterkeeps every hint, since those make boundaries of their own (for example forbackdrop-filter).will-changechanged.~,+) and:has()rules triggered from outside the layer;<style>text andinsertRule;opacityorfilterhint that reaches an already-marked layer through a rule on an ancestor's class or attribute (the layer's own changes are followed).data-hf-upcoming,data-hf-preview-lazy,data-hf-preview-raster) are one list,PREVIEW_ONLY_ATTRSin@hyperframes/core/studio-preview-mark. Copy (liveMarkupWithoutPreviewMarks) and text edits (buildTextField) leave them out of what is saved, and the runtime's keep-unchanged-media check ignores them, so a marked video keeps playing through an edit (this also fixes the same rebuild fordata-hf-upcoming). Studio's stacking model counts a marked layer as the stacking context its hint makes, so timeline z-order edits match the render.Measured
Synthetic composition of 48
will-changelyric layers (a fixture built for this PR), 528px player, DPR 2, 80 MB tile budget, full Chrome 154, 3 paired runs against the merge base. Playback for 8 s, then the player grown to 1100px and shrunk back to 528px.will-changecompositor layersThe layout of every element is unchanged (checked to 0.01px by the browser test). The paint is unchanged too: the browser test's frosted-glass and 3D probes compare the swap on and off at the same size.
Tests
packages/player/tests/browser/preview-raster-size.ts(player-perf load shard,test:browser-raster), fixturetests/perf/fixtures/preview-raster:::beforebehind them, 3D or a mixed hint keep their hint while the others lose it, and that a layer added later loses it too;set-display-scalemessage: with it off, the compositor trace must show layers rastered above 1.5x (so the measurement can fail); with it on, three traces in a row must show none. Frosted glass (backdrop-filterinside a hinted wrapper) and a 3D-rotated child must render the same both ways;packages/core/src/runtime/previewRasterHints.test.ts: which layers are marked and which keep their hint, restore leaving the document byte-identical, late layers, class changes, inline z-index, position andwill-changechanges, added stylesheets and attribute selectors.packages/studio/src/utils/authoredSource.test.tsanddomEditingTextFields.test.ts: copied markup and a saved text edit carry no preview mark.packages/core/src/runtime/stackingContext.test.ts: a marked layer keeps its stacking context.init.swapScenes.test.ts: a video carrying either preview mark is kept through a scene swap.previewRasterHints.test.tsalso checks that nothing is injected before aset-display-scalearrives.class, and reading hints through the mark.Before
A 528px player on a 2x screen with an 80 MB tile budget, playing the 48-layer fixture on main (merge base). Every hinted layer rasters at 3.6x the size shown, and 4 frames are drawn with missing tiles (text cut off or gone for a frame).
flicker-preview-before.mp4
After
The same playback with this PR: layers raster at the size shown, no frame is drawn with missing tiles, and the layout is the same.
flicker-preview-after.mp4
Review
An independent adversarial review ran on every head of this rework. What it found and what changed:
filterstand-in leaked into saved projects through Studio's blur slider and brokebackdrop-filter; a farperspectivestand-in could still be saved through perspective keyframes and the copy fallback. Both are gone: the runtime writes no inline style.transform, opacity) and inline style changes could make a dropped hint matter. They are now checked and followed, with tests.