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

Steve Lhomme robux4 at ycbcr.xyz
Wed Oct 14 15:14:29 CEST 2020 

On 2020-10-13 13:26, Alexandre Janniaux wrote:
> On Tue, Oct 13, 2020 at 12:39:38PM +0200, Steve Lhomme wrote:
>> 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.
> 
> The comparison is not fair, you cannot compare initialization of an
> object and setup of this object through module loading. That's the
> whole point of the object/module splitting. And even there, we don't
> setup the values after each filters and expect failing filters to
> clean the state or not modify it, so we don't even add things there.
> 
> In the case here, if we decided that filters must be flushed before
> closing, we should flush before closing at the correct location, but
> 
> 1/ we didn't decide that filters must be flushed before closing, and
>     even then this decision can be either explicit (assert that we
>     flushed before closing) or implicit like you did.

Do we have to agree on writing a patch before writing it ? (no)
If the patch is rejected, fine. But I'm not going to ask before doing 
every code change I'm doing.

> 2/ you're forcing this behaviour in close() instead of higher level
>     call site that would make it more explicit and thus readable in
>     client code.

That's code factorization.

>> 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.
> 
> Yes, why have code path that could call filter_Flush() without
> knowing it, both in the module and the filter user code, while
> they could want to drain originally? This is completely arbitrary
> “in case if the client forgets to do it“ and I think it's much
> more suitable to have the client code writing what it expects
> (and potentially fail if we arrive in an incorrect state) than
> taking high-level decision in case it's not correctly written.
> 
> I don't care how the code is written, I care about how I read
> and debug it, and here you add _additional_ hops outside the
> code I would be reading, to achieve the exact same effect,

Yes, that's how *you* end up on that code. That's not how I end up on 
that code. Trying to refactor the code I get to see all the usages, 
calls and implementations of filters. I do see patterns of things that 
can be factorized. And once you factorize things you realize more code 
can be factorized and simplified and so on. If we never do this work the 
code will always remain a giant blob of inconsistent code that can 
hardly ever been touched. And so you end up focusing on one local 
implementation when the same thing is done hundreds of time elsewhere, 
sometimes slightly differently. Or you decide to do it your way. Ending 
up doing the same mistakes everybody else went through. There's a lot of 
wasted time never factorizing code.

> hiding it both to the module that might implement this because
> it doesn't support it otherwise (for example because Flush is
> actually the cleaning function, so an internal detail) and the
> client code like chromecast that would make it explicit that
> end of streams are dropped instead of drained.
> 
> That's basically why I disagree with the patchset.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
>> 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
>>>
>> _______________________________________________
>> 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