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

[Alexandre Janniaux]

On Wed, Oct 14, 2020 at 03:14:29PM +0200, Steve Lhomme wrote:
> On 2020-10-13 13:26, Alexandre Janniaux wrote:
> > 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.

If the patch brings a new feature or a new interesting behaviour,
I don't deem it's necessary to discuss the design itself since it
can be changed afterwards. However I do think we need to discuss
when just changing semantics of existing points without rationales,
or especially when the only rationale is «it should do this this way»
and people disagree.

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

To be honest, it's like talking about micro-optimization. The goal is
not to blindly write an optimization for a very specific part, and
especially not without measuring it. It's to provide a general sensible
optimization by providing an optimization where it matters, otherwise
you just overly complexify future work and optimization for the long
term. Factorization is the same and we usually use function to provide
factorization of large chunk and reuse them locally. We don't usually
refactor a side-effect behaviour (filters calling their internal flush
method to cleanup their state) by putting it in the middle of the code
path (filters calling filter_Flush() to clean the internal state before
closing) and especially if it wasn't designed for this. Doing so is a
new design choice leading to a lot of work for absolutely no gain, need
people to adapt to the new changes and understand the new model, etc.

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

Fair, I hear your argument since it's mine too. But your vision is not
the one of the current code so it needs at least some solid rationale
to do that, otherwise I could just send the reverse patch saying that
it's cleaner and more readable and we would just ends up with the same
effect as controversial wikipedia pages, until someone locks the subjet
whereas we could just be smart, listen to each other and actually
discuss what we want to have in the end as global guidelines instead of
arguing that one's view of programming is better than the other on a
particular example, especially since you seem to disagree on this
subject, the defensive programming subject, and did a lot of refactor
that many people disagree on recently. It would probably help making
the situation clearer and more fair.

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

I agree that we need refactor, but like written above, I usually prefer
recurrent behaviour to be refactored into function call, not inlined in
an even more common code path.

Likewise, having a common cleaning method called «Flush» doesn't make
every filters needing a flush. It could also very likely be called
«Close» in filters without sys, but would make it unreadable.

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

There's a lot of wasted time reading code and not understanding it too.
By increasing the complexity of code path you also increase this and
I don't know how you can just say that your way is better without
accepting and compensating for this fact somehow.

Also, I'm not sure to see how this apply to the current patchset.

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