[vlc-devel] [PATCH 1/4] filters: flush before closing filters
Alexandre Janniaux
ajanni at videolabs.io
Tue Oct 13 13:26:21 CEST 2020
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.
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.
> 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,
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
More information about the vlc-devel
mailing list