github-actions[bot]
commented
2 months ago Download the artifacts for this pull request:
Closed Dudemanguy closed 2 months ago
github-actions[bot]
commented
2 months ago Download the artifacts for this pull request:
Jules-A
commented
2 months ago I'm honestly surprised it isn't already but if it is to be, I really don't think it should auto-load images by default.
sunpenghao
commented
2 months ago Another reason to leave it off by default is so that it doesn't conflict with the autoload.lua already in users' scripts directory (which most people do have if "autoload is miles away the most popular one").
hooke007
commented
2 months ago Another reason to leave it off by default is so that it doesn't conflict with the autoload.lua already in users' scripts directory (which most people do have if "autoload is miles away the most popular one").
Apply this logic, if so other builtin scripts should be disabled too. Third-parties oscs are much more popular.
sunpenghao
commented
2 months ago Third-parties oscs are much more popular.
I doubt it, but suppose you are right. The built-in OSC has been there virtually throughout the history of mpv, and disabling it is a well accepted prerequisite of using third-party ones. For autoload.lua, there is no built-in alternative until after the PR is merged, not to mention by having built-in autoload the behavioral change alone is enough to make it an incompatible update, so I don't think this is relevant.
hooke007
commented
2 months ago Before merging autoprofile, "there is no built-in alternative" too. We don't disable autoprofile by default at that moment.
hooke007
commented
2 months ago having built-in autoload the behavioral change alone is enough to make it an incompatible update
As we mentioned before, we already had a script-user option to control its behav. If you afraid of any incompatible things. Why not set it to no as the default value?
If the builtin scripts not be loaded by default. Why bundling it?
sunpenghao
commented
2 months ago As we mentioned before, we already had a script-user option to control its behav. If you afraid of any incompatible things. Why not set it to
noas the default value? If the builtin scripts not be loaded by default. Why bundling it?
Why are you arguing this in the first place? I'm just saying the behavior should be off by default. I don't care if this is done by setting a script option or not having the script loaded all together.
hooke007
commented
2 months ago Why are you arguing this in the first place
It was mentioned previously. Not me firstly.
sunpenghao
commented
2 months ago It was mentioned previously. Not me firstly.
So what's your point?
hooke007
commented
2 months ago It was mentioned previously. Not me firstly.前面已经提到过。首先不是我。
So what's your point?那么你的观点是什么？
https://github.com/mpv-player/mpv/pull/14549#discussion_r1677263650
sunpenghao
commented
2 months ago Exactly. From https://github.com/mpv-player/mpv/pull/14549#discussion_r1677263650:
You just need to make its first option's default value to yes instead.
Are we not in agreement in the first place? What's the point of spamming the thread with all these?
hooke007
commented
2 months ago disable the function ≠ Unload the script.
Actually there could be 4 opinions in this PR.
llyyr
commented
2 months ago I thought about doing this a while back, but at the end of the day the script is a giant hack. IMO this feature should be rewritten properly and be part of the mpv codebase instead of being bundled as a script if this behavior is desired enough to be worth all that effort. That way we could also consolidate some other options like audio-art-auto-exts, cover-art-auto-exts etc.
Of course someone would have to step up and actually do the work for that :^)
guidocella
commented
2 months ago We also debated on the IRC channel that if this is going to be builtin it and done properly it may be worth behaving like MPC-HC and uosc which scan the directory on playlist-next/prev instead of pre-filling the playlist. Then you can navigate to files moved in the directory after starting mpv, of if you append a file to the playlist at runtime, it will be the next file instead of being appended to the end of a playlist.
hooke007
commented
2 months ago "pre-filling the playlist" could be another choice for users too if on demand by adding script-binding/msg on the fly rather than only hook the event start-file currently. Apparently it requires that the script not to be disabled.
kasper93
commented
2 months ago IMO this feature should be rewritten properly and be part of the mpv codebase
mpv already can create playlist from directory, all the code is there. Maybe without the filtering part of autoload.lua the core functionality is duplicated. And while to mimic the same behavior some options would have to be added to mpv the script itself doesn't bring much functionality in itself.
Dudemanguy
commented
2 months ago I don't think calling autoload a hack is fair. It does exactly what it is supposed to using the APIs we expose to all mpv clients. I guess unless you consider looking at file extensions to be a sin, but mpv's demuxers do that to some extent as well. Just the reality of working with media files.
Anyways, the reason this script exists is because wm4 didn't want this functionality in core mpv. For me personally, I don't really care if this lives in mpv's source code or as a script on the side, but to implement it within mpv itself would be a lot of work likely prone to annoying edge cases and not trivial. I'm certainly not going to write this code. If someone wants to, they are of course welcome to, but I doubt we'll have anyone attempting anytime soon. Meanwhile, the autoload script works fine and does what it needs to do already.
kasper93
commented
2 months ago but to implement it within mpv itself would be a lot of work likely prone to annoying edge cases and not trivial.
I was thinking otherwise. I feel like the mpv has all ingredients to do this. It already can generate a playlist from directory. Correct me if I'm wrong, but the only missing thing is filtering, which could be done with OPT_STRINGLIST options. And single flag to auto create playlist from parent dir when a file is opened.
I'm certainly not going to write this code.
Fair. And don't get me wrong, not expecting that. Just discussing the overall problem that we have at hand.
Dudemanguy
commented
2 months ago Correct me if I'm wrong, but the only missing thing is filtering, which could be done with OPT_STRINGLIST options. And single flag to auto create playlist from parent dir when a file is opened.
Maybe I'm wrong. This doesn't sound so bad now that I think about it some more.
sunpenghao
commented
2 months ago Correct me if I'm wrong, but the only missing thing is filtering
Also natural sorting for playlist entries. This is the single most important reason why I use the script instead of directly loading directories.
guidocella
commented
2 months ago We have natural sorting since a7158ceec0.
sunpenghao
commented
2 months ago We have natural sorting since a7158ce.
Ah, don't know how I missed that.
na-na-hi
commented
2 months ago Correct me if I'm wrong, but the only missing thing is filtering
I think it's better to think through all of the required ingredients needed to be put together to make this work before making such a statement. The devil is in the details. For example, when I specify a filename on the commandline, loadfile will:
All of them need to reimplemented in C. This requires cooperation between loadfile (to determine whether to active autoload depending on the files provided, and determine which demuxer to use), the demux system (demux_playlist is used for opening a directory) and the playback core (to jump to the correct position). I feel that this would likely introduce some special cases and the implementation won't be trivial.
kasper93
commented
2 months ago All of them need to reimplemented in C.
Simple matter of programming.
figure out the directory name from the file name
mp_dirname if requested demux_playlist to open directory as normally mpv wouldlist directory contents, maybe recursively
Already implemented --directory-mode=<auto|lazy|recursive|ignore>
find the item position in the playlist and jump to that item
Do you think it would be hard to search open path in created playlist?
All of them need to reimplemented in C.
What exactly has to be "reimplemented"? Few options to control behavior of file opening, no?
I feel that this would likely introduce some special cases and the implementation won't be trivial.
Special cases, probably, but last played file is already restored for directory playback, so again, not sure why demonize it to be not trivial for some reason? I would be really reluctant if it really required all scan directory implementation and so on, but all this is already present in the code of mpv. It probably is not 10 lines patch, but nothing big either.
Again, I'm not saying it should be done, but bloating main mpv binary with duplicated implementation of those features in lua, effectively with filtering on top is not something I endorse.
EDIT:
I see two separate tasks here
demux_playlist with filtering similar to autoload.lua to allow create playlist consisting only of certain types of files when opening directory.kasper93
commented
2 months ago Allow to open parent directory based on file path, if requested by option. And start playback from the file requested.
Done in https://github.com/mpv-player/mpv/pull/14555 completely not tested and probably not working in all cases. I got baited by na-na-hi into doing it. I don't like when people try to say something is "undoable".
Extend demux_playlist with filtering similar to autoload.lua to allow create playlist consisting only of certain types of files when opening directory.
This one needs thinking what is the cleanest way to filter entries. I feel like current autoload.lua is little bit convoluted in what it does.
Dudemanguy
commented
2 months ago Closing since #14555 will solve this better.
Of all the scripts located in the TOOLS directory, autoload is miles away the most popular one. It gets the most contributions by far, and the feature is clearly fairly popular among users. Instead of forcing them to manually copy the script into their script folders, make it a builtin instead. Unlike the other lua scripts, this is disabled by default given that it's a big behavioral change that is not universally desirable but users can enable it with a simple line in their configs.