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

[Rémi Denis-Courmont]

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.

> assert that the input picture is never modified and proceed
> with documentation change?

I don't know how you can assert that.

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

-- 
レミ・デニ-クールモン
http://www.remlab.net/