mpv
mpv copied to clipboard
mpv-player
ytdl_hook: make path and json available to other scripts
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:
local utils = require 'mp.utils'
local function print_property(name, value)
print(name .. ": " .. utils.to_string(value))
end
mp.observe_property("user-data/ytdl/json-subprocess-result", "native", print_property)
mp.observe_property("user-data/ytdl/path" , "string", print_property)
This needs to be documented, see how #10410 did that.
/cc @christoph-heinrich
Has the situation changed since https://github.com/mpv-player/mpv/pull/10410, which was described as "never going to land"?
Also this PR doesn't address the following: https://github.com/mpv-player/mpv/pull/12852#issuecomment-1802495623
This is only set when ytdl_hook ran at least once, how useful is that for other scripts?
https://github.com/mpv-player/mpv/pull/10410#issuecomment-1234919124
Such raw data cannot be relied upon by scripts, as it can change at any time. For instance if tomorrow ytdl-hook invokes ytdl twice, or if tomorrow it's invoked in a way where the JSON doesn't have the other tracks info (because ytdl-hook doesn't actually need the info for other tracks when not using all-formats=yes), etc.
This is only set when ytdl_hook ran at least once, how useful is that for other scripts?
Chicken and egg problem. Someone has to detect and set the path of ytdl. And ytdl_hook is the entrypoint for all ytld operations, so imho it is expected that all "child" scripts that may depend on this information should run only if the value is set.
Such raw data cannot be relied upon by scripts, as it can change at any time. For instance if tomorrow ytdl-hook invokes ytdl twice, or if tomorrow it's invoked in a way where the JSON doesn't have the other tracks info (because ytdl-hook doesn't actually need the info for other tracks when not using all-formats=yes), etc.
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 all-formats mention. But in fact having json output from ytdl might be used in various ways and if user is adding a "child" script they should also make sure that ytdl-hook config is correct.
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:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Someone has to detect and set the path of ytdl.
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?
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.
Agreed, but this still needs to be documented as such, which means mpv passes ytdl's output as-is.
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?
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.
This is only a problem if user has custom path set in options.
In this case scripts can just read that option directly.
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?
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 requires that "library" script has its own instance of it. Now, if it was provided my mpv, like you are proposing, then I imagine the result of searching for the ytdl binary could be cached among multiple user scripts?
Has the situation changed since https://github.com/mpv-player/mpv/pull/10410, which was described as "never going to land"?
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?).
This is only set when ytdl_hook ran at least once, how useful is that for other scripts?
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 mp.get_property("path") is a URL, like YouTube, which is the case when the ytdl binary path gets set by ytdl_hook, so my scripts can use it.
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 thoughts
If 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.
Such raw data cannot be relied upon by scripts, as it can change at any time. For instance if tomorrow ytdl-hook invokes ytdl twice, or if tomorrow it's invoked in a way where the JSON doesn't have the other tracks info (because ytdl-hook doesn't actually need the info for other tracks when not using all-formats=yes), etc.
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 user-data/ytdl/json must be documented as such, as an unstable/uncontrollable format as far as mpv is concerned. Since mpv can't promise the stability of the json format, mpv / ytdl_hook breaking it shouldn't be much of an issue.
Technical debt incurring thoughts
If 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?
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 channel_name is offline, you get:
ERROR: [twitch:stream] channel_name: The channel is not currently live
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.
Modified ytdl_hook.lua to do the aforementioned from my last comment -- making the entire result of the ytdl --json-dump subprocess command available to user scripts, i.e. the stdout, the stderr and the exit code.
Also added some documentation. Please check if the placement (being nested under user-data) and the wording is alright.
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.
This probably needs interface_changes entry
Added an interface-changes entry and also rebased on master.