Repository navigation
@remotion/media: Avoid duplicate animated video effect paints - #12542
alec-watts wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
ℹ️ No critical issues — one observation inline.
Reviewed changes
This run reviewed the full diff of PR #12542 (3 files, 1 commit), which removes duplicate animated video-effect paints.
packages/media/src/video/video-for-preview.tsx— a newlastEffectsTimeref makes the effect-onlyredrawVideoEffects()layout effect a no-op whenever the same commit already advancedcurrentTime, on the assumption that the seek path repaints with the latest effects.packages/media/src/video-iterator-manager.ts— a seek whose trimmed time rounds equal tocurrentSeeknow callsredrawCurrentFrame()before returning, so a one-frame loop with changing effect params repaints the retained picture instead of dropping the update.packages/media/src/test/video-effect-redraw.test.tsx— new browser regression that counts effect applications and checks output pixels across successive seeks, a one-frame loop, and a paused effect edit.
The coalescing logic is sound: effects is identity-memoized by useMemoizedEffects, so a same-frame re-render with unchanged params does not re-run the redraw effect, while a frame advance is always followed by a seekTo for the new currentTime (the useCommonEffects dedup only skips when the time is unchanged). I did not find a common-case regression.
deepseek-v4.1-flash | 𝕏
JonnyBurger
left a comment
There was a problem hiding this comment.
There is one regression to fix before merging: in a one-frame loop with static effects and an animated effectsOutputSize, the preview skips its redraw because the composition time changed, while the iterator skips its redraw because the effects array is unchanged. The effect keeps the old resolution. I reproduced this in a browser: the effect stayed at width 160 when it should have updated to 176.
I prepared a fix that also tracks effect-chain state when deciding whether a same-media-time seek needs a redraw, and extended the existing browser regression to verify the rendered pixels:
d99befe
Please cherry-pick that commit or apply an equivalent fix. The regression is red/green verified; 16 focused browser tests, TypeScript, lint, and formatting pass. The broader remote-media tests timed out locally.
I would have applied the fix myself, but maintainer edits are disabled on this PR. Please enable “Allow edits by maintainers” if you would like me to push it directly.
|
Your prepared resize fix is included as e57bc33, preserving author credit. I verified the same-media-time redraw compares both effect identity and effect-chain state, and all 16 focused Chromium tests pass after refreshing onto main. Final head 729479d passes every applicable upstream CI job, including Required CI: https://github.com/remotion-dev/remotion/actions/runs/40047802558 . The first refresh hit unrelated GT Planar font-loading timeouts; the identical-tree recheck passed without changing tests or workflows. Ready for another review. |

Changing video effect parameters with each composition frame paints the previous decoded picture through
redrawVideoEffects(), then paints the newly selected picture through the ordinary seek. Both execute the effect chain and invoke the frame callback.Let the existing time-update seek paint the latest effects. Retain effect-only redraws when time is unchanged, and redraw a retained picture when a new composition frame maps to the same media timestamp and either its effects or its effect-chain state changed. Comparing chain state preserves animated
effectsOutputSizewith otherwise static effects in a one-frame loop.Applied the maintainer's prepared resize fix as
e57bc3360d, preserving author credit, and merged current main (dc32f3b1e0) in head66590de3a0.Validation
Refreshed onto upstream main
0eddd9ae31ine2fc8bb71c, without conflicts.Final head
729479d455: upstream CI passed all applicable jobs, including Required CI, Windows/Ubuntu/macOS package suites, web-renderer tests, browser E2E, lint/formatting, SSR and integration. The empty recheck commit has the identical code tree toe2fc8bb71c; the earlier GT Planar font timeouts did not recur. No timeout, assertion, dependency or workflow was changed.Both automatic Vercel previews passed. No manual redeploy was requested.
The earlier production-React Chrome probe over a 30-frame advance measured 31 animated effect applications versus 60 on main. This measures avoided work, not an FPS or CPU improvement, and predates the resize follow-up. The earlier reversal verified the resize regression fails on the pre-fix iterator and passes with the maintainer's fix.
Why each file changes
packages/media/src/video/video-for-preview.tsxpackages/media/src/video-iterator-manager.tspackages/media/src/test/video-iterator-manager.test.tspackages/media/src/test/video-effect-redraw.test.tsxDiff cleanup: reuses the existing seek, paint, and effect-chain owners. No scheduler, queue, cache, public API, dependency, generated bundle or CI workflow was added.