Skip to content

Adds the ability to quickly crop, with presets for popular aspect ratios, while adding no UI clutter - #4

Open
Critters wants to merge 1 commit into
omacom:masterfrom
Critters:crop
Open

Critters wants to merge 1 commit into
omacom:masterfrom
Critters:crop

Conversation

@Critters

@Critters Critters commented Aug 28, 2026

Copy link
Copy Markdown
omacut-crop.mp4

Click-drag on the video to draw a crop. Corner handles resize; drag inside the box to move it; Esc clears.

  • P / L / S — portrait 9:16, landscape 16:9, or square, centered and as large as it fits. Corner drags then keep that aspect.
  • Shift+corner — scale from the center
  • Applied only on export, together with the trim

@evilbuck

evilbuck commented Sep 2, 2026

Copy link
Copy Markdown

I came here to fork and create this feature. Thank you.
Hope it get's in master soon

@Critters

Critters commented Sep 3, 2026

Copy link
Copy Markdown
Author

I came here to fork and create this feature. Thank you. Hope it get's in master soon

Thanks, my first contribution to someone else's OS project! Have a play with it and let me know if you'd have done anything differently or if it gives you all you need and was intuitive to use. I was mindful of not adding to the UI as I don't think anyone would want this tool to slowly "degrade" into much beyond something to quickly trim (and hopefully crop) recordings.

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

solid overlay + export integration; tests cover crop-before-scale and quality sizing.

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed by Claude Opus 5 in Claude Code, with an independent second opinion from Codex at xhigh reasoning. Built and ran the full suite on a disposable Omarchy VM under a real Wayland session: 39 passed, 0 failed, including all twelve new crop tests. The overlay itself was exercised against the running app on that VM, since a drawn rectangle cannot be judged on a machine with no compositor.

Whether omacut should grow a crop feature at all is the maintainer's call and nothing below touches it. This is only about whether the code does what it looks like it does.

On an ordinary square-pixel, unrotated source it does. P, L and S land exactly where the arithmetic says, the filters compose in the right order (crop=...,scale=...), and with no crop set the ffmpeg argument list is byte-for-byte what master produced, for both Original and a downscale. clampCrop is sound in its own terms: neither qBound has inverted bounds, all four returned numbers are even, and x+w/y+h stay inside the frame it was handed.

The frame it is handed is the wrong one for any video carrying a rotation display matrix, which is most phone footage. probe() (src/ffmpeg.cpp:68) reads the coded width/height out of ffprobe, and for a portrait phone video those are the landscape numbers. Qt's VideoOutput applies the display matrix, so contentRect is the portrait picture, and sourceRect (src/CropOverlay.qml:39-46) multiplies fractions of that portrait picture by the landscape numbers. ffmpeg inserts its autorotation ahead of -vf, so the crop= filter runs against the portrait frame as well. Both failure modes were reproduced on the VM against a real file (1280x720 coded, tagged -display_rotation 90, so 720x1280 displayed):

  • L on a rotated video fails the export. The landscape preset selects the full frame, so the crop is crop=1280:720:0:0 against a 720x1280 picture and ffmpeg refuses it — Invalid too big or non positive size for width '1280' or height '720', exit 234. The app shows that raw ffmpeg string as the export error.
  • P on a rotated video silently exports a different region than the one drawn. The overlay draws a full-height strip down the centre of the portrait picture; the crop emitted is crop=404:720:438:0, and against a 720x1280 picture ffmpeg clamps x to 316 and takes only the top 720 rows. What lands on disk is a right-hand slice of the top 56% of the video. It exits 0, so nothing says otherwise.

The same root cause makes exportDialog (src/backend.cpp:209-211) choose the quality options from the wrong rectangle on those sources.

The fix is contained — probe() can read side_data_list for the Display Matrix rotation and swap width and height for ±90/±270, and both consumers of VideoInfo::width/height today (exportHeights and the new crop path) want the displayed frame, so nothing else moves. It has not been pushed to the branch because it only closes one of the two axes, and the other one is a decision rather than a defect:

The aspect presets are storage-pixel presets, not display-aspect presets. applyPreset (src/CropOverlay.qml:90) computes 9:16, 16:9 and 1:1 from coded width and height with no sample aspect ratio, and ffmpeg's crop carries the input SAR through to its output. Verified on the VM: on a 720x576 SAR 16:15 source (displayed 768x576), S produces a 576x576 file that still carries SAR 16:15 and therefore displays at 16:15, not square. On the same source the 9:16 preset exports at 3:5 and 16:9 at 256:135. Freehand cropping is unaffected, since a normalised rect maps proportionally onto the storage grid either way; it is specifically the preset and locked-aspect arithmetic that lives in storage space. Whether "square" ought to mean a square picture or a square pixel grid is a product question, so it is left here rather than patched.

P, L and S fire while the help overlay is open (src/Main.qml:249-268). The three shortcuts check quitConfirmVisible but not helpVisible, so pressing ? then S installs a square crop under the shortcuts panel; closing help reveals a crop nobody asked for, and the window then counts as having unexported work. Screenshotted on the VM.

One lower-severity path, reasoned rather than executed: commitDraw (src/CropOverlay.qml:213) enforces a minimum of eight display pixels rather than source pixels, so on a small source scaled up in the window a deliberate-looking drag can round to a 1x1 sourceRect, which clampCrop collapses and exportClip then rejects with "The crop has no area" while the box stays drawn on screen.

Two things that are not defects but worth naming. The MouseArea gaining visible: !win.hasVideo (src/Main.qml:539) removes master's click-the-preview-to-reopen-the-chooser behaviour — unavoidable if a click is going to start a crop, and Ctrl+O still works, but it is a behaviour change the description does not mention. And this collides with #8: both add a different trailing parameter to the same six signatures — Backend::exportDialog, Backend::exportClip, FilePicker::exportVideo, FilePicker::exportSelected, PortalFilePicker::exportVideo and ffmpeg::trimArgs — and touch the same lines of Main.qml and README.md, so whichever lands second needs a rebase. It is more than textual: -c:v copy cannot take a -vf crop=, so a merged version has to refuse stream copy while a crop is set, or force the re-encode, otherwise Ctrl+Shift+S with a crop drawn would quietly write the uncropped frame.

Where codex agreed with conclusions already reached — the rotation defect, clampCrop being internally sound, the unchanged no-crop argument list — its independence is not currently guaranteed, so read that as agreement rather than confirmation. The SAR and help-overlay findings it contributed on its own; both were then verified on the VM before being written down. The existing approval on this pull request is from another contributor rather than a maintainer and does not carry merge authority.

Waiting on the author for the rotation handling and for what the presets should mean on non-square-pixel sources, and on the maintainer for whether the feature lands and in what order relative to #8.

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