Repository navigation
feat(producer): opt-in video layer passthrough for a lone full-canvas video - #6914
Conversation
…passthrough is on
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 0 nits · head f306bfb5
This is the first review on the PR, so I reviewed the full diff rather than just the trim since 18cc5dbb.
- blocker: the required
Commentscheck fails on this head because the comment ratchet rose in three files:engine/src/types.ts(+1 line),extractVideosStage.ts(+1) andprobeStage.ts(+1). Cut those comment lines or move the explanation into a name. - One more blocker and one important finding are inline. ↓
Scope
Read all 14 files. I also traced initTransparentBackground, which the HDR layered paths share with clearCompositionRoot. CI was partly still running: CI, regression, Windows render verification and CodeQL were in progress. This is a feature PR, so no fails without the fix: line is needed.
— Genesis
Edit accuracy: accurate 271 (base branch 271), smooth 108 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
There was a problem hiding this comment.
Verdict: APPROVE — 0 resolved, 0 open, 0 new · ee323516 → 59c0f427. Static-frame dedup now ignores only the video that capture leaves out (the ids in hiddenElementIds). Reused frames still go to the encoder once per frame, so ffmpeg's video layer stays in step with the page. The rest of the diff is the evaluate callback's re-indent and shorter comments. Comments passes. CI, regression, Windows and CodeQL were still running when I approved.
— Genesis
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 59c0f4278900e2aa590efed0de13ea8f772871a4.
Important follow-up — packages/producer/src/services/render/videoLayerPassthroughPlan.ts:61, packages/engine/src/services/videoFrameExtractor.ts:1240: passthrough still accepts video styling that ffmpeg does not reproduce. inspectVideoLayout checks objectFit but discards objectPosition and does not refuse border radii. buildVideoLayerInput always uses a centered crop, and the final graph composites an unrounded source rectangle beneath the hidden video. A full-canvas object-fit:cover; object-position:0% 50% video therefore changes its framing when this opt-in path is enabled. Rounded video corners are also filled back in.
I exercised the exact source layout function in real Chromium with a real 320×90 red/blue VP9 source displayed in a 160×90 canvas. Both centered and left-positioned cases qualify {fit:"cover"}. At pixel (70,45), the left-positioned browser image is red [254,0,0], while the centered ffmpeg crop is blue [0,0,254]; the centered browser control is blue. A border-radius:30px case also qualifies while its browser corner is transparent. The CSS individual-scale control falls back through the bounding-box check. These are targeted function/browser/filter witnesses, not a complete Producer render.
Concrete fix: fail closed on non-centered object-position when cover crops, and on nonzero video border radius / unsupported clipping, or carry the measured positioning/mask through the encoder. Add regression cases for left/right anchoring and rounded corners, with a centered rectangular positive control. Keep the feature off by default until these presentation contracts are covered.
Genesis's previous wrapper-background and nested-composition tint findings are fixed by the current ancestor/subcomposition checks. The changed dedup analysis ignores only supplied hidden video IDs; reuse still feeds each output frame to the encoder. The source frame/color chain is shared, and extraction/audio/readiness and one-worker PNG streaming plumbing are consistent in the inspected paths.
Validation: 155 engine tests (videoLayerPassthrough, streamingEncoder, frameCapture) and 3 producer layout tests pass. The Chromium/ffmpeg controls above expose the missing contract beyond those tests. I did not rerun the author's 30-second/600-second performance or SSIM measurements, 4K films, or the complete render pipeline.
Need: yes, opt-in removal of repeated video capture has a concrete benefit · Size: ok, one engine/producer feature · Reuse: shared extraction color/frame filter builder · Simpler: reject unsupported layout instead of approximating it · Structure: fine
Risk: medium — opt-in rendering optimization can silently alter picture content.
Verdict: COMMENT (post-merge: With fixes)
Reasoning: Opt-in passthrough is already merged, but targeted browser/filter witnesses show a picture mismatch for accepted presentation styles. Refusing unsupported styling is the smallest safe follow-up.
— hf-review
What
An opt-in render mode (
HF_VIDEO_LAYER_PASSTHROUGH=true, off by default) for a composition whose only video fills the whole canvas and the whole timeline. The page is captured without the video, as transparent PNG frames, and ffmpeg composites the source video underneath while encoding. This skips the per-frame round trip of the video through JPEG, the page and a second JPEG (refs #6881).Anything that does not qualify renders exactly as today, and the log says why (
Video layer passthrough off: <reason>).When it applies
MP4 output, software encode, SDR, streaming encode eligible, exactly one video that starts at 0, covers the whole render, plays once at normal speed, is not colour-graded or hidden, and is a local file. A page check at nine sampled times confirms it fills the canvas, has no opacity, transform, filter, clip or blend on it or its ancestors, and nothing paints under it. HDR sources, variable-frame-rate sources and sources with alpha fall back.
It runs on one worker with screenshot capture. Frame choice and colour use the same filter chain as frame extraction (now one shared builder), so the two cannot drift.
Static-frame dedup and the left-out video
The passthrough capture hides the video, but the dedup eligibility scan still counted any
<video>in the page and switched dedup off ("video").computeStaticFrameSetnow ignores videos left out of capture (hiddenElementIds). On a 30 s test film this took capture from 74.8 s to 27.7 s (821 of 900 frames reused) with unchanged SSIM.Comments moved here
totalDuration(), notduration(), because a repeat or yoyo tween animates past one iteration; a repeating timeline is marked opaque over its whole span.tl.call()disqualifies the composition. It is a zero-duration tween whose callback is wired asonCompleteandonReverseComplete, so both directions run the same forward side effect. A DOM change driven that way is not seek-idempotent: the verifier's own seeking can mutate the page permanently and leak into a later static run. A harmless call cannot be told from a harmful one without running it.call()inside it is still detected before the verifier seeks through that span.Checked so far
Not checked yet
Size
About 390 changed lines across engine and producer, one feature.