[vlc-devel] [PATCH 2/4] ytdl: stream filter module for YoutubeDL

[Rémi Denis-Courmont]

Le torstaina 24. syyskuuta 2020, 10.15.40 EEST Steve Lhomme a écrit :
> On 2020-09-24 8:36, Rémi Denis-Courmont wrote:
> > Le torstaina 24. syyskuuta 2020, 9.05.34 EEST Steve Lhomme a écrit :
> >> On 2020-09-23 17:43, Rémi Denis-Courmont wrote:
> >>> +static void Close(vlc_object_t *obj)
> >>> +{
> >>> +    stream_t *s = (stream_t *)obj;
> >>> +    struct ytdl_playlist *sys = s->p_sys;
> >>> +
> >>> +    msg_Dbg(s, "terminating PID %u", (unsigned)sys->pid);
> >> 
> >> Do we need a debug message everytime a http URL has been opened ? (no)
> > 
> > Yes. If it gets stuck waiting we know why and on what PID.
> 
> But there's a log even if there is no problem.

This is ridiculous. This access adds 2 lines: one at the beginning and one at 
the end. That's a hell of a lot less than the Lua playlist parsers that fit in 
the same functional category in the same place.

I guess you did not even try this, or you would have noticed that the logs are 
so spammed by the MP4 demuxer that you won't even notice the difference. Fixing 
the MP4 demuxer is not in scope of this series. Be my guest if you want to 
improve that.

> Also does that mean some http queries may be stuck until explicitly
> closed ? Shouldn't we manage a time-out or something ?

I don't even know what you are on about.
This snippet is obviously about cleaning up after the child process.

> >>> +vlc_module_begin()
> >>> +    set_shortname("YT-DL")
> >>> +    set_description("YoutubeDL")
> >>> +    set_category(CAT_INPUT)
> >>> +    set_subcategory(SUBCAT_INPUT_STREAM_FILTER)
> >>> +    set_capability("stream_filter", 305)
> >>> +    set_callbacks(OpenFilter, Close)
> >>> +    add_bool("ytdl", true, N_("Enable YT-DL"), N_("Enable YT-DL"),
> >>> true)
> >> 
> >> Defines are usually used for such strings.
> > 
> > And then people forget to delete them, so they stick in the PO files and
> > occupy translators needlessly. There are no points in doing that when the
> > string are short enough.
> > 
> >> Also you should probably call it Youtube-DL.
> > 
> > I have very mixed feelings about that. First I don't want really to show a
> > trademark there. And more importantly, I don't want to mislead that it's
> > limited to a specific service.
> 
> Maybe use a completely different name then.
> Also we ship a youtube.lua with plenty of Youtube references.

First, youtube.lua parses YouTube URLs only while this parses hundreds of 
different sites. And then exactly zero of those Lua's references to YouTube end 
up showing up in the preferences. So I don't think that's comparable.

If the board is comfortable with putting YoutubeDL everywhere, they can make 
the trivial patch or ask me to do so at a later time. I won't make that call.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/