Skip to content

@remotion/media: Add effect output size to <Video> - #11436

Closed
alec-watts wants to merge 4 commits into
remotion-dev:mainfrom
alec-watts:alec/effects-output-size-current-main
Closed

alec-watts wants to merge 4 commits into
remotion-dev:mainfrom
alec-watts:alec/effects-output-size-current-main

Conversation

@alec-watts

@alec-watts alec-watts commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Superseded by #12178, which implements the maintainer-requested effectsOutputSize API across all effect-capable components and documents it on the shared effects page.

This closed PR's recorded diff is the earlier Video-only implementation. GitHub rejected reopening it after the branch was updated, so the revised work is published on a new feature branch in #12178. That replacement includes current main, native multi-component render/export verification, WebGL orientation checks, paused-preview resizing without decoder restart, and cancellation/resource cleanup coverage.

dukeeagle and others added 4 commits September 17, 2026 10:25
Avoid redraws from inline size objects and make internal absence explicit with nullable parameters.

Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the nullable default on the before composition so the comparison remains accurate.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel

vercel Bot commented Sep 17, 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 Sep 17, 2026 5:45pm UTC
remotion Ready Ready Preview Sep 17, 2026 5:45pm 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 minor simplification inline.

Reviewed changes

  • New effectsOutputSize? prop — <Video> accepts {width, height}; the public prop is optional, while the internal inputs (InnerVideoProps, MediaPlayer, videoIteratorManager, VideoForRendering) are required and nullable, matching the repo's convention.
  • resolveEffectsOutputSize() — resolves the effect canvas size from the prop, defaulting to the source frame dimensions, rounding to whole pixels, and throwing on non-finite / non-positive values.
  • Effect canvas is now sized to the resolved output — paintFrame resizes the preview canvas and passes the resolved size to runEffectChain; the rendering extraction callback resizes the canvas and runs the chain at that size. When effects is empty both paths keep the frame dimensions, so the prop is inert as documented.
  • Preview redraw wiring — getEffectsOutputSize is read through a ref, and the redraw effect depends on effectsOutputSize?.width/height so changing the prop repaints the retained frame.
  • Docs, example, tests — new docs section with before/after images, an EffectsTestbed composition, a unit test for resolveEffectsOutputSize, a preview browser test, and a web-renderer test asserting effect apply receives the requested size. All MediaPlayer / videoIteratorManager call sites (including the CI-failing premount-arm-kicks-scheduler fixture) were updated.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash | 𝕏

Comment on lines +333 to +340
effectsOutputSize:
effectsOutputSize?.width === undefined ||
effectsOutputSize?.height === undefined
? null
: {
width: effectsOutputSize.width,
height: effectsOutputSize.height,
},

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.

This conditional is unreachable for typed callers: effectsOutputSize is declared EffectsOutputSize | null with required width/height, so it is either null or a complete object. Passing it straight through is equivalent.

Worth noting it also creates a split behavior: a malformed JS value like {width: 100} is silently treated as omitted here, while the preview path (videoIteratorManager → resolveEffectsOutputSize) throws for the same input. Simplifying here keeps the two paths aligned (both validate via resolveEffectsOutputSize).

Suggested change
effectsOutputSize:
effectsOutputSize?.width === undefined ||
effectsOutputSize?.height === undefined
? null
: {
width: effectsOutputSize.width,
height: effectsOutputSize.height,
},
effectsOutputSize,

@alec-watts

Copy link
Copy Markdown
Contributor Author

Validation update: all build matrices, lint/formatting, monorepo tests, web-renderer tests, Lambda integration, SSR, template integration and both Vercel deployments passed.

The remaining failure is Browser tests, specifically the existing multi-sequence-inspector undo workflow (source did not return to sourceBefore after Cmd+Z). This test does not exercise Video/effects. I ran the entire 17-test example E2E suite on both pristine base 46a3a6bf13ca367376ff18bbb06dae2e87e0d068 and this feature branch: both passed locally. The 140 media browser tests also passed locally.

I attempted a failed-job rerun, but GitHub rejected it with “Must have admin rights to Repository.” A maintainer needs to rerun this apparent CI-only flake; Required CI remains red until then. I have not changed the test or CI configuration to conceal it.

@JonnyBurger: this is non-draft and includes the original review fixes. The formal reviewer request was also denied by repository permissions.

@JonnyBurger

Copy link
Copy Markdown
Member

@alec-watts This seems like a reasonable option, but I think this should not be <Video>-specific.
All components who support effects can benefit from this, and then ideally it is also documented on the effects page.

Would you be willing to do these other parts as well?

@JonnyBurger

Copy link
Copy Markdown
Member

I don't think this should (only) live on <Video>. If we do need this, then it should work for all components accepting effects.

@alec-watts alec-watts changed the title @remotion/media: Add effect output size to <Video> remotion: Add effectsOutputSize to all effect-capable components Oct 9, 2026
@alec-watts alec-watts changed the title remotion: Add effectsOutputSize to all effect-capable components @remotion/media: Add effect output size to <Video> Oct 9, 2026

This branch was successfully deployed

2 active deployments
Preview – remotion — aed50d37 Deployed Sep 17, 2026 by vercel[bot]
Preview – bugs — aed50d37 Deployed Sep 17, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants