Repository navigation
fix(producer): export sizes an unsized phone video as it plays, not as stored - #7228
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 0 important · 0 nits · head 2b210b4f
The poster-hint fix is right, and the fails without the fix: line names real tests. Two things block it:
- The
Commentscheck fails at this head.comment-ratchet.mjsreports the comment share rose inffprobe.ts(+4 lines) andrenderOrchestrator.ts(+1). Move those explanations into names or types, or cut them. - The layer path can regress for a rotated anamorphic clip (inline,
videoFrameExtractor.ts:1247).
Scope
I read all 6 changed files and the consumers of buildVideoLayerInput (videoLayerPassthroughPlan.ts, and the streamingEncoder.ts overlay). Other readers of stored width/height (background-removal/pipeline.ts, init.ts) are unchanged, as the PR intends. CI was still running when I reviewed, except Comments, which had already failed. I tested the ffmpeg behaviour on 5.1 with a 320x180 clip, SAR 4:3 and rotation 90.
— Genesis lane B
| }); | ||
| const filters = chain(startTime); | ||
| if (metadata.width !== canvas.width || metadata.height !== canvas.height) { | ||
| const shown = displaySize(metadata); |
There was a problem hiding this comment.
blocker: displaySize gives Chrome's size, but this compares it with the size ffmpeg decodes. For a rotated clip with non-square pixels, those two sizes differ. ffmpeg's autorotate transposes before scale=trunc(iw*sar/2)*2:ih, and the transpose inverts the SAR. So the width scales the stored height by 1/SAR, which is not what displaySize assumes (stored width × SAR, then swap).
Repro: I made a 320x180 clip with SAR 4:3 and rotation 90, then ran it through this chain. It decodes to 134x320. displaySize returns 180x426. On a 180x426 canvas, the new check skips the scale, so [1:v] reaches the overlay in streamingEncoder.ts:478 at 134x320 instead of the canvas size. That is the same small-frame bug this PR fixes. On main, 320≠180, so it always scaled.
Fix: keep the stored-size check here (metadata.width/height), or compare against the size the chain really outputs (turn first, then trunc(h/sar/2)*2). Then fix the comment in displaySize and the widens anamorphic pixels before the turn test, which say the order matches the decode.
Edit accuracy: accurate 267 (base branch 267), smooth 121 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
There was a problem hiding this comment.
Verdict: APPROVE — 2 resolved, 0 open, 0 new · 2b210b4f → e56e65d3
✅ The layer path is back to main's stored-size check, so a rotated clip with non-square pixels still gets scaled to the canvas. ✅ The new comment lines are gone, and Comments passes. CI passes too. regression and Windows render verification were still running when I approved.
— Genesis lane B
* chore: release v0.9.16 * chore: format v0.9.16 release notes * chore: add heygen-com#7228 and heygen-com#7231 to v0.9.16 notes * chore: add heygen-com#7217 heygen-com#7224 heygen-com#7225 heygen-com#7226 heygen-com#7229 heygen-com#7233 to v0.9.16 notes
What
A
<video>with no CSS size now renders at the size it plays at, sample aspect and rotation applied. Until now a phone's portrait clip (stored 1920x1080 with a -90 display matrix, as an iPhone records it) was laid out by export as a 1920x1080 box. In a 1080x1920 film that showed the picture small, letterboxed at the top right, while preview showed it filling the frame.Why
Reported against 0.9.13: an iPhone HEVC
.MOVin a portrait composition,<video class="clip">with no width or height, exports wrong while preview is right. An explicit 1080x1920 size worked around it. Phone footage is common, and nothing tells the person why export differs from preview.Related work
None open on this path.
Size
A lone fix: no open PR touches how export sizes a video, so nothing else can carry it.
How
Export gives each extracted
<video>a poster of its natural size (applyVideoMetadataHints), so layout is right before the first injected frame. That size came fromcollectVideoMetadataHints, which passed ffprobe's storedwidthandheight. Chrome and ffmpeg's autorotate both show the clip turned upright.extractMediaMetadatanow records the display matrix's turn asrotation(side data, else an older muxer'srotatetag).displaySize(metadata)inffprobe.tsturns stored size into the size Chrome lays the element out at: sample aspect applied to the width, then a quarter turn swaps the sides. The poster hint uses it.widthandheightkeep meaning the stored size. The BT.601/709 guess for untagged streams and background removal's own transposition read them that way.buildVideoLayerInputkeeps comparing the stored size with the canvas. Its frames come from ffmpeg, which turns the frame before widening the pixels, so a rotated anamorphic clip decodes to a different size than Chrome's. The stored-size check always scales a rotated clip to the canvas, which is correct (found in review).Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
fails without the fix:
collectVideoMetadataHints > sizes a phone's portrait clip as it plays, not as it is storedfails on main'srenderOrchestrator.ts(hint 1920x1080), and the threea phone clip's display sizetests inffprobe.test.tsfail on main. All pass here.renderOrchestrator.test.ts(298), the newffprobe.test.tscases andvideoLayerInput.test.ts, 3 runs in a row on Linux. Engine and producertscare clean, and so are oxfmt and oxlint on the changed files.ffprobe.test.tshas 4 failures on that Linux box with or without this change (the HDR PNG fixture tests andprobeMediaProfile). They read a checked-in fixture that machine's copy lacks. CI has it.Real render, the report's layout: a 1080x1920 composition, an unsized
<video class="clip">, and an HEVC.MOVstored 1920x1080 with display rotation -90. Left: the clip's own upright frame. Middle: rendered by 0.9.13, small at the top right. Right: rendered from this branch, filling the frame.Not covered: the reporter's own file, which isn't available, and an anamorphic clip in a real render (unit test only).