[vlc-devel] [PATCH 2/3] filter_chain: remove harmful handling for video buffer

[Alexandre Janniaux]

Hi,

After more tests, it doesn't break snapshot, but snapshot
didn't work. This is fixed in

    image: forward video context when converting

However, there were issues with the next patch leading to
infinite recursion and stack overfow, I'll send a better
version of the set later.

About this particular patch, filters cannot actually know
what their owner are so my initial assert that it was not
used for that is even stronger imho: it probably shouldn't
be used to provide resources that are not available
otherwise, so it's mostly a caching mechanism.

I'll try to improve this commit message with a complete
diagnosis of the failure with next patch.

Thank for review,

Regards,
--
Alexandre Janniaux
Videolabs

On Thu, Jun 18, 2020 at 02:29:31PM +0200, Alexandre Janniaux wrote:
> Hi,
>
> It should not break anything given that it creates new
> pictures but stay in the correct format. It could omit
> some resources in HW context but I haven't seen any case
> like this and it would have been quite dangerous.
>
> However, it can create more resources than needed since
> it basically kills any pooling mechanism. But given that
> it makes some filters assert or segfault with very confusing
> errors (like "expected I420, got RGBA" for a CVPX -> I420
> filter), it seemed to me that it was much safer to revert
> any optimization so as to fix behaviour, and then eventually
> rework the optimization in a way it doesn't make anything
> crash.
>
> I'll test the snapshot with hw decoding, thank you for your
> review!
>
> Regards,
> --
> Alexandre Janniaux
> Videolabs
>
> On Thu, Jun 18, 2020 at 01:31:18PM +0200, Thomas Guillem wrote:
> > Are you sure this doesn't break anything? Did you test taking snapshot with linux/windows hw video decoding or inserting sw filter with hw decoding?
> >
> > Otherwise, OK for me but other opinions are welcome.
> >
> > On Thu, Jun 18, 2020, at 10:50, Alexandre Janniaux wrote:
> > > The previous new_buffer owner callback was creating picture with an
> > > incorrect format in filter_chains, leading to assertion in case of
> > > format change in a CVPX -> CVPX -> Software chroma chain, and in general
> > > crash in the filter pipeline because of filter chroma not matching with
> > > picture chroma.
> > >
> > > This patch completely remove the special handling for that but keep the
> > > callback function to add details about what is needed to implement this
> > > function correctly. It would create new pictures unconditionnally
> > > instead of forwarding the creation request to the filter owner.
> > > ---
> > >  src/misc/filter_chain.c | 29 ++++-------------------------
> > >  1 file changed, 4 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/src/misc/filter_chain.c b/src/misc/filter_chain.c
> > > index 7b1a0ff63b..dc3ae9abbb 100644
> > > --- a/src/misc/filter_chain.c
> > > +++ b/src/misc/filter_chain.c
> > > @@ -98,31 +98,10 @@ filter_chain_t *filter_chain_NewSPU( vlc_object_t
> > > *obj, const char *cap )
> > >  /** Chained filter picture allocator function */
> > >  static picture_t *filter_chain_VideoBufferNew( filter_t *filter )
> > >  {
> > > -    picture_t *pic;
> > > -    chained_filter_t *chained = container_of(filter, chained_filter_t,
> > > filter);
> > > -    if( chained->next != NULL )
> > > -    {
> > > -        // HACK as intermediate filters may not have the same video
> > > format as
> > > -        // the last one handled by the owner
> > > -        filter_owner_t saved_owner = filter->owner;
> > > -        filter->owner = (filter_owner_t) {};
> > > -        pic = filter_NewPicture( filter );
> > > -        filter->owner = saved_owner;
> > > -        if( pic == NULL )
> > > -            msg_Err( filter, "Failed to allocate picture" );
> > > -    }
> > > -    else
> > > -    {
> > > -        filter_chain_t *chain = filter->owner.sys;
> > > -
> > > -        // the owner of the chain requires pictures from the last
> > > filter to be grabbed from its callback
> > > -        /* XXX ugly */
> > > -        filter_owner_t saved_owner = filter->owner;
> > > -        filter->owner = chain->parent_video_owner;
> > > -        pic = filter_NewPicture( filter );
> > > -        filter->owner = saved_owner;
> > > -    }
> > > -    return pic;
> > > +    /* TODO: We don't handle special case for chained filter yet.
> > > +     *       It must be done while ensuring each filter gets the
> > > +     *       correct format for new picture. */
> > > +    return picture_NewFromFormat(&filter->fmt_out.video);
> > >  }
> > >
> > >  static vlc_decoder_device *
> > > filter_chain_HoldDecoderDevice(vlc_object_t *o, void *sys)
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel