Repository navigation
fix(core): a resized preview video keeps its picture while its new copy loads - #6922
Conversation
There was a problem hiding this comment.
Reviewed at c620429b.
Blocker (process): this is a fix PR, and the body has no fails without the fix: line naming a test. The test plan says what breaks when each line is removed ("fails 2… 1… 1… 1"), but it names no test on that line. Please add one, for example:
fails without the fix: packages/core/src/runtime/mediaProxy.test.ts › a copy sized to how large the video shows › shows a still of its frame while a resized copy loads, then its own poster again
I'll approve at the new head once that's there and nothing else changes.
Code: I found no blockers. Notes, none blocking:
restoreOriginalwithoriginal === nullremovessrcand callsload(), so noloadeddata/seeked/errorfollows. The held still then stays as the poster for good, which is a frozen frame where the element used to be empty. That needs a<video>with nosrcattribute that was still proxied, so it's unlikely, but you could skipholdShownFramewhenoriginal === nullor also release onemptiedthere.loadeddatawithseeking === falsereleases before the runtime seeks the new file to the playhead. On a paused swap away from t=0 that can flash frame 0 for a frame instead of showing black. That's still better than before, but the manual recording is worth checking for it.writtenShapenow drops an authoredposter="", the same way it drops an emptyclass. That's fine.- Manual testing is still unchecked ("follows in a comment").
Edit accuracy: accurate 271 (base branch 271), smooth 104 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
There was a problem hiding this comment.
Re-reviewed at a1aedfed. Approving.
- The body now has a
fails without the fix:line. The named test (… shows a still of its frame while a resized copy loads, then its own poster again) expectspostersto be[STILL]at load, and withoutholdShownFrameinloadProxythat can't happen, so it does fail without the fix. - Putting the original back with no
srcno longer leaves a still stuck:letGoOfStillputs the authored poster back right away, and a new test covers it. The release condition is the earlier early-return turned inside out, so nothing else about it changed. - My earlier frame-0 concern is answered by the recorded runs: the seek starts at
loadedmetadata, beforeloadeddata. - The Comments check passes at this head.
Nit (not blocking): when the original's file comes from a <source> child, load() does pick that child and load it. So this path still blinks black instead of holding a still; it just can't get stuck any more. You could hold here and release on loadeddata, but that can be a follow-up.
…py loads Swapping a video to a copy made for its new size emptied it until the copy decoded, so the preview showed black for a moment. The swap now shows a still of the frame it had, as the video's poster, until the new copy can draw.
a1aedfe to
8b1c1d3
Compare
There was a problem hiding this comment.
Re-reviewed at 8b1c1d38. Approving.
- This head was rebased onto main. The engine and streaming-encoder changes in the compare come from #6920 and #6926, not from this PR. The PR's own diff is the same as
a1aedfed, which I approved, plusscripts/package-size-budgets.json. - The budget figure checks out: 1,678,811 × 1.05 = 1,762,751.55, rounded up to 1,762,752. That's the same rule #6865 used for unpacked. The 818 bytes this PR adds are the runtime's hold code, which ships twice.
- The
fails without the fix:line is still present and names a real test.
Nit (not blocking): resetting to measured +5% leaves about 84 KB of headroom on @hyperframes/core packed. The next few size jumps won't trip the 1% tolerance, so whoever owns the budget may want to tighten it again later.
jrusso1020
left a comment
There was a problem hiding this comment.
Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.
Reviewed at 8b1c1d3891a890622e3207f00be4633e5efee03c.
No new correctness finding. Both source-swap paths use the same hold/release helper (packages/core/src/runtime/mediaProxy.ts:205-290), and scene reconciliation compares the authored poster rather than the temporary still (init.ts:384-398). The release condition requires a drawable, non-seeking frame, or an error; release removes its three listeners. Rapid resizes reuse one hold; tainted-canvas failure leaves the original poster and proceeds with the swap.
Genesis's earlier process concern is resolved: the body names a real regression on its fails without the fix: line. The no-src restore releases an existing hold immediately. Genesis already notes that a <source> child can load after this branch, so that path can still blink; this review adds no duplicate finding. The packed budget arithmetic is consistent with the stated measured-size-plus-5% rule; I did not independently reproduce the package-size or recorded preview timing measurements.
Validation: core build passed. node ../../node_modules/vitest/vitest.mjs run src/runtime/mediaProxy.test.ts src/runtime/init.swapScenes.test.ts passed 234 tests. Four independent controls also passed: repeated swaps and listener cleanup, exact absent/empty poster restoration, and tainted-canvas fallback (238 combined). Removing the two hold calls made three relevant regression tests fail. No Desktop recording, physical-browser playback or visual alpha-video acceptance was rerun.
Need: yes, avoid blank preview frames during source swaps · Size: ok · Reuse: existing proxy-source identity bookkeeping and one hold helper · Simpler: already minimal · Structure: fine
Risk: medium — source-swap timing and authored-media identity affect preview continuity.
Verdict: COMMENT (post-merge: Ready)
Reasoning: No new defect found at this head; lifecycle controls and scene-reconciliation tests pass. Existing source-child and size-budget notes remain follow-ups.
— hf-review
What
When the preview resizes a video and switches it to a proxy copy made for its new size, the video keeps showing its last frame until the new copy can draw. Before, it showed black for about 100-250 ms.
Why
A preview proxy is sized to the box the video is shown at. When the stage changes size (a panel opening or folding beside it, a window resize),
keepSizedasks for a copy at the new size andloadProxyswaps it in withel.src = href; el.load(). That empties the element (emptied, readyState 0), and the video draws nothing until the new file's first frame decodes. At a lowered preview quality any change of box asks for a new copy, so a panel that settles over a few frames swaps several times and the picture blinks black each time. Putting the original back when quality returns to full (restoreOriginal) does the same.How to reproduce: open a project with a video clip, lower the preview quality, then change the preview's size a few times. A per-frame recording shows the picture area black for 3-7 frames after each change. A text-only composition never does: there is no media element to empty.
Related work
None found.
How
holdShownFrame(inmediaProxy.ts, called by both swaps): before the source changes, a video showing a frame gets a still of that frame as itsposter. The still is a JPEG at most 640 px wide, drawn through a canvas. A video with no data paints its poster with the sameobject-fit, so the old picture stays up through the gap. The authored poster (or none) comes back once the new file has a frame and is not seeking, or at once if it errors.heldPosters/unheldPostersit besideswappedElements/unproxiedSrcinproxySrc.ts, and a scene swap'swrittenShapereads the poster as authored, as it already readssrc. A video that is mid-swap still counts as unchanged markup.srcto restore (the video's file came from a<source>child) loads nothing, so no event would release a held still. That path lets go of any held still at once instead of holding a new one.loadeddataorseekedonly when the video can draw and is not seeking. In six recorded runs the runtime started its seek to the playhead atloadedmetadata, beforeloadeddata, so the release waited through it: everyloadeddataafter a swap arrived at the playhead time, never at frame 0. No seek followed aloadeddatawithin the swap.Size budget
@hyperframes/core's packed budget goes from 1,661,748 to 1,762,752 bytes: the size CI measured at this PR's head, 1,678,811, × 1.05, as #6865 did for the unpacked budget. Main already packs at 0.95-0.98% over the old budget (1,677,535 bytes when built locally). This PR adds 818 packed bytes, about 1 KB of minified code that ships twice (the runtime IIFE and its inlined copy), which tipped it past the 1% tolerance. The minified hold is already lean; dropping bytes would mean dropping the still.Test plan
mediaProxy.test.tssrcto restore lets go of a held still.posteras written, 1; not letting go when nothing loads back, 1.packages/core:vitest run src/runtime2697 passed (112 files);mediaProxy.test.tspassed 3 runs in a row;tsc --noEmit,oxlintandoxfmtclean on the changed files.