[vlc-devel] [PATCH 11/16] video_output: explicitly use the displayed.XXX pictures as filter chain inputs
robux4 at ycbcr.xyz
Tue Oct 20 11:01:15 CEST 2020
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.
>>> Alexandre Janniaux
>>> 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);
>>>> @@ -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);
>>>> - picture_t *filtered = filter_chain_VideoFilter(sys->filter.chain_interactive, torender);
>>>> + picture_t *filtered = filter_chain_VideoFilter(sys->filter.chain_interactive, sys->displayed.current);
>>>> if (!filtered)
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
More information about the vlc-devel