[vlc-devel] [PATCH 1/4] filters: flush before closing filters

Steve Lhomme robux4 at ycbcr.xyz
Tue Oct 13 12:39:38 CEST 2020


On 2020-10-13 11:53, Alexandre Janniaux wrote:
> Hi,
> 
> On Tue, Oct 13, 2020 at 10:15:33AM +0200, Steve Lhomme wrote:
>> On 2020-10-13 9:51, Rémi Denis-Courmont wrote:
>>> So what? An asynchronous filter would be able to output until close
>>> returns, not until close starts. Also some filters may output some place
>>> else (e.g. scene) and distinguish between flush and drain at that level.
>>
>> In the async case (not using sinks like I proposed) the output would be set
>> in stone. The filter "owner" would be responsible for it and the filter.
>> When the owner decides to close, it should drain the filter of every
>> possible output.
>>
>> But the flush is different from the drain, in most cases it drops some state
>> used to generate the next output, be it from normal filtering or draining
>> (for now they are the same call). Once the draining mentioned above is done,
>> it's fine to flush this state and then close the filter. The flush for such
>> filter should already exist anyway (fps is currently missing it and this is
>> wrong) so we might as well use this code.
>>
>>> You're just making assumptions/restrictions in the core about
>>> implementation decisions and details of the filters.
>>
>> There is no harm in calling the Flush. If the filter implements it, it
>> should be ready to be called by the "owner". We are going to close the
>> filter right after anyway. And that doesn't do any assumption on what the
>> filter will do.
>>
>> Filters can still call Flush or do similar things if they want. Except they
>> have the choice to avoid it and have even less boilerplate code.
> 
> To be honest, I prefer having one line in each modules that would
> need that instead of one line for every modules, even those that
> won't need it. When debugging I don't really care about the number
> of line (especially if it's about only one) but I do care about
> the number of files I must have in mind and could get involved
> in the code path. 

Then why stop there ? Why shouldn't every module initialize all filter_t 
values that are not explicitly set by the creator of the module ? For 
example all filters should set vctx_out to NULL. There is no guarantee 
it will always be set. And that pointer is their responsibility.

I think duplicating code is always wrong. It makes reading the code 
harder/longer. It's also more error-prone. But I guess we're back to the 
whole "defensive programming" thing. VLC's philosphy is to duplicate all 
the code in the hundreds of filter rather than having a centralized logic.

That's much like the replacement of Flush() call
> to filter_Flush() call.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
>>> Le 13 octobre 2020 09:49:56 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a
>>> écrit :
>>>
>>>      On 2020-10-13 8:37, Rémi Denis-Courmont wrote:
>>>
>>>          Hi,
>>>
>>>          I don't think that's correct. A filter could very well want to
>>>          drain
>>>          rather than flush at close, or whatever. It's no surprise that
>>>          flushing
>>>          is often done at close but I don't think it should be forced.
>>>
>>>
>>>      This is a Close call. At this point the filter cannot decide to drain,
>>>      there's noone left to receive the pictures. On the other hand, if it has
>>>      buffers pending, it MUST release them on Close. It can be done
>>>      internally or via a global Flush.
>>>
>>>          And then it makes reading the code more difficult, as it's
>>>          nonobvious
>>>          that Flush would be called implicitly at exit. Soon we'd see
>>>          Close ->
>>>          Flush calls being added again to fix nonexistent leaks.
>>>
>>>
>>>      It's a possibility but it's not a bug and won't cause any problem, just
>>>      wasted CPU cycles. Code review or debugging should avoid this in most cases.
>>>
>>>      Having filters with dangling pictures/blocks on Close is totally
>>>      preventable by making sure we always flush. It also avoids duplicate
>>>      code in Close when the Flush is already coded. I can see some other
>>>      filters that could be simplified because of this.
>>>      ------------------------------------------------------------------------
>>>      vlc-devel mailing list
>>>      To unsubscribe or modify your subscription options:
>>>      https://mailman.videolan.org/listinfo/vlc-devel
>>>
>>>
>>> --
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
>>> ma brièveté.
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
> 


More information about the vlc-devel mailing list