mpv-player
ytdl_hook.lua: trim preceding text before JSON
There might be additional text in the output before the JSON payload if certain options like --list-format are specified in the config.
specifying --list-formats in your config does not make sense, any other example?
@sfan5 I haven't tried looking for other examples but I can explain my rationale.
I have added it to my config for debugging, sometimes the formats may not be available so it's always handy to see the list when I use youtube-dl directly.
So IMO it's a valid use case and we should try to cater for all valid configurations anyway, especially since there are no downsides in this case.
Download the artifacts for this pull request:
Windows
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This looks like a hacky way to extract json. Isn't there a way to redirect json to stderr or pipe? So we can access it cleanly, regardless of other output? Could you raise this issue upstream?
@kasper93 Agreed, it's hacky, but it's a quick fix for an edge case, I'm still in favour of merging this until upstream has this sorted out, it might take some time for that to happen.
I will create an issue there and see what happens in the meanwhile.
Personally I would expect yt-dlp to return an error if I simultaneously used --dump-single-json and --list-formats, because the format list is plain text so the output would no longer be valid JSON.
Or alternatively it could return the format list as JSON but then I would expect it to skip the actual stream extraction (as it normally does when using --list-formats).
In either case this combination should not really work.
especially since there are no downsides in this case.
Yes there is and you said it yourself: the hacky code in mpv
Personally I would expect yt-dlp to return an error
I was thinking more along the lines of just ignoring the request since dumping the JSON should take precedence ideally. It could also write an error to stderr which shouldn't cause problems with the normal stdout output we capture.
but then I would expect it to skip the actual stream extraction (as it normally does when using --list-formats).
That can be overridden with --no-simulate so that I can still download the videos, that's what I did in my config.
Yes there is and you said it yourself: the hacky code in mpv
Well it's a fix to improve integration with a 3rd-party tool, so it's acceptable IMO. We can always remove it once upstream has a fix for this edge-case.
For this specific case we could also just specify --no-list-formats but it doesn't work with youtube-dl.
@guidocella I'm using yt-dlp and it seems to work when I try it directly but unfortunately it doesn't work with mpv --ytdl-raw-options-append=no-list-formats= for whatever reason.
Oh that would be better, I was thinking of adding it directly to the default command in ytdl_hook.lua which would break youtube-dl. I don't know why it wouldn't work with --ytdl-raw-options though.
What if format/title or anything in printed output contains { that is not part of json?
@kasper93 Well that would be an issue, but it's highly unlikely. We can modify the code to loop until it finds the right { but I think it's overkill for such an unlikely scenario which we may never encounter in the wild.
Also I have an update regarding upstream, I talked with the developers in yt-dlp's Discord and they want me to open an issue in their repo, but even assuming that they do fix this, we would still have an issue as yt-dlp is a fork of the real upstream... and that hasn't received any updates in over 4 months, so it's very unlikely that it's going to be fixed soon.
These are our options:
- Change our officially supported
youtube-dlversion to the active fork and wait for them to fix the issue. - Post an issue in the
youtube-dlrepo and wait for however long it takes to get it fixed (if ever). - Merge this workaround and just go with the flow until we have a fix from upstream.
Option 3 is the most immediate fix available, and obviously it's my preferred option until this is fixed in upstream as it's dependent on external factors.
I don't think this needs to be fixed with youtube-dl since this is very niche and youtube-dl is barely maintained (currently it doesn't even work with youtube).
I'm using
yt-dlpand it seems to work when I try it directly but unfortunately it doesn't work withmpv --ytdl-raw-options-append=no-list-formats=for whatever reason.
I think I got the same results yesterday but now --no-list-formats doesn't work even on the command line, what gives?
I opened an issue in upstream: https://github.com/yt-dlp/yt-dlp/issues/14378
I think I got the same results yesterday but now --no-list-formats doesn't work even on the command line, what gives?
Perhaps it stopped working after updating, I did an update yesterday and I cannot use --no-list-formats manually anymore, it used to work earlier unless I am somehow mistaken.
--no-list-formats is short for --no-list-formats-as-table, which is an alias for --compat-options list-formats. This will just revert the formats table to how it was printed in youtube-dl, not disable the formats list.
These are our options:
There is an option 4 which is "consider --list-formats in user-specified options as invalid", with practically no downsides and no workarounds needed in mpv.
@sfan5 I'm sorry but I have to disagree, option 4 is just accepting that we can't handle certain valid (as considered by youtube-dl) options with a very real downside of not being able to use it in mpv entirely.
If this isn't fix isn't incorporated then users would be forced to maintain a separate config just for mpv and have it go out of sync potentially with the default config.
Compared with Option 3 it's just a simple check in the Lua code, and the only downside is that our code is a bit hacky now, but we can always remove it once upstream has fixed the issue.
we would still have an issue as yt-dlp is a fork of the real upstream... and that hasn't received any updates in over 4 months, so it's very unlikely that it's going to be fixed soon.
I don't think "upstream" youtube-dl is used by anyone anymore, it doesn't even work for youtube. If the project is unmaintaned and doesn't fix bugs it will remain broken, we don't have to add random workarounds for abandoned software.
That being said, proper fix is to update yt-dlp to allow reading JSON cleanly.
... (currently [yt-dl] doesn't even work with youtube).
Instead of --no-list-formats, consider either --ignore-config or perhaps more usefully --config-location /path/to/mpv/ytdl/hook/default/ytdl_config.
The ytdl hook should be able to assume standard settings rather than what has been configured by the user; if the system-wide settings have been made in a way that breaks the ytdl hook code, that is a problem with the system-wide installation. Then the hook options can safely add its own options, such as -j, that can be amended or complemented by users with --script-opts ... or otherwise.
Having said that, options that have only a --opt with no --no-opt or, worse, the converse are poorly designed and a feature request ideally with a PR would be welcome.
@dirkf That's an ideologically different take than what's currently implemented, I agree that having a separate yt-dl config for mpv has its benefits but it is my opinion that by default we should respect the user's config, they have configured it that way for a particular reason.
As a real world example, maybe they have bandwidth limits and only want to view videos within a certain resolution, or only want to view videos in a free format like VP9 or AV1 instead of the H26X formats.
if the system-wide settings have been made in a way that breaks the ytdl hook code, that is a problem with the system-wide installation.
We can go one level deeper and think about why it breaks the hook and fix the breakage... that's what I did :)
Having said that, options that have only a --opt with no --no-opt or, worse, the converse are poorly designed and a feature request ideally with a PR would be welcome.
I agree, I was looking for such an option as well. There's already an issue open with yt-dlp to fix the root cause but it may take some time.
In the meanwhile we can have this hotfix merged and remove it once upstream has dealt with it. Without this hotfix users would be forced to compromise in some other way... personally I'd just make a wrapper script which just filters the output to just the JSON because I don't want to maintain a separate config just for mpv.
As a real world example...
Hence my suggestion to have a well known user config location for the ytdl hook.
In the example, the config for limited bandwidth and/or screen resolution would probably be better in the system-wide config*; if not, or for one user's foibles, the user could link or copy or edit the user config to that location, or perhaps there could be a hook option to choose between hook default location, yt-dl default location or specified location.
* Another real-world example, though not using mpv: for a set-top box installation, where the built-in player is the only way to access its H.264 decoder and can't handle more than 50fps+1080p, we put that system config in the yt-dl package for the platform.
@dirkf Pardon me, but I don't really understand what you're suggesting, it just sounds like a more complicated way to do what are already able to do in mpv currently with just normal config options.
Also it does not solve the issue if the user is able to customize the config, one way or another.
So, with minimal knowledge of the hook internals, this is my suggestion...
- The system yt-dl/yt-dlp config should be expected to include whatever is necessary/appropriate for the system itself, as in the real-world examples above.
- The ytdl hook UI should specify a location for a ytdl hook user config, say the yt-dl/yt-dlp user config default with
.ytdl_hookextension, to be used instead of any yt-dl/yt-dlp user config. - If no ytdl hook user config is present, the default yt-dl/yt-dlp user config should be used.
For implementation, check for the presence of the well known ytdl hook user config and add --config-location well/known/location if found.
Examples:
- an empty ytdl hook user config would prevent per-user config from affecting ytdl hook processing
- a minimal ytdl hook user config would allow some possibly useful additional config while omitting breaking options like
-F - an unwise user could still put
-Fin the ytdl hook user config, or in the yt-dl/yt-dlp user config with no ytdl hook user config, but: "Doctor, it hurts when I hit myself with this hammer"...
Personally I do not see the problem that is solved by making ytdl_hook not respect the global ytdl configuration file.
You can already do --ytdl-raw-options-append=ignore-config= --ytdl-raw-options-append=config-location=/path/to/config if you really want to. But it is simpler to set any extra yt-dlp options in mpv.conf.
That's great news, we can now work around this by explicitly passing that option in mpv... so a new PR? :thinking:
Also since this is a fix in the forked upstream how should we handle this?