Skip to content
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: broadcast ytdl_path and json #10410

Conversation

christoph-heinrich
Copy link
Contributor

@christoph-heinrich christoph-heinrich commented Jul 12, 2022

Share the found path to yt-dlp/youtube-dl and the acquired json for videos with other scripts.

I've been using this together with a modified quality-menu for about a week now without any problems and was curious what you think about this.

There are currently no script messages being sent out to user scripts (afaik), but this seems like the easiest way of sharing that information with others.

The documentation is based on the one for events.

dyphire added a commit to dyphire/mpv-scripts that referenced this pull request Jul 13, 2022
Scripts that wanted to use yt-dlp/youtube-dl had to either search
for it themselves or provide an option for users to set a valid
path for it.
ytdl_hook already does the work of finding a valid ytdl_path,
so share that path with others via a script message.

Scripts that wanted to use the json from yt-dlp/youtube-dl for
the currently playing file had to call yt-dlp/youtube-dl themselves.
ytdl_hook already calls yt-dlp/youtube-dl to get that json,
so share that json with the corresponding url via a script message.
@christoph-heinrich christoph-heinrich changed the title [RFC]: ytdl_hook: broadcast ytdl_path and json ytdl_hook: broadcast ytdl_path and json Aug 31, 2022
@christoph-heinrich
Copy link
Contributor Author

christoph-heinrich commented Sep 1, 2022

From the discussion on IRC it became clear that this is never going to land and quality-menu is basically just a work around for deficiencies in mpv and I should be working on those instead.

@avih
Copy link
Member

avih commented Sep 1, 2022

Basically the info (all available ytdl tracks info) is already exposed as track metadata when using all-formats=yes. Maybe not enough for everyone, but definitely enough for many/most, and improvements here would be welcome.

However, OP doesn't use all-formats=yes because there's a bug where it doesn't work with --vd-queue-enable - #10394 , but they do want this option - not for the option main goal of threaded decoding, but because it has a side effect of queuing about 1s worth of frames, and without this big frames queue a 10 years old CPU can't play smoothly some 1440p 60fps clips.

So to work around this bug and issue, and to get the available info without enabling all-formats=yes but with --vd-queue, they wanted to broadcast the JSON so that a script could capture the track metadata for all tracks even without all-formats=yes.

Other than being an ugly hack to work around some others issues and local limits, it has another big problem where this JSON is basically a dump of raw data directly from ytdl.

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.

If there's info which ytdl-hook has privately but which could be useful to other clients/scripts, then we could export this in a well-defined way, which scripts can rely on (and probably as a shared script property rather than broadcast). But this PR serves nothing except solving a very unique and specific limitation, and is not useful to any script other than to work around the described limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants