[vlc-devel] [PATCH] fps: hold the extra pictures we send

Steve Lhomme robux4 at ycbcr.xyz
Fri Oct 2 09:13:05 CEST 2020

On 2020-10-02 7:59, Alexandre Janniaux wrote:
> Hi,
> On Thu, Oct 01, 2020 at 09:39:25AM +0200, Steve Lhomme wrote:
>> On 2020-09-30 10:06, Alexandre Janniaux wrote:
>>> Hi,
>>> On Tue, Sep 29, 2020 at 06:02:06PM +0300, Rémi Denis-Courmont wrote:
>>>> Le tiistaina 29. syyskuuta 2020, 8.36.26 EEST Steve Lhomme a écrit :
>>>>> The same picture is supposed to be filtered or rendered at a different date,
>>>>> but the pixels should be the same. The content is not going to be changed
>>>>> in the pictures we output from this filter.
>>>>> ---
>>>>>    modules/video_filter/fps.c | 5 +----
>>>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>>> diff --git a/modules/video_filter/fps.c b/modules/video_filter/fps.c
>>>>> index 4e6a59dfd66..5dffffe4d35 100644
>>>>> --- a/modules/video_filter/fps.c
>>>>> +++ b/modules/video_filter/fps.c
>>>>> @@ -116,10 +116,7 @@ static picture_t *Filter( filter_t *p_filter, picture_t
>>>>> *p_picture) should be avoided, it's only here as filter should work in that
>>>>> direction too*/ while( unlikely( (date_Get( &p_sys->next_output_pts ) +
>>>>> p_sys->i_output_frame_interval ) < p_picture->date ) ) {
>>>>> -        picture_t *p_tmp = NULL;
>>>>> -        p_tmp = picture_NewFromFormat( &p_filter->fmt_out.video );
>>>>> -
>>>>> -        picture_Copy( p_tmp, p_sys->p_previous_pic);
>>>>> +        picture_t *p_tmp = picture_Hold( p_sys->p_previous_pic );
>>>>>            p_tmp->date = date_Get( &p_sys->next_output_pts );
>>>>>            last_pic = vlc_picture_chain_Append( &p_sys->p_previous_pic,
>>>>> last_pic, p_tmp );
>>>> Filters assume that their input picture is not shared, and are conversely not
>>>> allowed to provide a shared picture on output.
>>>> Holding a picture like this would work if VLC implemented a systematic copy-
>>>> on-write policy, but AFAIK it does not at this time.
>>> I agree with you, but maybe we can review the existing code,
>>> assert that the input picture is never modified and proceed
>>> with documentation change?
>>> The main case in which we're actually modifying the picture
>>> is blender if I'm not wrong, but opaque buffers that are
>>> sampleable are not necessarily renderable so it raises issues
>>> to allow picture modification anyway, much alike encoder opaque
>>> buffers in other context.
>> Yes there is something to be done with blending in GPU. But a blender is not
>> exactly a filter. It has its own type, its own capability and its own API. I
>> was specifically talking about video filters.
> I think it doesn't change the issue here because we eventually
> have a blender in the end in some case (android, screenshot,
> spu overlay...) and if the original picture is used despite the
> new policy on picture, it would be incorrect and probably glitch
> in multiple cases.

I think the issue is entirely on the blender side. It was always wrong 
to write in the source picture unknowingly. In fact if you do you will 
very quickly notice decoding garbage. That's why in practice there is 
always some checks/workaround around the use of blending.

> On of the fix could be done by specifically copying like you
> mention below.
>> In the video output blending is either done specifically on an RGB32 CPU
>> picture, or via a copy of the of the source picture.
>> https://code.videolan.org/videolan/vlc/-/blob/master/src/video_output/video_output.c#L1344
>> So whether fps sends the same buffer with different dates has no impact
>> here.
>> On Android there's also a local blender for the SPU but it also seems to be
>> CPU based pictures (we do not currently have GPU blenders on master).
>> Then there are the audio visualizations. They all work with software chromas
>> as well.
>> The only blender that may have issues with GPU sources is the one in the
>> transcoder. There is already some tricks to not write on a filter if there's
>> no filter chain (?). I suppose it's assuming that if there is a filter chain
>> the source MAY be a GPU one. In that case it's using the input format of the
>> encoder which is, as of today, is a CPU chroma but should change soon. So
>> this is wrong.
>> In any case, whether fps sends the same buffer with different dates has no
>> impact on this.
> I don't think the issue is related to CPU vs GPU. This is
> the issue you were trying to fix (though it should have
> been picture_Clone).
> The issue mentioned by Rémi is more about picture usage
> policy. If you send a CPU buffer and it's matching a blender
> format, the blender would write directly on it, thus changing
> the content for future fps pictures.
>>> I see CoW as a potential optimization, whereas using picture_Hold
>>> (actually clone would be better suited here because of metadata
>>> changes like date) widen the scope of pictures supported by the fps
>>> filter.
>>> I do see the issue with using picture_Hold though, given we could
>>> modify the date behind the back of picture clients.
>> I don't understand. The source of a picture can modify the date. That's the
>> most basic use of picture_Hold in the code. What is the "picture client"
>> here ?
> picture_Hold means that you get the same exact image as the
> one you hold. If you hold the picture twice in fps so as to
> resample and change the pts twice, it will be held once.
> picture_Clone creates a new picture with the same baking
> storage as the origin picture. But since it's a different
> picture, you can actually set a new pts for it.

You're right. I meant picture_Clone all along but wrote picture_Hold.

> Here, picture client is the component after the filter that
> will get the picture. For instance, vout_thread that would
> schedule the picture to render.
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

More information about the vlc-devel mailing list