[vlc-devel] [PATCH] transform: inline error handling

[Steve Lhomme]

On 2021-01-08 12:16, Alexandre Janniaux wrote:
> Hi,
> 
> On Fri, Jan 08, 2021 at 10:50:57AM +0100, Steve Lhomme wrote:
>> On 2021-01-08 10:29, Rémi Denis-Courmont wrote:
>>> Le perjantaina 8. tammikuuta 2021, 11.19.38 EET Steve Lhomme a écrit :
>>>> On 2021-01-08 10:16, Alexandre Janniaux wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Jan 08, 2021 at 07:40:11AM +0100, Steve Lhomme wrote:
>>>>>> On 2021-01-07 16:51, Alexandre Janniaux wrote:
>>>>>>> ...and remove useless error label. The resources are released by the
>>>>>>> vlc_objres mechanism already.
>>>>>>
>>>>>> Did you check all possible ways to create filters do release resources on
>>>>>> Open error ? If so this should be done in other filters as well.
>>>>>
>>>>> I don't understand, this patch is local to the transform filter.
>>>>> I don't change anything outside of it, and have checked that the
>>>>> objres release is actually done (which it is, through module_need).
>>>>
>>>> So you understand that the key for this patch to work is that
>>>> module_need calls do the cleaning.
>>>
>>> And? All this patch does is replace an explicit (objres) free with an implicit
>>> objres free. As Alexandre already pointed out, it has no bearing on other
>>> modules.
>>
>> No but my point is that if we assume plugins (all of them?) are already
>> cleaned when the activate callback fails, *then* we can do the same patch
>> for all other plugins.
> 
> I think the confusing came from the way you formulated your
> request, which felt like something I had to do to merge this
> patch, ie. that I had to modify all other plugins.
> 
>> On the other hand if it's not clear, or checked, that plugins are always
>> cleaned on activate, then this patch introduces a leak.
> 
> It's quite easy to check in the code, thus why I wrote that
> it was already released in the commit message. ;)

I don't find it easy. Modules, let's say just video filters in this 
case, can be loaded in various ways. For video filters it seems to 
always via filter chains (core, mosaic bridge, transcode). Just for that 
in transcode you need to follow vfilters_cfg which seems to be only used 
in p_filterscfg and then in transcode_video_filters_init(), which for a 
"video filter" ends up in psz_filters and is used with 
filter_chain_AppendFromString().

The filter chains use module_need which does the vlc_objres_clear() for 
us. This wouldn't be the case if the module was loaded through a custom 
activate function. That's when vlc_module_load() is used directly.

There aren't many occurrences of "video filter" in the code so it's 
still manageable. But IMO it's far from straightforward. In the absence 
of rules on how we free module resources every time we load one, it's 
always a lot of extra checks with the possibility of missing something.

Maybe all we need is to do this vlc_objres_clear() call in 
vlc_module_load() as all module loading ends up going through. Then we 
wouldn't have to worry anymore.