-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
ytdl_hook: make path and json available to other scripts #14098
base: master
Are you sure you want to change the base?
Conversation
This needs to be documented, see how #10410 did that. |
Has the situation changed since #10410, which was described as "never going to land"? Also this PR doesn't address the following:
|
Chicken and egg problem. Someone has to detect and set the path of ytdl. And
This is the question for script makers. If this info is useful. It is not "raw data" it is json with defined structure upstream. If any info is not included in the result the "child' script can act accordingly. I think the above comment is fucusing on some specific use-case with If the alternative is to rerun youtube-dl in every script that might need that info, I think the solution of sharing the json is better. |
Download the artifacts for this pull request: |
This is only needed if the scripts want to run the ytdl binary themselves, right? In this case why can't the path detection function be extracted and made as a common function available to scripts?
Agreed, but this still needs to be documented as such, which means mpv passes ytdl's output as-is. |
Currently we don't detect path in separate step. Script just try to run the subprocess and if it succeeded it means we "have" ytdl. This is only a problem if user has custom path set in options. |
In this case scripts can just read that option directly. |
I don't think they can. Also there is still some detection. |
The ytdl binary search can indeed be extracted into a separate function, and even placed in a separate script if desired. I extracted ytdl_hook's ytdl path detection in one of my "library" scripts that my other scripts use. Doing so in a user script is inefficient though, as my understanding is that the result can't be cached among different user scripts, every user script that |
My understanding for the "never going to land" comment is that a big part of that was the PR being essentially the XY problem (XY solution?).
Can't speak for everyone, everyone's scripts are different, but it is very useful for me. My scripts act on the input file/URL, so they run ytdl only when Anyway, the intention of the PR is to simply make things that yt_hook already has, available to other scripts, in case they find them useful, with the minimal amount of changes and technical dept. If they find them useful -- good, if not, then the user scripts are not worse than where they were before this PR. Technical debt incurring thoughtsIf the ytdl binary path not being set unless the ytdl_hook runs at least once is a deal breaker for the PR, we could make the ytdl binary searching code in ytdl_hook run on the script init, always setting the ytdl binary path. It is inefficient though because that will result in the ytdl executable always getting invoked on ytdl_hook init. It also goes against the spirit of keeping the changes to ytdl_hook minimal for this PR, avoiding adding extra technical dept to ytdl_hook to support this.
The yt-dlp json format stability is obviously at the whim of the yt-dlp project, who can break it if they so desire. So Technical debt incurring thoughtsIf the json format must be future-proofed against mpv breakage by all costs, once ytdl_hook introduces a json format breaking change, ytdl_hook could preserve the old behavior for that variable at a cost of an extra ytdl invocation, possibly gated behind some backwards compatibility config flag, but that's sounds like a lot of technical dept and with the json format already being uncontrollable I'd say don't preserve the old behavior and just let it break. |
I don't think they can.
Should be possible with mp.get_opt.
Now, if it was provided my mpv, like you are proposing, then I imagine it could be cached among multiple user scripts?
To make the result cached, we can have a separate script dedicated to the path extraction. The first script which wants the path sends a script-message to that script, which then broadcasts the result with another script-message or by setting a user-data key. Then make the built-in ytdl script also use this method. This way cached path extraction isn't called when unnecessary and can be done even when the built-in ytdl script is disabled or the hook is never called.
My scripts act on the input file/URL, so they run ytdl only when mp.get_property("path") is a URL, like YouTube, which is the case when the ytdl binary path is set by ytdl_hook.
In this case isn't it more appropriate to use script-message for this since you're only interested when the ytdl_hook is called?
|
afc282b
to
2cce6c3
Compare
2cce6c3
to
924ebf0
Compare
After playing around with yt-dlp, I noticed that when it errors, it doesn't produce json output with the error information, it only prints the error to stderr. For example, if a Twitch channel
and no json. So if user scripts wanted to react to a specific yt-dlp error, they can't do that with the json output alone as there would be no json output in the case of an error, the scripts also need to access the stderr output, checking for error patterns, to make sense of what has happened. It upsets me that yt-dlp doesn't provide error information in a machine-readable format like json, but it is what it is. So I'm thinking of changing the PR to export the entire subprocess return value, the table containing: stdout, stderr, status, killed_by_us, etc., as a single native property (not as a script-message as script-messages can send only strings, which subprocess's return value isn't). That way user scripts have all the information they can possibly have, the json, the errors, the exit code, etc. |
fb8ed4f
to
6700176
Compare
It's useful for user scripts to be able to use the same ytdl binary that ytdl_hook uses without having to replicate ytdl_hook's process of searching for the ytdl binary. Some user scripts might also find it useful to be able to access ytdl's json output that the ytdl_hook already receives, sparing user scripts from having to make a duplicate ytdl binary invocation to get the json output. Providing just the json output is not enough though, as ytdl doesn't communicate errors though it -- if an error occurs, ytdl provides no json output and instead prints to stderr. So without stderr, there is no way for user scripts to figure out why ytdl has failed: no such username / video id, the channel is not live yet, etc. Because of that, the entire result of the subprocess call is provided to the user scripts, containing stdout (json), stderr, ytdl's exit code, etc.
6700176
to
46ce40a
Compare
Modified ytdl_hook.lua to do the aforementioned from my last comment -- making the entire result of the Also added some documentation. Please check if the placement (being nested under |
Should the url be added to the result? Then scripts can check that the result belongs to the file that they think it belongs to, after all they run asynchronously. Maybe I'm overly paranoid here, as I'm sure this will work fine pretty much all of the time anyway. |
It's useful for user scripts to be able to use the same ytdl binary that
ytdl_hook uses without having to replicate ytdl_hook's process of
searching for the ytdl binary.
Some user scripts might also find it useful to be able to access ytdl's
json output that the ytdl_hook already receives, sparing user scripts
from having to make a duplicate ytdl binary invocation to get the json
output.
Providing just the json output is not enough though, as ytdl doesn't communicate
errors though it -- if an error occurs, ytdl provides no json output and instead
prints to stderr. So without stderr, there is no way for user scripts to figure
out why ytdl has failed: no such username / video id, the channel is not live
yet, etc. Because of that, the entire result of the subprocess call is provided
to the user scripts, containing stdout (json), stderr, ytdl's exit code, etc.
Fixes #14097, #12852 and #10410.
Tested to work with: