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

[Alexandre Janniaux]

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