fix(engine): preserve video frame replacement geometry

[miguel-heygen]

Problem

Fixes #837.

The issue repro uses full-bleed 1920x1080 videos (position:absolute; inset:0; width:100%; height:100%) with image overlays that overlap clip boundaries. During render, HyperFrames swaps native <video> elements for pre-extracted frame <img> elements so screenshots stay frame-accurate. That replacement image was inheriting the video's authored layout constraints after we had already measured and assigned the used frame box.

What this fixes

1. Video frame injector layout leak (ab533b3)

The video frame injector now treats layout geometry as owned by the measured video box. It copies visual parity styles such as filters, masks, border radius, object fit, z-index, and transforms, but it no longer copies width, height, top, left, right, bottom, or inset from the source video onto the replacement image.

This keeps the injected frame pinned to the same 1920x1080 box as the source video without also carrying opposing constraints like inset: 0 / right: 0 that can overconstrain replaced-image sizing on Chrome capture paths.

2. Always pass clip to Page.captureScreenshot (0861b18)

The original fix only passed the CDP clip parameter when deviceScaleFactor > 1 (supersampling). Without an explicit clip at dpr=1, Chrome can resolve replaced-element sizing differently when full-bleed absolute videos interact with overlay layers — producing anisotropic frame stretching on some compositor paths. Now clip with scale=dpr is always passed, including at dpr=1, ensuring geometry is locked to the measured viewport dimensions.

Credit to @brian-t-allen for identifying this fix.

Root cause

injectVideoFramesBatch() set the replacement image geometry from offsetLeft, offsetTop, offsetWidth, and offsetHeight, then copied the shared media visual style list afterward. That shared list includes layout properties. For full-bleed absolute videos, the copy step overwrote right: auto, bottom: auto, and inset: auto with the video's computed 0px constraints.

The result was an injected <img> with both measured dimensions and opposing inset constraints. Some Chrome compositor/capture paths can resolve that overconstrained replaced element differently from the native video and resample the frame anisotropically around overlay/layer changes.

Additionally, pageScreenshotCapture only sent clip when supersampling (dpr > 1), leaving the default capture path at dpr=1 without geometry constraints — allowing Chrome's compositor to resolve replaced-element sizing ambiguously.

[jrusso1020]

LGTM — clean two-part fix, well-targeted at the actual root cause.

Audited

• injectVideoFramesBatch layout leak fix (screenshotService.ts:380-432) — the root cause is precise: visual-style copy ran AFTER the measured box was assigned, overwriting right: auto / bottom: auto / inset: auto (defaults for a fresh absolute <img>) with the source video's computed 0px constraints inherited from inset: 0. Two coordinated changes resolve this:
  • New replacementLayoutProperties = new Set(["width", "height", "top", "left", "right", "bottom", "inset"]) — the copy loop skips these
  • Style copy now runs BEFORE the measured-box assignment, then img.style.position = "absolute" + left/top/width/height from offsetLeft/Top/Width/Height are the final authority
  • Removed the prior sourceIsStatic exclusion — that only skipped layout copy for position: static sources, but #837's bug shape is position: absolute + inset: 0, which the old gate didn't cover ✓
• pageScreenshotCapture always-clip fix (screenshotService.ts:130-144) — was dpr > 1 ? clip : undefined; now unconditionally passes { x: 0, y: 0, width, height, scale: dpr }. Math is identical to before (the underlying capture dimensions don't change), but the explicit clip constrains Chrome's compositor to resolve replaced-element geometry against the measured viewport instead of resolving anisotropically when full-bleed absolute videos meet overlay layers. Brian Allen's empirical debugging caught the call shape difference. ✓
• Body claim verification — every claim matches the diff:
  • "It copies visual parity styles such as filters, masks, border radius, object fit, z-index, and transforms, but it no longer copies width, height, top, left, right, bottom, or inset" — exclusion set ✓
  • "Always pass clip to Page.captureScreenshot" — verified ✓
  • "Credit to @brian-t-allen" — issue filer + comment author ✓
• Test fixture (packages/producer/tests/render-video-overlay-stretch/) — regression-gates the exact #837 shape: two 2s clips with position:absolute; inset:0; width:100%; height:100%; object-fit:cover + an image overlay crossing the cut. meta.json PSNR threshold 30, frame failures 0. ✓
• Unit test for injectVideoFramesBatch — stubs getComputedStyle to return the exact inset: 0 shape, runs the injection, asserts img.style.right/bottom/inset === "auto" (the defaults survive when no copy overrides them). Pins the fix. ✓
• CI — Test, Typecheck, Lint, Build, Format, CodeQL, Test: runtime contract, CLI smoke (required), Tests on windows-latest, Render on windows-latest, Smoke: global install, Preview parity, preview-regression, player-perf, Perf:drift/load/fps/parity/scrub all green. regression-shards still in-progress; the cancelled jobs in the run list are stale runs cancelled when newer commits pushed. ✓

Non-blocking note

replacementLayoutProperties is a hardcoded string set. If MEDIA_VISUAL_STYLE_PROPERTIES ever grows to include CSS Logical Properties (block-size, inline-size, inset-block-start/end, inset-inline-start/end), the same overconstrained-replaced-element bug could re-emerge for compositions using logical-property layout. Not relevant today since the current list is physical-only, but worth a comment near the exclusion set noting "if you add a layout-affecting property to the visual-style list, add its physical/logical counterparts here too" — or better, derive the exclusion from a structural categorization (e.g., extract LAYOUT_PROPERTIES from parityContract.ts so both lists stay in sync). Cosmetic / future-proofing.

Closes #837 cleanly with the regression fixture pinning the exact repro shape.

— Review by Rames Jusso (pr-review)

[vanceingalls]

Fix lands on the correct root cause and is decomposed cleanly into two independent levers. The PR-body framing — replaced-image overconstraint from inset:0 carried over after the used-box was already pinned, plus Page.captureScreenshot resolving replaced-element sizing differently without an explicit clip at dpr=1 — matches the code change.

Calibrated strengths

• packages/engine/src/services/screenshotService.ts:380-428 — replacementLayoutProperties block-set + reordering the copy loop before the used-box assignment so the measured pixel geometry is final. Comment at 411-415 spells out the failure mode for future readers.
• packages/engine/src/services/screenshotService.test.ts:97-193 — the new injectVideoFramesBatch unit test pins the exact pre-fix bug (right: 0px / bottom: 0px / inset: 0px on the injected img after the swap) by asserting all three resolve to auto. That is the right shape: pin the failure mode, not the wording.
• packages/producer/tests/render-video-overlay-stretch/ — the regression fixture is the input topology from the repro (two full-bleed position:absolute;inset:0 videos + an overlay across the cut) with minPsnr: 30, maxFrameFailures: 0. A future regression on this code path will fail this test deterministically.

Findings

important — Parity-harness emulator drift. packages/producer/src/parity-harness.ts:161-205 (emulateProducerVideoSwap) still implements the OLD copy logic — the sourceIsStatic branch is intact and top/left/right/bottom are copied from the source video's computed style when non-static. The harness's job is to emulate the production producer's video swap so preview-vs-producer parity is honest; with the production swap now ignoring those layout properties on the replacement <img>, this emulator can report false parity (a preview snapshot that does NOT reproduce the stretch when the producer would, or vice versa). Either align this with the production logic (the same block-set + post-layout ordering) or leave a comment that the drift is intentional. The risk is low-amplitude — the harness is opt-in via --emulate-producer-swap=true — but the contract is exactly the kind of thing the parity rig exists to catch.

nit — replacementLayoutProperties is exhaustive against today's MEDIA_VISUAL_STYLE_PROPERTIES (verified — width, height, top, left, right, bottom, inset all present in both). Worth considering min-width/max-width/min-height/max-height for defense if those are ever added to the visual list — they would re-introduce the same overconstraint failure mode. Pure future-proofing, not a current bug.

nit — pageScreenshotCapture at packages/engine/src/services/screenshotService.ts:133-138: the old comment that explained why the clip was only set for dpr > 1 (supersample contract) was deleted. The new always-clip path deserves a short comment with the new rationale (replaced-element sizing resolves consistently at dpr=1 when geometry is explicit) — the PR body has it; the code doesn't. The fix is right; just leave breadcrumbs for the next reader.

Notes

• CI: regression-shards 1-8 were still in progress at review time. mergeStateStatus: BLOCKED is reviewer-gate, no required-check failures. Hold for the shards.
• Reporter (@brian-t-allen) suggested the always-clip fix in the issue thread and has not yet confirmed the updated branch on their M4 repro. Given the original ab533b3 was insufficient on their setup, a green ack from them before merge is worth waiting for.

Verdict: APPROVE
Reasoning: Root cause is correctly identified and split into two independent, well-pinned fixes; tests pin the failure shape; only finding with a concrete failure mode is parity-harness drift, which is opt-in and out of the merge-critical path.

Review by Vai

[vanceingalls]

Verdict note: Intended APPROVE; stamp-harness gated the --approve write.

Fix lands on the correct root cause and is decomposed cleanly into two independent levers. The PR-body framing — replaced-image overconstraint from inset:0 carried over after the used-box was already pinned, plus Page.captureScreenshot resolving replaced-element sizing differently without an explicit clip at dpr=1 — matches the code change.

Calibrated strengths

• packages/engine/src/services/screenshotService.ts:380-428 — replacementLayoutProperties block-set + reordering the copy loop before the used-box assignment so the measured pixel geometry is final. Comment at 411-415 spells out the failure mode for future readers.
• packages/engine/src/services/screenshotService.test.ts:97-193 — the new injectVideoFramesBatch unit test pins the exact pre-fix bug (right: 0px / bottom: 0px / inset: 0px on the injected img after the swap) by asserting all three resolve to auto. That is the right shape: pin the failure mode, not the wording.
• packages/producer/tests/render-video-overlay-stretch/ — the regression fixture mirrors the reporter's repro topology (two full-bleed position:absolute;inset:0 videos + an overlay across the cut) with minPsnr: 30, maxFrameFailures: 0. A future regression on this code path will fail this test deterministically.

Findings

important — Parity-harness emulator drift. packages/producer/src/parity-harness.ts:161-205 (emulateProducerVideoSwap) still implements the OLD copy logic — the sourceIsStatic branch is intact and top/left/right/bottom are copied from the source video's computed style when non-static. The harness's job is to emulate the production producer's video swap so preview-vs-producer parity is honest; with the production swap now ignoring those layout properties on the replacement <img>, this emulator can report false parity (a preview snapshot that does NOT reproduce the stretch when the producer would, or vice versa). Either align this with the production logic (the same block-set + post-layout ordering) or leave a comment that the drift is intentional. Risk is low-amplitude — the harness is opt-in via --emulate-producer-swap=true — but the contract is exactly the kind of thing the parity rig exists to catch.

nit — replacementLayoutProperties is exhaustive against today's MEDIA_VISUAL_STYLE_PROPERTIES (verified — width, height, top, left, right, bottom, inset all present in both). Worth considering min-width/max-width/min-height/max-height for defense if those are ever added to the visual list — they would re-introduce the same overconstraint failure mode. Pure future-proofing, not a current bug.

nit — pageScreenshotCapture at packages/engine/src/services/screenshotService.ts:133-138: the old comment that explained why the clip was only set for dpr > 1 (supersample contract) was deleted. The new always-clip path deserves a short comment with the new rationale (replaced-element sizing resolves consistently at dpr=1 when geometry is explicit) — the PR body has it; the code doesn't. The fix is right; just leave breadcrumbs for the next reader.

Notes

• CI: regression-shards 1-8 were still in progress at review time. mergeStateStatus: BLOCKED is reviewer-gate, no required-check failures. Hold for the shards.
• Reporter (@brian-t-allen) suggested the always-clip fix in the issue thread and has not yet confirmed the updated branch on their M4 repro. Given the original ab533b3 was insufficient on their setup, a green ack from them before merge is worth waiting for.

Verdict: APPROVE
Reasoning: Root cause is correctly identified and split into two independent, well-pinned fixes; tests pin the failure shape; only finding with a concrete failure mode is parity-harness drift, which is opt-in and out of the merge-critical path.

Review by Vai