screenshot: fix %f corner case #17021
Conversation
|
I also changed the This just an aside, though. If you want |
|
According to DOCS/contribute.md GNU features should not be used and it may not work on Windows. |
WhitePeter
force-pushed
the
fix-screenshot-template-f-expansion
branch
from
7fd0aa5 to
98e477a
Compare
|
I am a little embarassed for missing that. I grepped for |
WhitePeter
force-pushed
the
fix-screenshot-template-f-expansion
branch
2 times, most recently
from
896d9d2 to
a5f29e5
Compare
|
Sorry for the last force push, had to rewrite history. |
When the path is a URL, use the whole path as basename. The dirname will be "." in this case. This simplifies various checks throughout the codebase. In particular, it avoids returning an empty string for URLs ending with /. The filename property will return full URLs, which is desirable because the domain and path segments before the last are useful information. This fixes half of mpv-player#10975. The empty string check in mp_property_filename() can be removed because it only happened with the basename of URLs ending with /. mp_property_filename() will not be called with directories with trailing slashes because mpv expands them immediately when they are the current playlist entry; I tested it by observing filename. Also mpv-player#16896 will prevent directories with trailing slashes anyway. This also fixes the issue described in mpv-player#17015 of %f in --screenshot-template being evaluated to an empty string for URLs ending with /, which can inadvertently make it use an absolute path. Alternative to mpv-player#16932 and mpv-player#17021.
When the path is a URL, use the whole path as basename. The dirname will be "." in this case. This simplifies various checks throughout the codebase. In particular, it avoids returning an empty string for URLs ending with /. The filename property will return full URLs, which is desirable because the domain and path segments before the last are useful information. This fixes half of mpv-player#10975. This also fixes the issue described in mpv-player#17015 of %f in --screenshot-template being evaluated to an empty string for URLs ending with /, which can inadvertently make it use an absolute path. Alternative to mpv-player#16932 and mpv-player#17021.
|
Download the artifacts for this pull request: Windows |
|
I've also changed |
player/screenshot.c
Outdated
| static char *stripext(void *talloc_ctx, const char *s) | ||
| { | ||
| const char *end = strrchr(s, '.'); | ||
| const char *end = strrchr(mp_basename(s), '.'); |
|
Why are you copying things from my PR? You are duplicating review work for maintainers. This should be closed and further comments should go in my PR. |
|
Sorry, I did not mean to offend. I gave you credit and it is not a direct copy, because as you can see in my last commits |
WhitePeter
force-pushed
the
fix-screenshot-template-f-expansion
branch
2 times, most recently
from
91a53ff to
38d805f
Compare
|
Sorry, had to squash some commits after discovering some subtle bugs in Also, the check for initial dots was in the wrong place, in case |
|
Yeah I also realized |
|
I suggest:
|
|
It's also worth noting that we also have |
|
I am having second thoughts on the multiple leading dots issue. But I don't want to change anything else for the time being to not have yet another push. There is also a warning about incompatible pointer conversion I missed and want to shut up, just as a reminder to myself. |
I didn't know about them. I just saw But I am having a hard time wrapping my head around bstr usage. Can you give me or point me to a quick rundown, what purpose they server? I know from the bstring(3) manual that they are not necessarily Ideally there should only be one of each |
|
See https://en.wikipedia.org/wiki/Null-terminated_string#Limitations and https://en.wikipedia.org/wiki/Null-terminated_string#Improvements But using Another thing we can do is to replace |
|
While I am digesting your last digesting your last comment, @guidocella, an idea popped into my head. Could this perhaps be done in lua? I am asking because, I still don't quite like the duplication when expanding So maybe this PR can serve as a stopgap measure to then port this stuff to lua, which I reckon to be way easier than doing this in C. It's not as if this is a hot code path. |
|
I don't see how there is duplication. |
|
The cases |
|
There is nothing to outsource. Getting the filename is trivial. And core functionality should work in builds with Lua disabled. |
|
And in said Lua version there will be one helper function that gets called in all the cases. Or maybe Lua can express cases better? Not sure, needs exploration. In Lua the |
OK, but I was looking at that whole long
I think the core functionality can be boiled down to |
No. Retrieving a single property is different from expanding property strings. And Lua functions are built on C functions, not the opposite.
Core functionality like playback and taking screenshots works fine without Lua, so it doesn't need to be required. |
Yes, I know that. But why use the C functions with all that memory management overhead, i.e. dragging the talloc context along, and all that makes C difficult to deal with, when a simple
I am totally with you on playback, of course, but only half with screenshots. One can get 99% of But I don't want to argue. I am just trying to make a case for some code trimming to get a leaner meaner mpv core which really only provides media playback as best as can be done. Screenshots are media creation and as such not a show stopper if they did not exist at all as concept in mpv; there was, after all, a time before P.S.: Just think about it. I don't how big the audience of the thread is, but maybe others think differently. And if, in the end, we still disagree: no harm done. |
It literally doesn't. I just explained that retrieving properties and expanding property strings are different. You are thinking of the
That's fair if we were implementing |
Don't get hung up on the
Worse things have happened. I was surprised quite a few times by mpv dropping functionality from its core and changing options etc. Most of the time I could see why and made my peace with new reality otherwise. It's not like this has to be an explosion and I think you may be overestimating the impact. The simplest case would be to ask users to install Lua. Since options are exposed as properties, nothing else needs to change. |
|
Just to be clear. I am not talking about getting rid of |
That's just 3 lines of typical boilerplate like everywhere in mpv's codebase, no crazy overhead for which everything must be rewritten.
Continuing to use the options for a script would be incorrect and lead to arguing, see #15655 (comment). Whereas changing them will be a breaking change, and script-bindings and script-opts are more verbose/less convenient to use than commands and options, so there's that too. I also don't think I'm overestimating the impact since users continuously complain about every change, and in fact specifically about screenshot options I remember that when we renamed Also for every added script there is the overhead of adding a manual section to document it, add boilerplate code in several files to load it, add and document an option to disable it, and have it constantly running in its own thread, though the overhead is minimal.
Please stop the ridiculous and off-topic ideas. |
|
Message received. |
|
One last thing I just have to say to this because it does demonstrate, that 99.9% can be achieved with plain Adding this to input.conf: emulates my initial example ( Now I shut up about the matter. I will go through the chore list and come back when I'm done. Thanks for all your insights. |
|
|
|
Thinking it through, the way you just suggested is the easiest since most If we were to implement it now, we could get rid of You would have to rebind all 4 default screenshot bindings to change path though, so maybe they are worth having for that, but at least the format specifiers are not needed. It's probably not worth breaking all user configs and libmpv clients now though. But we could recommend using only the properties in the documentation and only keep format specifiers for backwards compatibility. |
WhitePeter
force-pushed
the
fix-screenshot-template-f-expansion
branch
from
38d805f to
d759f26
Compare
|
Sorry for the delay, personal matters. I have squashed some commits (went through your suggestions) and made some minor changes, i.e. use a macro for mp_basename_or_url. Other than that it's a cleaned up version of the same. |
This introduces a simple wrapper around `mp_basename`. It returns URLs unchanged. Especially with filename representations of streaming service URLs the last path segment, which usually points to a file in most other contexts, is becoming ever less meaningful. So provide this to conveniently use URLs as-is where usually a file name would be expected. Some caution is advised, because components that expect no '/' may get tripped up. Credit goes to @guidocella for the idea.
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 qualifies as
URL. 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`
Not particularly pretty but less ambiguous and more consistent over a
diverse range of input paths.
[1] mpv-player#14635 (comment)
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.
My previous commits for %f expansion in screenshot templates leads to
inconsistency with the %{filename} format specifier. Since consensus
seems to be(come) to always use full URLs[1], make the filename property
getter return just that, which also fixes aforementioned inconsistency
in screenshot template expansion.
DOCS/man/input.rst: Update filename property
[1] mpv-player#10975
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.
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.
Reuse existing functionality, see previous commit introducing above function.
WhitePeter
force-pushed
the
fix-screenshot-template-f-expansion
branch
from
d759f26 to
118a3dd
Compare
|
Sorry again, I just had to harden that macro, otherwise, if used in the wrong expression, operator precedence might have silently changed the semantics, e.g. |
3 participants
URLs can end with trailing slashes (/) which in turn results in GNU basename[1] returning the empty string. Catch that case and fallback on the raw value of mpctx->filename. Subsequent path sanitation takes care of translating invalid path component chars.
Issue reproduction steps:
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.extNot particularly pretty but less ambiguous and more consistent over a diverse range of input paths. For transparency and reference also see [2] which sadly could not be revived.
[1] #14635 (comment)
[2] #17015
Check!