Repository navigation
fix(engine): local renders lay out width-only videos like the browser - #5771
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 173 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 33aae6b3. The only change since 6c26d10d renames the poster variable and drops a comment. Swapping the width/height attribute hints for a transparent SVG poster is the right fix. The attributes were acting as CSS sizes, so a video whose page set only one dimension (or only max-width) had its other axis pinned to the probed pixel size. A poster sets the video's natural size, which is what Chrome would use once the real metadata loads, so every CSS combination lays out the same way it does in the preview.
What I checked:
- Callers: all three call sites at
frameCapture.ts:2509,:2665and:3459await the now-async evaluate, so the poster decode finishes before capture starts. - Removed attributes and fallback: nothing in engine, core runtime or producer reads the hinted
width/heightattributes or the removedaspect-ratiofallback. The frame-injection overlay inscreenshotService.ts(~line 798) sizes the<img>fromgetBoundingClientRect, so it picks up the corrected box with no other change. - Authored posters: a video that already has a
posteris skipped, so authored posters are never overwritten. The PR body states the trade-off. - Test: the browser test pins the six boxes Chrome gives a 640x360 video once its metadata has loaded, including the bordered flex item that defeated the
contain: sizeapproach. The PR body reports it failing onmainfor four of them, so it does guard against this regression.
Non-blocking:
image.decode()rejects if a poster fails to decode, which would makePromise.allthrow and abort page setup. With a well-formed SVG data URL built from validated finite positive numbers that shouldn't happen.Promise.allSettledwould still make the worst case a wrong box instead of a failed render.- The poster is a real
posterattribute on the live element. Studio's scene-swap URL comparison (sceneUrlsincore/src/runtime/init.ts) and the asset-error diagnostics both readposter. That doesn't matter in a render page, but it's worth remembering if this hint ever runs in a preview context.
No live run on my side; I relied on the PR body's measured numbers and the browser test.
— Rames
What changed
A local
hyperframes renderdrew a<video>that the page sizes with only a CSS width (or only a height, or only a max-width) in the wrong box. A 200px-wide 16:9 clip got a 200x360 box with the picture centred inside it, so it sat lower than in the browser preview and in distributed chunk renders, which draw it at 200x112.Cause: local renders pass the probed size of every extracted video to the engine, and
applyVideoMetadataHintswrote it aswidth/heightattributes. Those attributes are CSS size hints, so whichever axis the page left unset took the hinted pixel value instead of following the video's shape.The hint exists for videos whose metadata Chrome never loads (frames are injected, so Chrome would otherwise lay them out at 300x150). It now gives such a video a transparent SVG poster of the probed size instead. Chrome takes a video's natural size from its poster until the video's own metadata loads, so every CSS combination lays out exactly as it does with the real video. The explicit
aspect-ratiofallback is gone for the same reason. A video that already has a poster is left alone.contain: size+contain-intrinsic-sizewas tried first and rejected: it matches the simple cases but shrinks a borderedwidth:100%flex item (style-9-prod's layout) by the border width.What I measured
frameCapture-videoMetadataHints.browser.test.ts: six 640x360 videos with no metadata (no size, width only, height only, both sizes with object-fit, max-width only, bordered flex item) must get the boxes Chrome gives the same markup once a real 640x360 video's metadata has loaded (measured separately: 640x360, 200x112.5, 160x90, 200x300, 100x56.25, 416x241).frameCapture.test.ts: 55 passed. Engine typecheck, oxlint and oxfmt clean.output/compiled.htmlin headless Chrome and compared every video's box with old hints, new hints and no hints, with media loading and with media blocked. With media loading, old and new give the same box in all 28, so no baseline should move. With media blocked, new matches the loaded box in all 28; old was off for style-9-prod (1920x1087 instead of 1936x1096).What I did NOT exercise