[vlc-devel] [PATCH 11/16] video_output: explicitly use the displayed.XXX pictures as filter chain inputs

[Alexandre Janniaux]

Hi,

On Tue, Oct 20, 2020 at 07:51:23AM +0200, Steve Lhomme wrote:
> On 2020-10-19 16:07, Alexandre Janniaux wrote:
> > Hi,
> >
> > I don't mind doing this switch, but note that using the return value of
> > picture_Hold() actually allows to inject tracking code for the Hold/Release
> > cycles and thus determining where an Hold() has not been matched with a
> > Release(). It needs changes in the code (like the revert of this patch)
> > anyway so there's no point in keeping it though.
>
> Looking at the code there is about half of picture_Hold calls that don't
> store the result value. Most of the time, that's just before calling a
> filter (chain). So if such a change is introduced, all these calls will need
> to be checked anyway.

Sure, that's what I said, but because this is fairly easy to catch with asan
(use-after-free, double free, leak), it takes no more than 5 min to patch the
code. I just highlighted that it was very convenient and first mentioned I
didn't care about this. It could be useful to you too.

> > However, it's weird that in the first patches of the serie, you removed
> > assignments to sys->displayed.current from ThreadDisplayPreparePicture
> > and then in this part, use sys->displayed.current directly instead of
> > the previous intermediate variable. I'm not sure I follow the rational
> > behind these changes.
>
> I can't find a patch where I removed assignments to displayed.current. There
> are patches to move the assignment outside of ThreadDisplayPreparePicture()
> (which now returns a pre-rendered picture). But it's just moving the code to
> have the logic clearer to follow.

You removed the assignment to displayed.current in ThreadDisplayPreparePicture
yes, that's exactly what I've written. If the goal is to have the logic clearer
to follow, why not passing displayed.current to ThreadDisplayRenderPicture too?

> The general goal of this patchset, some previous ones, and more to come, is
> to factorize/simplify the code so it's more readable. One of the tools to do
> this is to avoid intermediate values that serve no purpose. If you look at
> displayed.current and find 3 intermediate variables using it with different
> effect on it, it becomes harder to understand what happens to it in the end.

TBH, moving code around without a rationale is not more readable to me.

The way it was written before is quite alike C++ objects. ThreadDisplay* were
methods of the vout thread and modified internal state like routine. I agree
that using intermediate variable is a bit more tedious to follow, but that's
level more readable than intermediate functions in my opinion, and there's no
point in keeping both paradigms where method reading internal state or return
a state that will be used to modify internal state. I think it would actually
be clearer only if you do the same everywhere, and it just moves complexity
without gain otherwise.

Regarding modifying internal state or returning new state for ThreadDisplay*
function, I don't really care about which one is the best. Moving the picture
logic to a single function helps tracking what is displayed in the end and
splitting from the actual processing, but keeping as internal state allows to
clearly recognize the state across the whole vout_thread.

> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Fri, Oct 16, 2020 at 04:26:42PM +0200, Steve Lhomme wrote:
> > > ---
> > >   src/video_output/video_output.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> > > index 3b3c8e36bd1..28c357cb607 100644
> > > --- a/src/video_output/video_output.c
> > > +++ b/src/video_output/video_output.c
> > > @@ -1139,7 +1139,7 @@ static picture_t *ThreadDisplayPreparePicture(vout_thread_sys_t *vout, bool reus
> > >           sys->displayed.timestamp     = decoded->date;
> > >           sys->displayed.is_interlaced = !decoded->b_progressive;
> > >
> > > -        picture = filter_chain_VideoFilter(sys->filter.chain_static, decoded);
> > > +        picture = filter_chain_VideoFilter(sys->filter.chain_static, sys->displayed.decoded);
> > >       }
> > >
> > >       vlc_mutex_unlock(&sys->filter.lock);
> > > @@ -1215,12 +1215,13 @@ static int ThreadDisplayRenderPicture(vout_thread_sys_t *vout, bool is_forced)
> > >   {
> > >       vout_thread_sys_t *sys = vout;
> > >
> > > -    picture_t *torender = picture_Hold(sys->displayed.current);
> > > +    // hold it as the filter chain will release it or return it and we release it
> > > +    picture_Hold(sys->displayed.current);
> > >
> > >       vout_chrono_Start(&sys->render);
> > >
> > >       vlc_mutex_lock(&sys->filter.lock);
> > > -    picture_t *filtered = filter_chain_VideoFilter(sys->filter.chain_interactive, torender);
> > > +    picture_t *filtered = filter_chain_VideoFilter(sys->filter.chain_interactive, sys->displayed.current);
> > >       vlc_mutex_unlock(&sys->filter.lock);
> > >
> > >       if (!filtered)
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > 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