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

[Steve Lhomme]

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.

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