Repository navigation
fix(engine): a video held past its media end keeps following its animation - #6424
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 b69b5c48
The engine fix looks right to me: restyle-only items skip the transport, the src write and the decode, they aren't reported as injected, and the sub-sample guard keeps blurred video rows matching the plain render. Two things block it, both inline:
- The squash reverts #6416, which merged as this PR's base commit
dc56b3d2. It deletesisClipHiddenByRuntime, the layer-walk prune and its tests, and putsCLIP_HIDDEN_ATTRback inclipReveal.ts. This revert is also what trips thePR capturesgate (the PR now touches studio) and half ofComments(clipReveal.ts). Commentscomment-ratchet:videoFrameInjector.tsgoes from 19.1% to 19.2% (+2 comment lines).
Scope
Read in full: videoFrameInjector.ts, injectResolvedVideoFramesBatch, withVideoFrameSources (an empty updates passes straight through), and every onBeforeCapture caller. captureAccumulatedFrame injects at (frameTime, frameTime) and sub-samples come through prepareFrameForCapture with heldVideoTime. HDR passes no held time, so it restyles. Held hosts behind a hidden ancestor still get their img hidden. When the host shows again, the held frame is now published (before this change it was skipped). CI at review time: Comments and PR captures failed, the rest were still running. No prior reviews.
b69b5c4 to
f65fca9
Compare
Edit accuracy: accurate 271 (base branch 271), smooth 130 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
Merge 20 more upstream commits (acd0bc3..ad4d3fa, v0.8.205: heygen-com#6403, heygen-com#6363, heygen-com#6209, heygen-com#6395, heygen-com#6424, heygen-com#6426, heygen-com#6442, heygen-com#6430, heygen-com#6402, heygen-com#6435, heygen-com#6439, heygen-com#6449, heygen-com#5436, heygen-com#6455 and docs). No conflicts. Record the 11 open PRs heygen-com#6440-heygen-com#6458 and issue heygen-com#6443 in docs/DECISIONS.md (none adopted), re-check the earlier open PRs, and advance the commit watermark to ad4d3fa, the PR watermark to heygen-com#6458 and the issue watermark to heygen-com#6443. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
What
A
<video>whose clip runs longer than its media now keeps following its animation after the media ends. Before, once the last frame was held, the replacement image kept the opacity, transform and filter it had at that moment, so a fade-out or a move stopped dead for the rest of the clip.Why
#4049: a 2 s clip in a 4 s slot fading to black froze at the opacity it had at 2 s (reproduced on 0.8.193, R = 133 from frame 61 on, while a sibling div reached 3).
The frame injector skipped a video whose decoded frame index had not changed. That skip saved resending the picture, but it also skipped copying the video's style onto the replacement image. Past the end of the media the index never changes again, so the style never updated. The same skip also left the style one frame behind whenever a slower source repeats a frame.
Related work
Fixes #4049. Builds on the review of #4087 (closed), which found the right cause but re-sent and re-decoded the frame on every held frame and broke motion blur over video (#5150).
How
The skip now separates the picture from the style:
videoFrameInjector.ts: an unchanged frame index no longer drops the video. It goes intorestyleIdsunless the call is a motion-blur sub-sample (heldVideoTime !== time), which must keep the style injected at the output frame's time.screenshotService.ts:injectVideoFramesBatch(page, updates, restyleIds). A restyle item has no picture, so it skips the frame transport, thesrcwrite and the decode, and runs the same style copy as a new frame (one owner for that copy). It is not reported as injected, so the GPU reseek and the cache bookkeeping are unchanged.Design call, options considered:
syncVideoFrameVisibility: no extra round trip, but a second copy of the style-copy code in another page function.page.evaluateon frames where a held video is the only change.Test plan
Real render,
motionBlurVideo.integration.test.ts(Chrome + ffmpeg): a 0.2 s white clip in a 0.5 s slot fading out on black. Every frame after the media ends is darker than the one before and the last one is under 15% of the first (plain levels 255, 238, ... 34, 17). With motion blur on, every frame's pixels match the plain render. 7 of 7 tests passed, 3 runs in a row.Mutation: without the restyle, the fade test fails (
frame 7: expected 170 to be less than 170). Restyling motion-blur sub-samples too fails both the new blur test and the existing "keeps the video rows identical to the unblurred render" test.Unit tests updated: an unchanged frame now asks for a restyle with no picture (
[], ["pip"]), and a motion-blur sub-sample asks for nothing.videoFrameInjector.test.ts+screenshotService.test.ts: 59 passed, 3 runs.tsc --noEmitfor engine and producer, oxfmt, oxlint on the changed files: clean.Render cost, measured (640x360, 30 fps, 11 s slot, draft MP4, one worker, software GPU; branch and main engine interleaved, 5 rounds each, one build):
A normal render never takes the new path (0 restyle calls), so its per-frame work is unchanged; the wall difference there is within run-to-run spread. A held frame costs about 4 ms more (capture average 56 ms vs 53 ms), which is the style copy main skips and gets wrong.
Comments follow CONTRIBUTING.md "Comments".