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

[Steve Lhomme]

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.

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

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.


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