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.lua: support playlist_title #15098

Merged
merged 1 commit into from
Oct 26, 2024
Merged

Conversation

Earnestly
Copy link

@Earnestly Earnestly commented Oct 15, 2024

This PR mostly serves as a discussion about how this issue might be resolved.

ytdl_hook is aware of many useful playlist metadata tags which are essentially lost during the creation of a playlist as yt-dlp is called twice: once for the playlist url and then again for each entry. Here the json representing the playlist and its metadata is no longer available in the json produced for each entry.

It is not clear whether yt-dlp should provide the metadata or if ytdl_hook should go out of its way to try and capture this information and provide it during another instance of run_ytdl_hook() and what consequences that might have.

See also yt-dlp/yt-dlp#11234

Copy link

github-actions bot commented Oct 15, 2024

Download the artifacts for this pull request:

Windows
macOS

@@ -27,6 +27,7 @@ end)

local chapter_list = {}
local playlist_cookies = {}
local playlist_title = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= "" needs to be removed to not always add this to the json as empty strings are truthy in Lua.

Copy link
Author

@Earnestly Earnestly Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have actually done this locally because of exactly the issue you're describing. In fact playlist_title is now a table called playlist_metadata which updates the playlist_title and playlist_id; a loop is then used to update the information for single videos. I.e.:

        -- restore playlist metadata if it exists
        for key, value in pairs(playlist_metadata) do
            if value then
                print(value)
                json[key] = value
            end
        end

@guidocella
Copy link
Contributor

If you play a regular youtube video as the second argument after a playlist it keeps the previous playlist title, is there a way to reset it? The commit works otherwise.

@Earnestly
Copy link
Author

Earnestly commented Oct 24, 2024

If you play a regular youtube video as the second argument after a playlist it keeps the previous playlist title, is there a way to reset it? The commit works otherwise.

I'm not sure. The general heuristic here is to assume if a playlist title was available at some point that any subsequent video(s) are part of that playlist, which is pretty silly.

Because the playlist title is not directly associated with videos there is no reliable relationship to use. This is primarily why I think yt-dlp should provide it even with --flat-playlist as per yt-dlp/yt-dlp#11234

I can't personally think of any nice way to track this state and still believe yt-dlp needs to provide this.
(E.g. By using non-standard #PLAYLIST entries in the generated m3u, trying to track some combination of playlist- properties to detect a non-playlist item, etc. None of these seem reasonable to me but I am open to better insights here.)

Edit: Maybe one possible way is to create a table that associates each playlist entry with a playlist title and only to update the title if the single video exists in that table.

@Earnestly
Copy link
Author

Earnestly commented Oct 24, 2024

Something like this seems to work but can be improved with a better designed structure.

local playlist_titles = {}

-- ...

        -- preserve playlist metadata
        playlist_metadata["playlist_title"] = json["title"]
        playlist_metadata["playlist_id"] = json["id"]

        for _, entry in pairs(json.entries) do
            playlist_titles[entry.url] = playlist_metadata
        end

-- ...

    else -- probably a video
        -- restore playlist metadata if it exists
        local title_metadata = playlist_titles[json.original_url]

        if title_metadata ~= nil then
            for key, value in pairs(title_metadata) do
                if value then
                    json[key] = value
                end
            end
        end

        add_single_video(json)

@guidocella
Copy link
Contributor

That seems fine.

@Earnestly
Copy link
Author

Earnestly commented Oct 25, 2024

Had some time so here is a second attempt.

This does still have the downside that if a playlist and url is provided and that url happens to be part of the playlist, it will still retain the playlist's metadata.

I'm also not very familiar with lua so I'm not sure if it's memory sane to assign the metadata to every url (which can be thousands). Does lua make copies of the tables or store a reference to the same table?

@guidocella
Copy link
Contributor

Tables are assigned by reference. But always using the same metadata for all URLs is wrong if you play multiple playlists and navigate back and forth between them, so metadata needs to be local to where it's assigned.

@Earnestly
Copy link
Author

Earnestly commented Oct 25, 2024

always using the same metadata for all URLs is wrong if you play multiple playlists

While I am assigning the metadata to all urls in a playlist, each new playlist that is loaded will overwrite the entires, so the new metadata is implicitly available during the lookup later.

if you play multiple playlists and navigate back and forth between them, so metadata needs to be local to where it's assigned.

This does currently work for me* when testing using a youtube channel (videos, live, shorts, etc.), however the issue of multiple playlists (including single videos interspersed within) is nevertheless prescient. Under the current approach all urls are essentially "global" keys, so if any of the urls are shared between playlists then the metadata assigned to a specific url will be whatever playlist was seen last.

* With one minor change, if entry.url then playlist.urls[entry.url] = playlist.metadata end.

Do you have any suggestions? I've tried thinking of a few but I keep coming back to the same problem.

Edit: The test I'm using for multiple playlists is the g-p binding to switch between them.

@guidocella
Copy link
Contributor

I mean if you play 2 unrelated playlists, go to the second one, then go back to the first one, it now has the playlist title and id of the second one.

@guidocella
Copy link
Contributor

To associate the correct metadata to videos contained in multiple playlists, can't you associate metadata to playlist urls instead of video urls, and set it based on playlist-path?

@guidocella
Copy link
Contributor

Here it is:

diff --git a/player/lua/ytdl_hook.lua b/player/lua/ytdl_hook.lua
index 7e31177ef7..d878238a2d 100644
--- a/player/lua/ytdl_hook.lua
+++ b/player/lua/ytdl_hook.lua
@@ -27,6 +27,7 @@ end)
 
 local chapter_list = {}
 local playlist_cookies = {}
+local playlist_metadata = {}
 
 local function Set (t)
     local set = {}
@@ -1079,6 +1080,11 @@ local function run_ytdl_hook(url)
             return
         end
 
+        playlist_metadata[url] = {
+            playlist_title = json["title"],
+            playlist_id = json["id"],
+        }
+
         local self_redirecting_url =
             json.entries[1]["_type"] ~= "url_transparent" and
             json.entries[1]["webpage_url"] and
@@ -1193,6 +1199,12 @@ local function run_ytdl_hook(url)
         end
 
     else -- probably a video
+        local metadata = playlist_metadata[mp.get_property("playlist-path")] or {}
+
+        for key, value in pairs(metadata) do
+            json[key] = value
+        end
+
         add_single_video(json)
     end
     msg.debug('script running time: '..os.clock()-start_time..' seconds')

@Earnestly Earnestly force-pushed the master branch 2 times, most recently from 62826a9 to 52a75ef Compare October 25, 2024 22:08
Add support for tracking playlist_title and playlist_id metadata
corresponding to a given playlist url and the entries associated
with it.

This allows the inclusion of ytdl_playlist_title and ytdl_playlist_id
in the metadata property for single videos which have a corresponding
playlist-path property.

This commit also resolves yt-dlp/yt-dlp#11234
@kasper93 kasper93 merged commit 9f6cca6 into mpv-player:master Oct 26, 2024
25 checks passed
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.

3 participants