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

[Steve Lhomme]

On 2020-10-20 10:25, Alexandre Janniaux wrote:
>>> 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

Maybe I didn't emphasize enough that I *moved* the code, not *removed* it.

> to follow, why not passing displayed.current to ThreadDisplayRenderPicture too?

I did not touch that part of the code. I did in another patchset that 
was partly refused IIRC. I'll go back to it at some point.

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

It makes total sense to shuffle the code around and rename things that 
don't have carry the meaning of what they do.

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

This is what I have been working on for months. I keep discovering 
things because everything was intertwined and pretty much impossible to 
follow with many booleans colliding, with names that make you think it's 
something when in fact it's not, etc. Once the spaghetti code is split, 
it's a lot clearer who does what. For example the frame by frame that 
was impossible to read and in the end is just doing one thing: read the 
next picture and put it in the current.

>>> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>