path_utils: strip trailing path separators

[WhitePeter]

Previous behavior results in a '\0' (nul char) being returned if 'filename' is a URL that ends with a '/', which in turn would lead to 'screenshot-dir' being ignored or overwritten, which then results screenshot-path effecitively starting with '/' in case of nesting screenshot dirs per file, e.g.:

$ mpv \
	--screenshot-dir='~/tmp/mpv-shots' \
	--screenshot-template='%f/%P' \
	https://example.org/video/

This happens with ytdl-hook on some sites, in case it matters. Since %f expands to '' and somehow mpv also forgets about screenshot-dir in the process, the effective path template now is '/%P', which is most certainly not writable for ordinary users, on *nix at least.

This strips trailing path separators in mp_basename before doing the reverse search for the first '/' which otherwise returns the very last index of the string + 1 which in turn points to the terminating '\0'.

Read this before you submit this pull request:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md

I have read the aforementioned document.

[guidocella]

mpctx->filename is the full path. You are confusing it with the filename property. Unfortunately paths are called filename in some places.

[WhitePeter]

mpctx->filename is the full path. You are confusing it with the filename property. Unfortunately paths are called filename in some places.

Thanks for clearing that up(*). I think I have found the culprit in https://github.com/mpv-player/mpv/blob/28ef6b780c227507e248890b18397da21027d935/misc/path_utils.c#L56C1-L56C28. Interestingly enough there is mp_path_strip_trailing_separator in the same file but it is not called in mp_basename, but it should. But then the compiler complains about discarding the const qualifier of path. So, is it a hard requirement for path to be a const? And if so, what if I then call mp_path_strip_trailing_separator((char *)path)? It does fix both issues I mentioned above, the filename property now reflects the basename of said URLs and %f returns the same result.

(*) Are you absolutely sure that mpctx->filename != p.filename? Because after stripping trailing slashes inside mp_basename, mpctx->filename is the basename.

[guidocella]

You can verify the implementation in mp_property_filename().

Also see #14635 (comment)

[guidocella]

mp_path_strip_trailing_separator() modifies the original string. You can't just suddenly start modifying every string passed to mp_basename().

[WhitePeter]

Thanks again. But it gets stranger because turning that mp_basename call into a noop, still results in the trailing '/' being stripped, which did not happen before.

Anyway, I think the last commit should round this out by adding a call to mp_path_strip_trailing_separator before doing anything else. But it feels wrong to cast path to (char *). I am unsure how to proceed since my C skills are rather limited. Hints are very welcome.

[WhitePeter]

I think this should be it. I've updated the PR message to reflect my findings. Correct me if I am wrong, seeing that in case of no slashes in path it would be returned as (char *) anyway, I believe it is OK to cast to (char *) on that path stripping call. This does however strip the trailing path separators permanently. I've tried char *path_ = strdup(path) and doing the manipulations on that but must have done something wrong, because mpv locked up hard when i hit the screenshot key.

[na-na-hi]

Since %f expands to '' and somehow mpv also forgets about screenshot-dir in the process, the effective path template now is '/%P'

This is intended according to the documentation. --screenshot-template can be an absolute path.

[WhitePeter]

BTW, doesn't this overwrite what happens above in case mpv runs on Windows and path is not a URL?

[WhitePeter]

Since %f expands to '' and somehow mpv also forgets about screenshot-dir in the process, the effective path template now is '/%P'

This is intended according to the documentation. --screenshot-template can be an absolute path.

Yes, it may be, as is mine. But setting --screenshot-dir=~/tmp/mpv-shots and --screenshot-template='%f/%P' and the filename property being one of those URLs ending in a '/' resulted in the logic to fail and even erase the screenshot-dir(!) part in the process, effectively. Because mp_basename returns '\0', which terminates strings, in that case. And something must have blown up further down the line.

[na-na-hi]

and the filename property being one of those URLs ending in a '/' resulted in the logic to fail and even erase the screenshot-dir(!) part in the process, effectively.

This is not what is happening. When %f expands to an empty string, --screenshot-template is /%P, an absolute path. So this path is used as is and screenshot-dir is ignored.

[WhitePeter]

and the filename property being one of those URLs ending in a '/' resulted in the logic to fail and even erase the screenshot-dir(!) part in the process, effectively.

This is not what is happening. When %f expands to an empty string, --screenshot-template is /%P, an absolute path. So this path is used as is and screenshot-dir is ignored.

Sorry for the misleading message. What actually happens is that mp_basename finds the first (last) occurence of '/' with strrchr (searches from the end), which in such URLs is the last character and since that does evaluate to TRUE it returns s + 1 which is the next char and that is '\0'.

P.S.: I've updated the PR message. Sorry again for the confusion, I simply missed that part on my last edit.

[WhitePeter]

mp_path_strip_trailing_separator() modifies the original string. You can't just suddenly start modifying every string passed to mp_basename().

Sorry I missed this. Arguably those trailing slashes are redundant. And running basename /foo/bar///// in any shell returns bar, doesn't matter that it's a directory.

Maybe some guidance on how to solve this? I've tried working with a strdup copy but I guess the callers expect a pointer to a (sub)string of path? Because returning one to the duplicate led to mpv locking up when hitting the screenshot key which triggers a mp_basename call.

[WhitePeter]

Also, about manipulating path in place, isn't that what this #if block does as well?

[WhitePeter]

Ok, so the lockup happens because screenshot enters an infinite loop and writes files directories to the proper screenshot-path (screenshot-dir + screenshot-template). Should the caller maybe use a strdup copy to pass to mp_basename?

P.S.: As I suspected mp_dirname expects the returned result to point to an index inside the original path by mp_basename(fname) - path. Not sure yet what to make of it.

[guidocella]

Arguably those trailing slashes are redundant.

Are you sure yt-dlp doesn't return them for a reason? e.g. is there an extra redirect if you reload that playlist entry?

running basename /foo/bar///// in any shell returns bar, doesn't matter that it's a directory.

mpv implements the GNU version of basename(3) as I already showed in #14635 (comment). Changing it requires careful consideration of the pros and cons and ensuring everything like utils.split_path() still works.

Maybe some guidance on how to solve this? I've tried working with a strdup copy but I guess the callers expect a pointer to a (sub)string of path? Because returning one to the duplicate led to mpv locking up when hitting the screenshot key which triggers a mp_basename call.

mp_basename either needs to allocate memory or return a bstr.

Also, about manipulating path in place, isn't that what this #if block does as well?

That only changes the pointer.

[WhitePeter]

Arguably those trailing slashes are redundant.

Are you sure yt-dlp doesn't return them for a reason? e.g. is there an extra redirect if you reload that playlist entry?

Pretty sure. Also filename, or path rather, is not returned by yt-dlp, it's the URL argument with which yt-dlp gets called. In my case, I can just omit the trailing / and get the same stream. It's just that the URLs are part of a playlist, so I have no real control over them.

running basename /foo/bar///// in any shell returns bar, doesn't matter that it's a directory.

mpv implements the GNU version of basename(3) as I already showed in #14635 (comment).

Sorry, I missed that before. Now I understand and stand corrected.

Maybe some guidance on how to solve this? I've tried working with a strdup copy but I guess the callers expect a pointer to a (sub)string of path? Because returning one to the duplicate led to mpv locking up when hitting the screenshot key which triggers a mp_basename call.

mp_basename either needs to allocate memory or return a bstr.

Thanks for pointing that out, but I think it's in direct conflict with the policy (GNU basename). I must look elsewhere then for finding the cause of the problem.

Also, about manipulating path in place, isn't that what this #if block does as well?

That only changes the pointer.

Ah, now I see. Thanks for explaining!

[guidocella]

Pretty sure. Also filename, or path rather, is not returned by yt-dlp, it's the URL argument with which yt-dlp gets called. In my case, I can just omit the trailing / and get the same stream. It's just that the URLs are part of a playlist, so I have no real control over them.

Ok. In theory direct media URLs could end with /. I don't know if these actually exist, but if it doesn't cause issues, trailing slashes would have to be stripped in mp_normalize_path(). After #16896 is merged, playlists will only use normalized paths.

Though I was not shooting down changing mp_basename, it just needs consideration. I don't know what is better.

[WhitePeter]

@guidocella but it does cause issues. Those URLs do exist, e.g. this one which triggered some bug as described in the PR message. So mp_basename does return an empty string as per the GNU basename manual. But that should not affect screenshot-dir, yet it does somehow.

My updated expectation is that a %f specifier that evaluate to the empty string, would simply lead to an empty value in its place in the final path, which is basically the concatenation of screenshot-dir and screenshot-template (if it doesn't start with '/', and it does not in my case).
So mpv --screenshot-dir=${HOME}/tmp/mpv-shots --screenshot-template='%f/%P' https://example.org/video/ should result in said screenshot path being /home/user/tmp/mpv-shots//timestamp.ext but as it is now it results in /timestamp.ext which is not a user writable destination.

And I can't really understand where it goes wrong.

[guidocella]

I meant if stripping the trailing slash doesn't cause issues with any direct media URL redirecting or not playing. #17015 (comment) already explained what is happening.

[WhitePeter]

I meant if stripping the trailing slash doesn't cause issues with any direct media URL redirecting or not playing. #17015 (comment) already explained what is happening.

But that is not what the documentation suggests. The decision, if screenshot-template is an absolute path should be solely based on it starting with / before expanding the template. Because from my perspective as a user, going by all that I know from the manual filename and thus %f should never be an empty string; it may be non-existent, in which case a fallback is set (NO_FILE), but that is also not immediately clear. That GNU basename detail (return '' if it ends with '/') does however make the expansion of %f into '' in this error scenario.
Edit: If anything, the way how the filename property works, I expected the same value I get by print-text ${filename} (which is the whole URL, when they end with /) being sanitized as a path component.

Could this maybe solved by stating that all screenshot-template are relative to screenshot-dir? If the latter is empty all that's left is screenshot-template. So effectively joining screenshot-dir ? screenshot-dir/ : '' and screenshot-dir, and only then expand the fmt specifiers. This would automatically result in screenshot-template becoming absolute if screenshot-dir is unset and screenshot-template (expanded or not) starts with /.

[guidocella]

Can't you just also use the fallback if the filename is empty? i.e. if (!video_file || !video_file[0])

[WhitePeter]

Can't you just also use the fallback if the filename is empty? i.e. if (!video_file || !video_file[0])

I did think of that, but I figured there may be a reason why it is not done that way already. My guess was that there is a subtle difference in the property getter returning NULL (property unavaible) and a long URL that just happens to evaluate to '' because of that GNU basename quirk. I would rather have '' evaluate to '', which seems to be how other tools do it, i.e. yt-dlp (unless an explicit fallback is requested).

[guidocella]

It's not done that way because nobody thought about it. It was added in a8e6de9 for the case of running without any file.

[WhitePeter]

It's not done that way because nobody thought about it. It was added in a8e6de9 for the case of running without any file.

That commit describes a very different scenario, which only strengthens my belief in the distinction. After all filename is anything but empty and a file is playing. But indeed there is no final filename component. So there's that.

[WhitePeter]

But it irks me that, if %f/%P expands to /timestamp, screenshot-dir gets overridden. I only discovered this issue by pure chance; accidentally hit 's' and got a write permission error, which never happened before. And only then did my setup fall apart.

[WhitePeter]

Oh, one more thing, which I forgot to carry over in my edits to the PR message:
%{filename[:fallback]} behaves diametrically opposed to %f in direct contradiction to the manual. Since %{filename} is expanded in a generic case, which handles all property names used in a template, it does not get the basename treatment and is expanded as-is with / translated to _.

[WhitePeter]

How about falling back to the full content of the filename property, in case %f expands to ''? That should sync the behavior to the %{filename} case. That way not all such streams end up sharing the same 'NO_FILE' fallback. While a URL (with _ in place of /) may be ugly to look at, it is great for disambiguation.

Any takers?

[guidocella]

You can just update the PR to do that.

[WhitePeter]

Reopening for new approach to tackling the underlying issue.
It does look like my first instinct was correct; my premature deletion of that branch severed the link for good, doesn't that I recreated it with the same name.