Export frame - #46
Conversation
Introduce optional exportFrame crop in project.json so the composition canvas can differ from delivery size. Preview shows a dimmed overscan overlay with a draggable frame handle, aspect presets in the monitor, and Fast export crops JPEGs to the frame. Realtime export is disabled while a frame is set.
Move the caption inside the crop, add edge drag strips, and fix handle vertical alignment with 0.8/1/0.9 opacity states for normal, hover, and active drag.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded optional ChangesExport frame reframing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProgramMonitor
participant ProjectModel
participant ExportSetup
participant FastExport
User->>ProgramMonitor: Select or drag export frame
ProgramMonitor->>ProjectModel: Update project.exportFrame
ProjectModel->>ExportSetup: Provide active crop
ExportSetup->>FastExport: Update export engine state
FastExport->>FastExport: Encode cropped preview JPEG
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.js`:
- Around line 5984-5996: Make manual export-frame repositioning
keyboard-operable by adding keyboard movement handling alongside the pointer
logic in app.js around the export-frame drag setup, updating and persisting
project.exportFrame with accessible, predictable step behavior. In index.html
around the existing export-frame UI, add or expose a focusable framing control
with appropriate accessible labels; apply the requested changes in both app.js
(lines 5984-5996) and index.html (lines 93-100).
- Around line 5713-5718: Update the active export-frame toggle branch around
state.exportFrameView so it only hides the overlay by setting
state.exportFrameView to false. Remove the project.exportFrame = null
assignment, while preserving syncExportFrameSel, updateMonitorRes, and
scheduleSave so the crop remains applied until “Full canvas” is explicitly
selected.
In `@CLAUDE.md`:
- Around line 157-160: Update the documented exportFrame example to use an
in-bounds 9:16 rectangle, such as x=405, y=0, w=405, h=720, within the 1280×720
canvas; keep the surrounding crop and coordinate guidance unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2ae3019-37aa-4a81-908c-6cb177b97096
📒 Files selected for processing (6)
CLAUDE.mdREADME.mdapp.jsindex.htmlmcp-server.jsstyle.css
ronak-create
left a comment
ronak-create
left a comment
There was a problem hiding this comment.
Nice feature and the plumbing is thorough — normalization/clamping, persistence through projectJSON(), the MCP setProject allowlist, keyboard nudge with an aria-label. Two blocking issues before this can land.
1. Odd crop dimensions produce a 0-byte export
fitExportFrameAspect() rounds without forcing even values. On the default 1280×720 canvas, clicking the new ⬒ Frame button takes the 9:16 branch and yields 405×720.
server.js pipes the JPEGs straight into libx264 -pix_fmt yuv420p with no scale/pad filter, so:
[libx264] width not divisible by 2 (405x720)
out.mp4 = 0 bytes
Reproduced against ffmpeg 8.0.1 with the exact pipeline the export uses (-f image2pipe -framerate 30 -i - -c:v libx264 -pix_fmt yuv420p). 608×1080 (9:16 on a 1920×1080 canvas) encodes fine, so it is specifically the default button on the default project size that breaks — and w: 405 is the value used in this PR's own CLAUDE.md example.
Either round to even in fitExportFrameAspect() and normalizeExportFrame() (w &= ~1), or add -vf "pad=ceil(iw/2)*2:ceil(ih/2)*2" in beginExport(). The client-side fix is probably better since the frame dimensions are user-visible in the monitor readout.
2. No ffmpeg + export frame set → silently uncropped output
openExportSetup() sets els.engineRealtime.disabled = true whenever a frame is set, but startChosenExport() only tests engineFast:
if (els.engineFast.checked && !els.engineFast.disabled) fastExport();
else startExport();So when ffmpeg is unavailable and an export frame is set, both radios are disabled, yet the else branch still runs the realtime export — producing a full-canvas video with no crop. That contradicts the README line added here ("Realtime export is disabled while a frame is set"). Options: have startChosenExport() bail with a message when the chosen engine is disabled, or block setting an export frame at all when ffmpeg is missing.
Notes, not blockers
- Merge conflict with #50, which removes the
#monitorResspan this PR extends viaupdateMonitorRes(). Whichever lands second needs a rebase; if #50 goes first,updateMonitorRes()will throw on a null element duringapplyProject(). - The
box-shadow: 0 0 0 9999pxdim is clipped by.monitor-stage { overflow: hidden }, so no spill — that part is fine. - The frame can be moved but not resized. Fine for a first pass given the aspect presets, just noting it in case it was meant to be resizable.
|
Heads up — I merged #47 and #50, so this branch now needs a rebase on #50 removed the Worth deciding rather than mechanically resolving — after #50 the monitor header shows the aspect select (which already carries the dimensions) plus the FPS select, and there is no free-text readout left to hang
Your call — I do not have a strong preference, just flagging that The two blocking issues from my earlier review still stand independently of the rebase. |
2 participants
What does this PR do?
This is a proposal, as rendering is affected (the one called "realtime" - it is disabled while frame is active).
Quite often the footage is, for example, horizontal, but a vertical reel must be produced. Abstracting out the video reframing done externally, for the manual process now one can set a project to 16x9, with a cropping frame 9x16. On ffmpeg export, this will be preserved. While making a composition (by a human), it is quite easy to manualy frame the source horizontal video, as it is visible, but dimmed ouside the frame area.
Type of change
How was it verified?
node --check server.js && node --check app.js && node --check mcp-server.jspassesCLAUDE.md/README.mdif the schema, props, or API changedChecklist
Summary by CodeRabbit