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

[Alexandre Janniaux]

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.

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.

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