[vlc-devel] [PATCH] youtube.lua: Add support for youtube playlist

[Pierre Ynard]

Hello,

> +function parse_json(url)

This looks like cargo cult; did you just copy-paste this function from
somewhere else?

> +    vlc.msg.dbg("Trying to parse JSON from " .. url)

The youtube script doesn't need that, vlc.stream() will output logs for
this call anyway.

> +    local json = require ("dkjson")

I'm not a fan of relying on modules that makes scripts not
self-contained. Moreover, everywhere in the script parses just the
necessary data by hand, so it doesn't make much sense that dkjson would
be used for playlists only. Also, that's lazy initialization and I
suspect it's cargo cult from other kinds of lua scripts, but this is not
a requirement in playlist parser scripts.

>      then -- This is the HTML page's URL

This is wrong, your patch adds playlist support in simply the wrong
place. It would match the page of a video that's part of a playlist, and
parse its playlist instead of playing the video. And it would fail to
parse a playlist page, which would be the expected behavior as far as I
can tell. For example:

https://www.youtube.com/watch?v=VuNIsY6JdUw&list=PLMC9KNkIncKtPzgY-5rmhvj7fax8fdxoj&index=3
This is a video page. We want the script to play the video. Your patch
would parse the playlist it's part of instead.

https://www.youtube.com/playlist?list=PLMC9KNkIncKtPzgY-5rmhvj7fax8fdxoj
This is the corresponding playlist page. Your patch doesn't support it.

> +        if string.match(vlc.path, "list=") then
> +            local playlistID = get_url_param( vlc.path, "list" )

The two parsing calls are redundant.

> +            local response = parse_json(vlc.access .. "://www.youtube.com/list_ajax?action_get_list=1&style=json&list=" .. playlistID)
> +            local playlist = {}
> +            for _, item in ipairs(response.video) do

You don't check the return value of parse_json(). If it fails for any
reason, in particular vlc.stream() failing on a network error, the
script crashes there.

To be honest I'm just not convinced that playlist support belongs
in the main youtube script. It's already big enough, but playlist
business doesn't seem to be in any way coupled to the many things the
script already handles for video pages. The only advantages I see are
distribution simplicity, and avoiding any conflict when claiming support
of particular youtube pages in separate probe functions; but really how
hard can the latter be anyway.

-- 
Pierre Ynard
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."