Skip to content

Trim a video straight from a link - #5

Open
Zedster07 wants to merge 1 commit into
omacom:masterfrom
Zedster07:paste-a-link
Open

Zedster07 wants to merge 1 commit into
omacom:masterfrom
Zedster07:paste-a-link

Conversation

@Zedster07

Copy link
Copy Markdown

Trimming something you found online is a two-step dance today: download it yourself, find where it landed, then open it in omacut. This makes it one step.

Ctrl+V hands whatever link is on the clipboard to yt-dlp, which fetches the video into ~/.cache/omacut/downloads. The moment it lands it goes through the existing load() path, so the filmstrip, zoom, preview, trim edges and export all behave exactly as they do for a local file — because by then it is one.

Why download rather than stream

Trimming means scrubbing, and scrubbing a remote stream means a dozen network seeks just to build the filmstrip, URLs that expire mid-edit, and the separate video/audio streams MediaPlayer won't take. Fetching the file first leaves every existing behaviour untouched and keeps the change small — the new code stops at the point where a file exists, and the app it hands off to is the one that was already there.

Choices worth flagging

  • Capped at 1080p, h264 preferred. Trimming is a length edit, not a mastering job. h264 is what exports are re-encoded to anyway, and sites increasingly serve AV1, which plenty of machines can only decode in software — enough to make scrubbing stutter. It's a -S sort rather than an -f filter, so an AV1-only source still downloads.
  • Downloads live in the cache, so an export of one is suggested among your videos rather than in ~/.cache. A file you opened yourself still exports next to itself, as before.
  • yt-dlp is optional — an optdepends in the PKGBUILD. Without it everything else works, and pasting a link says exactly what's missing.
  • No new UI, beyond one line of hint text on the empty state and one row in the ? overlay. The download reports itself through the existing status line, which now also shows before a video is loaded, since that's when a download happens.
  • omacut <url> works on the command line too, alongside omacut <file>.

Testing

./bin/test — 32 passing. Five new cases cover link detection, the argument list, progress parsing, error extraction and the export-path change; Ctrl+V is added to the existing shortcut test. All of it is pure functions and QML wiring, so no test touches the network.

Verified by hand on Omarchy end to end: paste a link, watch the percentage in the status line, video opens with its filmstrip, trim it, export it.

Ctrl+V hands whatever link is on the clipboard to yt-dlp, downloads it into
the cache, and opens it for trimming as soon as it lands — the same filmstrip,
preview and export as any other file, because by then it is one.

Links come down at up to 1080p and prefer h264, which is what exports are
re-encoded to anyway and what stays smooth while scrubbing. The finished path
comes back from yt-dlp itself, so pasting a link already in the cache reopens
it instantly. A downloaded source is a cache entry rather than a file the user
keeps, so its export is suggested among their videos instead of beside it.

yt-dlp is only needed to open links; nothing else about omacut requires it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016rcJ8LUydunVeEgaT9hHDr
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at f5ab461, by Claude Opus 5 and independently by Codex at xhigh reasoning. Everything below was executed on a disposable Omarchy worker VM, never on the machine doing the review.

What ran. ./bin/test builds clean and passes 32/32, including the five new cases. The feature was then proved end to end on a real compositor rather than argued about: with a local HTTP server standing in for a site, omacut http://.../Sample%20Clip.mp4 fetched into ~/.cache/omacut/downloads, showed Downloading 23% in the status line over the empty state, and opened the result with its filmstrip, playhead and trim bar. The empty-state hint sits correctly under the button, and with yt-dlp removed from PATH the status line reads Cannot open video: \yt-dlp` was not found on your PATH. Install yt-dlp to open links.`

Command construction is clean, which was the main thing worth checking. The URL reaches yt-dlp as one element of a QStringList at src/ytdlp.cpp:68, started with proc->start(tool, ...) at src/backend.cpp:372 — an argv array, no shell, no sh -c, no startCommand() anywhere in the repo. Option injection into yt-dlp's own parser is closed by ytdlp::isLink: compiling it and running 30 adversarial inputs through it, -x, --exec=..., --config-location=..., file:///etc/passwd, ftp://, javascript:, data:, //host/x, http:// with no host, embedded newline, CR and tab, a leading zero-width space, and anything with a backtick or a quote are all rejected, and nothing accepted can begin with -, so the missing -- separator costs nothing here. The output template is confined too: a page whose <title> was ../../../../../../tmp/omacut-pwned produced ..⧸..⧸..⧸..⧸..⧸..⧸tmp⧸omacut-pwned [page-1].mp4 inside the download directory, and a title of a;touch /tmp/omacut-shell-pwned;b \id` $(id)created a file with that name and ran nothing. A killed download leaves a.part` and no marker file, so a partial is never probed as a real source.

Four things did come up.

1. Concurrent instances share one marker file, and one set of filenames (medium). src/backend.cpp:309 always uses downloads/.omacut-filepath, removes it, and src/backend.cpp:351 reads back its last line. Two omacut processes downloading at once can have one read the other's path and open the wrong video, or have the marker removed between a write and the read, which reports yt-dlp did not report a downloaded file for a download that actually succeeded. This is more reachable than it sounds, because Ctrl+V is disabled while backend.busy, so a second window is the natural way to queue a second link. Codex added the half I had missed: two instances on the same URL also derive identical .part and final filenames from the template at src/ytdlp.cpp:50, so the media file races too — which is why a per-process marker name alone would not be the whole fix, and why this is left for you rather than patched onto the branch. A staging directory per run would fix both but would give up the "pasting the same link again reuses what's already there" behaviour the README promises, so it is a design call.

2. A multi_video link can exit zero and then fail to open (medium). --no-playlist does not cover yt-dlp's separate multi_video result type — its own docs say so at YoutubeDL.py:387 ("playlists (not multi_video)") — and --concat-playlist defaults to multi_video at options.py:1746. For such a link each entry prints its own after_move:filepath line, then FFmpegConcatPP.concat_files writes the joined file to the pl_video path and returns the entry files as files_to_delete (postprocessor/ffmpeg.py:1141-1181); the concatenated path is never printed through after_move. So the last line of the marker names a file yt-dlp has just deleted, and loadPath probes it and reports a load failure on a download that worked. The single-entry case does it too, via the os.replace branch. Mechanism read out of the installed yt-dlp 2026.08.19; not reproduced end to end, since a multi_video source is awkward to fake locally.

3. Cancelling a download does not stop yt-dlp's children (low). QProcess::kill() at src/backend.cpp:384 signals yt-dlp only, not its process tree, and yt-dlp forks ffmpeg to do the merge this PR asks for with --merge-output-format mp4. Verified on the worker that a SIGKILL to a parent leaves the grandchild alive and reparented to PID 1. Opening a file by hand or quitting mid-merge therefore leaves an ffmpeg running and writing into the cache after the app considers the fetch abandoned. Litter rather than breakage, but it is real.

4. The 1080p cap is a preference, not a cap (low). bv*[height<=1080]+ba/b[height<=1080]/b at src/ytdlp.cpp:54 ends in an unfiltered /b, and slash alternatives are fallbacks — so when nothing satisfies the filter the last branch takes the best format at any resolution. Demonstrated both ways on the worker: yt-dlp's own selector picks the 2160p format when that is the only one, and running the exact command line above against a 3840x2160 source downloaded it at 3840x2160. It also fires on the common case rather than an exotic one, because height<=1080 is false when height is unknown, which is most direct-link sources. This may well be deliberate degradation — better a big download than none — but the README's "capped at 1080p" and the comment at src/ytdlp.cpp:26 both read as a guarantee, so either the wording or the selector wants to move. Codex found this one; it was not in my own pass.

Nothing was pushed to the branch: all four are calls about how the feature should behave rather than contained defects, and a partial fix for the first would be worse than the current state.

Collisions. #6, by the same author, merges cleanly with this. #3 does too. #4 conflicts in src/backend.cpp and tests/backend_tests.cpp, and not only textually — it rewrites the same Backend::suggestedExportUrl() this PR replaces with suggestedExportUrlFor, and changes the exportDialog/exportClip signatures, so whichever lands second has to reconcile the export-path suggestion by hand. #8 conflicts only in tests/backend_tests.cpp, in the private slots: list, which is mechanical.

Waiting on the author for the four above, and on the maintainer for the ordering against #4.

Where Codex agreed with what had already been concluded — the shell-safety and option-injection analysis, the marker race — its independence is not currently guaranteed, since its read-only sandbox restricts writes rather than reads. Findings 2 and 4, and the filename-collision half of finding 1, are things it raised that this review had not reasoned about, and those stand on their own; each was verified against yt-dlp's source and behaviour before being written down here.

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.

2 participants