Repository navigation
fix: derive $VIDEO_NAME from the path of URL inputs - #582
qwist1233-cpu wants to merge 2 commits into
Conversation
For a URL input, `get_file_name()` took `os.path.basename()` of the whole URL, so the query string and fragment leaked into `$VIDEO_NAME`. With `https://host/clip.mp4?token=1.2` the default `list-scenes` output became `clip.mp4?token=1-Scenes.csv`, which is an invalid file name on Windows (OSError: Invalid argument), and a URL with no path produced an empty name (`-Scenes.csv`). URLs now use the last non-empty segment of the path component, so `https://host/clip.mp4?token=1.2` -> `clip`. A URL with no path falls back to the host (plus `_port` when present) and never includes credentials. The CLI warns when no name could be derived at all. Closes Breakthrough#197
Breakthrough
left a comment
There was a problem hiding this comment.
Need to make a few minor tweaks to wording in docs/strings, but LGTM otherwise.
| if not self.video_stream.name: | ||
| logger.warning( | ||
| "Could not determine a video name from the input path, so $VIDEO_NAME will be" | ||
| " empty in output filenames. Set an explicit filename on the output command" |
There was a problem hiding this comment.
Let's remove the second sentence of this message to keep it terse.
|
|
||
|
|
||
| def get_file_name(file_path: StrPath, include_extension: bool = True) -> str: | ||
| """Return the file name that `file_path` refers to, optionally removing the extension. |
There was a problem hiding this comment.
I think this CL introduces significant enough complexity to rework this docstr entirely. Can we have a separate sub section for URL/URI-like inputs and also document what heuristic we use to detect them?
|
Both done.
One behaviour change came out of writing that: a URL with a port but no host (
|
Closes #197
What
get_file_name()now parses URL inputs ("://"in the path) withurllib.parseand uses the last non-empty path segment, ignoring the query string and fragment. A URL with no path falls back to the host (192.168.0.1_554forrtsp://192.168.0.1:554), never the credentials. The CLI logs a warning if no name could be derived at all. All three backends go through this helper, so$VIDEO_NAMEis fixed foropencv,pyavandmoviepy.Why
os.path.basename()of a whole URL keeps the query string, so$VIDEO_NAMEforhttps://host/clip.mp4?token=1.2wasclip.mp4?token=1and the defaultlist-scenesoutput name becameclip.mp4?token=1-Scenes.csv, which fails on Windows. A URL with no path gave an empty name (-Scenes.csv).How verified
tests/resources/goldeneye.mp4over a localhttp.serverand ranscenedetect -i "http://127.0.0.1:18765/goldeneye.mp4?token=1.2" -b opencv time -e 3s list-sceneson Windows. Before:OSError: [Errno 22] Invalid argument: 'goldeneye.mp4?token=1-Scenes.csv'. After:goldeneye-Scenes.csvis written.tests/test_platform.py(19 pass),ruff check/ruff format --checkclean, and the rest of the suite passes except the moviepy-only tests, which are skipped here because moviepy is not installed.I picked option (c) from the discussion on the issue: derive the name from the URL path and only warn when nothing usable is left. Happy to switch to erroring out if you prefer that.