Skip to content

fix screenshot filenames with trailing / + update mp_splitext #17470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kasper93 merged 5 commits into from
Mar 3, 2026
+28 −27
Merged

fix screenshot filenames with trailing / + update mp_splitext #17470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e778514
screenshot: fix %f with trailing slashes
WhitePeter Nov 7, 2025
5f309fb
screenshot: remove redundant mkdirp call
WhitePeter Nov 7, 2025
0fe76be
misc/path_utils: fix mp_splitext bugs
WhitePeter Nov 11, 2025
6e55710
misc/path_utils: new mp_strip_ext shorthand
WhitePeter Nov 11, 2025
209a2db
screenshot: use mp_strip_ext for %F
WhitePeter Nov 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions misc/path_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,26 @@ void mp_path_strip_trailing_separator(char *path)
char *mp_splitext(const char *path, bstr *root)
{
mp_assert(path);
int skip = (*path == '.'); // skip leading dot for "hidden" unix files
const char *split = strrchr(path + skip, '.');
if (!split || !split[1] || strchr(split, '/'))
char *bn = mp_basename(path);

// Skip all leading dots, not just for "hidden" unix files, otherwise we
// end up splitting a part of the filename sans leading dot.
bn += strspn(bn, ".");

const char *split = strrchr(bn, '.');
if (!split || !split[1])
return NULL;
if (root)
*root = (bstr){(char *)path, split - path};
return (char *)split + 1;
}

char *mp_strip_ext(void *talloc_ctx, const char *s)
{
bstr root;
return mp_splitext(s, &root) ? bstrto0(talloc_ctx, root) : talloc_strdup(talloc_ctx, s);
}

bool mp_path_is_absolute(struct bstr path)
{
if (path.len && strchr(mp_path_separators, path.start[0]))
Expand Down
3 changes: 3 additions & 0 deletions misc/path_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ char *mp_basename(const char *path);
*/
char *mp_splitext(const char *path, bstr *root);

// This is a shorthand to remove the extension
char *mp_strip_ext(void *talloc_ctx, const char *s);

/* Return struct bstr referencing directory part of path, or if that
* would be empty, ".".
*/
Expand Down
4 changes: 1 addition & 3 deletions player/command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,9 +536,7 @@ static int mp_property_filename(void *ctx, struct m_property *prop,
if (strcmp(ka->key, "no-ext") == 0) {
action = ka->action;
arg = ka->arg;
bstr root;
if (mp_splitext(f, &root))
f = bstrto0(filename, root);
f = mp_strip_ext(filename, f);
}
}
int r = m_property_strdup_ro(action, arg, f);
Expand Down
31 changes: 10 additions & 21 deletions player/screenshot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,6 @@ void screenshot_init(struct MPContext *mpctx)
};
}

static char *stripext(void *talloc_ctx, const char *s)
{
const char *end = strrchr(s, '.');
if (!end)
end = s + strlen(s);
return talloc_asprintf(talloc_ctx, "%.*s", (int)(end - s), s);
}

static bool write_screenshot(struct mp_cmd_ctx *cmd, struct mp_image *img,
const char *filename, struct image_writer_opts *opts,
bool overwrite)
Expand Down Expand Up @@ -176,20 +168,20 @@ static char *create_fname(struct MPContext *mpctx, char *template,
}
case 'f':
case 'F': {
char *video_file = NULL;
if (mpctx->filename) {
if (mp_is_url(bstr0(mpctx->filename)))
video_file = mp_basename(mp_url_unescape(res, mpctx->filename));
else
video_file = mp_basename(mpctx->filename);
char *name;
if (!mpctx->filename) {
name = "NO_FILE";
} else if (bstr_endswith0(bstr0(mpctx->filename), "/")) {
name = mpctx->filename;
} else {
name = mp_basename(mpctx->filename);
}

if (!video_file)
video_file = "NO_FILE";
if (mp_is_url(bstr0(mpctx->filename)))
name = mp_url_unescape(res, name);
Copy link
Contributor

@N-R-K N-R-K Feb 27, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@guidocella guidocella Feb 27, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@guidocella guidocella Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@N-R-K N-R-K Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@guidocella guidocella Feb 27, 2026

Choose a reason for hiding this comment

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

Would be inconsistent with ${filename} though.

Copy link
Contributor

@N-R-K N-R-K Feb 27, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@guidocella guidocella Feb 27, 2026

Choose a reason for hiding this comment

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

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`'.

Copy link
Contributor

@N-R-K N-R-K Feb 27, 2026

Choose a reason for hiding this comment

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

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


char *name = video_file;
if (fmt == 'F')
name = stripext(res, video_file);
name = mp_strip_ext(res, name);
append_filename(&res, name);
break;
}
Expand Down Expand Up @@ -305,9 +297,6 @@ static char *gen_fname(struct mp_cmd_ctx *cmd, const char *file_ext)
void *t = fname;
dir = mp_get_user_path(t, ctx->mpctx->global, dir);
fname = mp_path_join(NULL, dir, fname);

mp_mkdirp(dir);

talloc_free(t);
}

Expand Down