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

[Alexandre Janniaux]

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.

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.

Regards,
--
Alexandre Janniaux
Videolabs