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

[Steve Lhomme]

On 2020-09-26 18:27, Rémi Denis-Courmont wrote:
> 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.

Two wrongs don't make one right.

>> 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.

You're the one who replied:
"Yes. If it gets stuck waiting we know why and on what PID."

So my question is will it result in some HTTP requests potentially stuck 
forever and is there a measure to mitigate that ?

>>>>> +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/
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>