ytdl_hook cleanups and playlist index support #5505
Conversation
wiiaboo
force-pushed
the
ytdl_playlists
branch
from
9766e48
to
f249b09
Compare
| @@ -320,7 +320,8 @@ local function add_single_video(json) | |||
|
|
|||
| if not (sub_info.data == nil) then | |||
| sub = "memory://"..sub_info.data | |||
| elseif not (sub_info.url == nil) then | |||
| elseif not (sub_info.url == nil) and | |||
| url_is_safe(sub_info.url) then | |||
There was a problem hiding this comment.
Should this have been in the point release? Is this really just a nit?
There was a problem hiding this comment.
Well, it's still somewhere where youtube-dl could give us unsafe urls, so I guess it could've been.
Would need something like
<video id="video" controls preload="metadata">
<source src="video/video.mp4" type="video/mp4">
<source src="video/video.webm" type="video/webm">
<track label="English" kind="subtitles" srclang="en" src="unsafe://stuff" default>
</video>
player/lua/ytdl_hook.lua
Outdated
| @@ -543,7 +540,8 @@ mp.add_hook(o.try_ytdl_first and "on_load" or "on_load_fail", 10, function () | |||
| local subfile = "edl://" | |||
| for i, entry in pairs(json.entries) do | |||
| if not (entry.requested_subtitles == nil) and | |||
| not (entry.requested_subtitles[j] == nil) then | |||
| not (entry.requested_subtitles[j] == nil) and | |||
| url_is_safe(entry.requested_subtitles[j].url) then | |||
| @@ -142,6 +142,45 @@ local function is_blacklisted(url) | |||
| return false | |||
| end | |||
|
|
|||
| local function parse_yt_playlist(url, json) | |||
There was a problem hiding this comment.
Should probably include some more detail about the issue or feature this is addressing in the commit message.
| @@ -560,6 +560,10 @@ Program Behavior | |||
| which mpv should not use with youtube-dl. The patterns are matched after | |||
| the ``http(s)://`` part of the URL. | |||
|
|
|||
| The `use_manifests` script option makes mpv use the master manifest URL for | |||
| formats like HLS and DASH, if available, allowing for video/audio selection | |||
| in runtime. It's disabled ("no") by default for performance reasons. | |||
There was a problem hiding this comment.
So prior to this commit, the effective default was the opposite, right? That is, stream selection was always enabled if available. If so, should probably add that to the commit message.
player/lua/ytdl_hook.lua
Outdated
| -- youtube extractor provides only IDs, | ||
| -- others come prefixed with the extractor name and ":" | ||
| local prefix = site:find(":") and "ytdl://" or | ||
| "https://youtu.be/" |
There was a problem hiding this comment.
how sure are we that youtube is the only place we see such urls?
There was a problem hiding this comment.
Not sure. Would need to go through youtube-dl's url parsing, but just looking at youtube's extractor, everything except the ID is optional. Looking at lazy_extractors.py, which is used to generate an optional list of valid URLs for faster extraction, there's 35 extractors which use the shortcut <extractor>:<id>. There's no such shortcut for youtube.
| @@ -215,8 +215,8 @@ local function edl_track_joined(fragments, protocol, is_live, base) | |||
| end | |||
|
|
|||
| local function has_native_dash_demuxer() | |||
| local demuxers = mp.get_property_native("demuxer-lavf-list") | |||
| for _,v in ipairs(demuxers) do | |||
| local demuxers = mp.get_property_native("demuxer-lavf-list", {}) | |||
There was a problem hiding this comment.
If this is aimed at helping people backport ytdl, you might want to include some of the information from the PR intro in the commit message.
This is important if backporting by grabbing the latest version of the script without backporting the commit that added the property.
This was overlooked when doing the whitelisting for video and audio.
Remove obsolete comment about FFmpeg ignoring non-http proxies which was repeated in ytdl_hook before the feature was added. Remove unnecessary conditions for not nil. Lua tables will always return nil for non-existent keys.
Still needs `--ytdl-raw-option=yes-playlist=` because this only works for youtube. This was requested in a few issues: #1400 #2592 #3024 For #1400 to be completely implemented would need ytdl_hook to re-request the same playlist with the last video's ID for the mix to continue indefinitely which would probably too hackish to work reliably.
Disable by default. This feature was added in 7eb3427, which allowed stream selection in runtime. Problem with this atm is that FFmpeg will try to demux every first packet of every track leading to noticeable delay opening the URL. This option can be changed to enabled by default or removed when HLS/DASH demuxers are improved upstream.
Only youtube playlists return ID-only urls. Other extractors may return "<extractor>:<ID>" so those still need the ytdl:// prefix. Reproduced with http://www.cbc.ca/burdenoftruth/videos/trailers-promos/burden-of-truth-returns
wiiaboo
force-pushed
the
ytdl_playlists
branch
from
f249b09
to
a995288
Compare
|
Dealt with all comments so far. |
Also another off by default script opt to avoid reverting support for manifest_url usage. Can be changed to on or removed when hls/dash demuxing of master playlists is improved in FFmpeg.
The first commit is due to seeing maintainers simply grabbing ytdl_hook for their backporting needs. If the commit adding demuxer-lavf-list isn't backported too using it will break the script.
Concerning on_load_fail, there's no way to probe for that afaik, so if you're backporting this by simply grabbing the latest version of the script, remove that too or backport 89f81da too.