Skip to content

@remotion/media: Avoid duplicate animated video effect paints - #12542

Open
alec-watts wants to merge 7 commits into
remotion-dev:mainfrom
alec-watts:codex/coalesce-video-effect-redraw
Open

alec-watts wants to merge 7 commits into
remotion-dev:mainfrom
alec-watts:codex/coalesce-video-effect-redraw

Conversation

@alec-watts

@alec-watts alec-watts commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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 effectsOutputSize with 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 head 66590de3a0.

Validation

Refreshed onto upstream main 0eddd9ae31 in e2fc8bb71c, without conflicts.

  • Core/player/media production dependency builds and TypeScript checks pass (4 tasks).
  • All 16 focused Chromium tests pass across effect redraw, iterator ownership/seeks and preview iterator timing, including the maintainer's static-effect/animated-output-size one-frame-loop regression.
  • Diff cleanup finds no bloated files; whitespace checks pass.

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 to e2fc8bb71c; 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

File Reason
packages/media/src/video/video-for-preview.tsx Avoid effect-only painting when the same commit already requests a new video time.
packages/media/src/video-iterator-manager.ts Preserve changing effects or effect resolution when composition time advances but mapped media time does not.
packages/media/src/test/video-iterator-manager.test.ts Guard against extra frame callbacks on same-time seeks without changed effects.
packages/media/src/test/video-effect-redraw.test.tsx Verify one effect application per frame update, actual pixels, repeated-source-time behavior, paused edits and animated output sizing.

Diff cleanup: reuses the existing seek, paint, and effect-chain owners. No scheduler, queue, cache, public API, dependency, generated bundle or CI workflow was added.

@vercel

vercel Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
bugs Ready Ready Preview Oct 24, 2026 1:10am UTC
remotion Ready Ready Preview Oct 24, 2026 1:10am UTC

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 new lastEffectsTime ref makes the effect-only redrawVideoEffects() layout effect a no-op whenever the same commit already advanced currentTime, 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 to currentSeek now calls redrawCurrentFrame() 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash | 𝕏

@JonnyBurger JonnyBurger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

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.

This branch was successfully deployed

2 active deployments
Preview – remotion — 729479d4 Deployed Oct 24, 2026 by vercel[bot]
Preview – bugs — 729479d4 Deployed Oct 24, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants