fix screenshot filenames with trailing / + update mp_splitext by guidocella · Pull Request #17470 · mpv-player/mpv

guidocella · 2026-02-27T11:31:59Z

Updated version of #17021

WhitePeter's commits have the following modifications:

Rebased to unescape %f and %F URLs
Renamed mp_stripext mp_strip_ext (I will rename mp_splitext in a future PR, also making it static and the backend for mp_strip_ext and a new mp_get_ext)
Made mp_strip_ext always allocate a new string as per N-R-K's suggestion

I did not make mp_basename_or_url itself unescape URLs because

that requires making it allocate memory
${filename} already unescapes URLs on its own
mp_basename_or_url is useful in configfiles.c which probably shouldn't unescape URLs for watch later files

N-R-K · 2026-02-27T12:48:53Z

player/screenshot.c

-                video_file = "NO_FILE";
+            char *name = mpctx->filename ? mp_basename_or_url(mpctx->filename) : "NO_FILE";
+            if (mp_is_url(bstr0(name)))
+                name = mp_url_unescape(res, name);


I do not agree with this change at all. A good amount of urls will contain a useable filename at the end, e.g example.com/video_name.mp4.

Just because the handling of urls with trailing slashes was incorrect doesn't mean we should make things worse for the average case which don't have this problem.

Most played URL are ytdl_hook URLs which don't have enough information in the basename.

I guess it could be conditional on user-data/mpv/ytdl/json-subprocess-result being set?

Edit: or the last segment containing an extension/.?

I guess it could be conditional on user-data/mpv/ytdl/json-subprocess-result being set?

Edit: or the last segment containing an extension/.?

Or keep it as is and handle the off-case when there's a trailing slash (i.e basename returned empty string).

Would be inconsistent with ${filename} though.

Most played URL are ytdl_hook URLs which don't have enough information in the basename.

Even in the case of youtube, I'd much rather see watch?v=whatever.png as the filename (which already tells me it was a youtube video) than https:__www.youtube.com_watch?v=whatever.png. The latter is just useless noise being added for no reason.

If there's enough people who prefer the whole url, then we can consider adding it as some kind of option. But making it noisy under the guise of fixing a bug is something I consider a feature regression.

Would be inconsistent with ${filename} though.

True. Does it matter though?

Well I suggested deprecating the formatters in favor of properties in #17021 (comment). And #10975 requested URL domains and paths in properties and track lines. So this gets messy, but whatever, updated to not take the basename only of paths ending with '/for%f`'.

The latest change addresses my concern, but the commit message no longer correctly describes what the commit does.

N-R-K · 2026-02-27T16:28:42Z

DOCS/man/input.rst

-    unmodified filename.)
+    Currently played file, with path stripped, except URLs. The latter will be
+    returned in full with percent encoding undone. (The result is not
+    necessarily correct, but looks better for display purposes. Use the


What's "not necessarily correct" here? I'd assume this was referring to taking basename of urls, but since that's no longer being done, does this still apply?

N-R-K · 2026-02-27T16:33:11Z

misc/path_utils.h


+/* Return pointer to last path segment or the original argument if it is a URL
+ */
+#define mp_basename_or_url(p) mp_is_url(bstr0(p)) ? (char *)p : mp_basename(p)


Wrap this in a paren just in case.

Suggested change

#define mp_basename_or_url(p) mp_is_url(bstr0(p)) ? (char *)p : mp_basename(p)

#define mp_basename_or_url(p) (mp_is_url(bstr0(p)) ? (char *)p : mp_basename(p))

Or better yet, just use a function which avoids precedence and/or double evaluation issue altogether. I don't really see any good reason for this to be a macro.

guidocella · 2026-02-27T16:54:31Z

Also applied the strspn suggestion from #17021 (comment)

na-na-hi · 2026-02-28T04:42:28Z

player/command.c

    if (mp_is_url(bstr0(filename)))
        mp_url_unescape_inplace(filename);
-    char *f = (char *)mp_basename(filename);
+    char *f = mp_basename_or_url(filename);


This should only be done for URLs with trailing slash to keep consistency with %f.

I already noted that it's inconsistent. But then #10975 would not be fixed.

It is documented that %{filename} is the same as %f.

But then #10975 would not be fixed.

OSC and terminal display can be fixed separately without changing ${filename} behavior.

The OSC doesn't even use ${filename} but ${media-title} specified in the configurable title script-opt. How would you change that? Also the OSC is just an example reported here, e.g. the filename in stats has the same issue.

URL display would also still be very inconsistent since show-text ${playlist} and the playlist and history menus already show full URLs.

The inconsistency between %f and ${filename} is also bad for not allowing to deprecate the formatters. But even a new ${filename} sub-property wouldn't fix ${media-title}...

The OSC doesn't even use ${filename} but ${media-title} specified in the configurable title script-opt. How would you change that?
But even a new ${filename} sub-property wouldn't fix ${media-title}...

Let ${media-title} use the new sub-property that keeps full url when there is no title. It can also do other processing on its own.

URL display would also still be very inconsistent since show-text ${playlist} and the playlist and history menus already show full URLs.

They are not relevant to the topic here and these URLs also do not undo percent encoding. All of these were implemented after ${filename} and not designed for consistency to other properties.

The inconsistency between %f and ${filename} is also bad for not allowing to deprecate the formatters.

I would not deprecate the existing formatters and I am not seeing a consensus that they should be deprecated.

Also the initial implementation of ${filename} was 7ccf483 where a filename ending with a slash will have the full name displayed. In my opinion this fix should be done regardless of the existence of %f.

guidocella · 2026-02-28T08:14:07Z

I'm not sure if mp_basename_or_url is even worth it if we need to check if the filename is a URL anyway to decode it?

It may or may not not be used in these places:

%f and %F screenshot formatters, must take the basename of URLs (fix screenshot filenames with trailing / + update mp_splitext #17470 (comment)), must decode URLs (ideally would deprecated in favor ${filename})
${filename}, must not take the basename of URLs (Display long URLs in a user-friendly and informative way #10975), must decode URLs
show-text ${playlist}, must not take the basename of URLs, must decode URLs (would be updated by misc/path_utils: make mp_basename() return full URLs #17022)
configfiles.c / watch later files with --ignore-path-in-watch-later-config, must not take the basename of URLs, must not decode URLs because the undecoded URLs in the playlist are compared for resuming playback (would be updated by misc/path_utils: make mp_basename() return full URLs #17022) (now that I think about it, there could even be normalization to use the same watch later file whether you play the same URL encoded or not)
terminal printing of track URLs (c94e8bcdd6), must not take the basename of URLs (Display long URLs in a user-friendly and informative way #10975), must decode URLs

The playlist and history menus already takes the basename of non-URLs only.

guidocella · 2026-03-02T10:42:54Z

Just removed mp_basename_or_url and ${filename} modifications for now.

URLs can end with trailing slashes (/) which in turn results in GNU basename[1] returning the empty string, i.e. when the filename property is queried. Instead use the whole path for everything that ends with /. Subsequent path sanitation takes care of translating invalid path component chars. Issue reproduction steps: ``` mpv \ --screenshot-dir=$HOME/mpv-shots \ --screenshot-template='%f/%P' \ https://example.org/video/ ``` This would result in %f expanding to '' and thus render screenshot-template an absolute path which, for some reason, would in turn take precedence of screenshot-dir and hence result in a non-writeable path, i.e. `/timestamp.ext`. With this new approach the resulting path looks like this: `/home/user/mpv-shots/http:__example.org_video_/timestamp.ext` [1] mpv-player#14635 (comment) Co-authored-by: Guido Cella <[email protected]>

There is no reason to create the screenshot dir separately, because the final path starts with it. So defer creating directories to the last possible moment with all the information necessary.

The detection for leading dots was in the wrong place, in case path was not already a basename. Fixed by acting on basename. Also all leading dots are now getting skipped, otherwise the root of `path/to/...ext` could end up as `path/to/..` which in the wrong place/context would mean directory traversal. Additionally, since we act on the basename there is no longer a need to test for trailing path separators. The previous check also did not account for Windows paths with \ separators. Co-authored-by: Guido Cella <[email protected]>

There are at least two users of this functionality, the filename/no-ext property and the screenshot template %F specifier. The latter uses a buggy private version. So split out what the former does, so it can be reused. Co-authored-by: Guido Cella <[email protected]>

Reuse existing functionality, see previous commit introducing above function.

N-R-K

LGTM.

github-actions · 2026-03-03T02:00:57Z

Download the artifacts for this pull request:

Windows

macOS

guidocella force-pushed the basename-or-url branch from e39ee9d to d4be69b Compare February 27, 2026 11:34

N-R-K suggested changes Feb 27, 2026

View reviewed changes

guidocella force-pushed the basename-or-url branch from d4be69b to ca9fddd Compare February 27, 2026 16:16

N-R-K suggested changes Feb 27, 2026

View reviewed changes

guidocella force-pushed the basename-or-url branch 2 times, most recently from ed68100 to 6663f7f Compare February 27, 2026 16:54

na-na-hi reviewed Feb 28, 2026

View reviewed changes

guidocella changed the title ~~various: add basename_or_url macro~~ fix screenshot filenames with trailing / + update mp_splitext Mar 2, 2026

guidocella force-pushed the basename-or-url branch from 6663f7f to b401941 Compare March 2, 2026 10:42

WhitePeter and others added 2 commits March 2, 2026 12:00

screenshot: remove redundant mkdirp call

5f309fb

There is no reason to create the screenshot dir separately, because the final path starts with it. So defer creating directories to the last possible moment with all the information necessary.

guidocella force-pushed the basename-or-url branch from b401941 to 49cfecc Compare March 2, 2026 11:02

WhitePeter and others added 3 commits March 2, 2026 12:04

screenshot: use mp_strip_ext for %F

209a2db

Reuse existing functionality, see previous commit introducing above function.

guidocella force-pushed the basename-or-url branch from 49cfecc to 209a2db Compare March 2, 2026 11:04

N-R-K approved these changes Mar 2, 2026

View reviewed changes

kasper93 requested a review from na-na-hi March 2, 2026 23:03

na-na-hi approved these changes Mar 3, 2026

View reviewed changes

kasper93 merged commit c401ef9 into mpv-player:master Mar 3, 2026
48 of 51 checks passed

kasper93 mentioned this pull request Mar 3, 2026

screenshot: fix %f corner case #17021

Closed

	#define mp_basename_or_url(p) mp_is_url(bstr0(p)) ? (char *)p : mp_basename(p)
	#define mp_basename_or_url(p) (mp_is_url(bstr0(p)) ? (char *)p : mp_basename(p))

Conversation

guidocella commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidocella Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-R-K Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidocella commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

na-na-hi Feb 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

na-na-hi Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidocella commented Feb 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guidocella commented Mar 2, 2026

Uh oh!

N-R-K left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

guidocella Feb 27, 2026 •

edited

Loading

N-R-K Feb 27, 2026 •

edited

Loading

na-na-hi Feb 28, 2026 •

edited

Loading

na-na-hi Mar 1, 2026 •

edited

Loading

guidocella commented Feb 28, 2026 •

edited

Loading