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

[Alexandre Janniaux]

Hi,

On Wed, Sep 30, 2020 at 06:15:01PM +0300, Rémi Denis-Courmont wrote:
> Le keskiviikkona 30. syyskuuta 2020, 11.06.04 EEST Alexandre Janniaux a écrit
> :
> > 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,
>
> I really don't know if CoW is a good or bad idea for picture_t, notably if it
> would improve or worsen performance. But frankly, I think we should hold on
> design changes at this point in the 4.0 development timeline, or we'll never
> be done.

I agree that it's an unnecessary change for 4.0.

> > assert that the input picture is never modified and proceed
> > with documentation change?
>
> I don't know how you can assert that.

I meant assert as «after review, there is no case which is modifying
the input, so let's write it down in a patch that switch the documentation».

> > I do see the issue with using picture_Hold though, given we could
> > modify the date behind the back of picture clients.
>
> You just pointed out a(nother) bug in the patch.

Just to be clear, your point was only on the pixel data then?

Regards,
--
Alexandre Janniaux
Videolabs