Skip to content

fix: derive $VIDEO_NAME from the path of URL inputs - #582

Open
qwist1233-cpu wants to merge 2 commits into
Breakthrough:mainfrom
qwist1233-cpu:fix/video-name-url-inputs
Open

qwist1233-cpu wants to merge 2 commits into
Breakthrough:mainfrom
qwist1233-cpu:fix/video-name-url-inputs

Conversation

@qwist1233-cpu

Copy link
Copy Markdown

Closes #197

What
get_file_name() now parses URL inputs ("://" in the path) with urllib.parse and 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_554 for rtsp://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_NAME is fixed for opencv, pyav and moviepy.

Why
os.path.basename() of a whole URL keeps the query string, so $VIDEO_NAME for https://host/clip.mp4?token=1.2 was clip.mp4?token=1 and the default list-scenes output name became clip.mp4?token=1-Scenes.csv, which fails on Windows. A URL with no path gave an empty name (-Scenes.csv).

How verified

  • Served tests/resources/goldeneye.mp4 over a local http.server and ran scenedetect -i "http://127.0.0.1:18765/goldeneye.mp4?token=1.2" -b opencv time -e 3s list-scenes on Windows. Before: OSError: [Errno 22] Invalid argument: 'goldeneye.mp4?token=1-Scenes.csv'. After: goldeneye-Scenes.csv is written.
  • New parametrized tests in tests/test_platform.py (19 pass), ruff check / ruff format --check clean, 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.

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 Breakthrough left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Need to make a few minor tweaks to wording in docs/strings, but LGTM otherwise.

Comment thread scenedetect/_cli/context.py Outdated
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"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's remove the second sentence of this message to keep it terse.

Comment thread scenedetect/platform.py


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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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?

@Breakthrough Breakthrough added this to the 0.7.3 milestone Oct 20, 2026
@qwist1233-cpu

Copy link
Copy Markdown
Author

Both done.

  • The warning is now one sentence; the "set an explicit filename" hint is gone.
  • get_file_name has a dedicated URL and URI-like inputs section. It names the heuristic (anything containing ://, i.e. is_url) and why URLs are split by component instead of by path separator, then covers the two cases separately: last non-empty path segment (percent-decoded), and the host fallback when there is no path. It also states the two things that were implicit before — userinfo is never used, and include_extension=False does not strip anything off a host, since the last . there is a domain separator. Arguments:/Returns: sections match the rest of the module.

One behaviour change came out of writing that: a URL with a port but no host (rtsp://:554) used to return _554. It now returns an empty string, which is what the docstring promises for "no usable name". Two cases added to test_get_file_name_without_extension for it.

pytest tests/test_platform.py passes (21 tests), ruff check and ruff format --check clean on the three changed files.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correctly calculate $VIDEO_NAME macro for URL inputs

2 participants