[vlc-devel] [PATCH] picture: always copy context when cloning

[Alexandre Janniaux]

Hi,

I can wait for the patchset using this yes.

Just for info here is how I use it:

When creating the video context
+ I allocate N output pictures
+ I create N context, one for each picture
+ I call picture_pool_New on my pictures

For the picture context
+ the _copy_ operation hold my video context
+ the _destroy_ operation release my video context

And for the video context
+ the _destroy_ operation destroy the pool, removes the context
  from the pictures, free them and release the pictures.

Regards,
--
Alexandre Janniaux
Videolabs

On Wed, Jul 01, 2020 at 09:09:49AM +0200, Steve Lhomme wrote:
> This could work but I'd prefer with an example of using this feature
> (calling picture_pool_New() with pictures that already have a
> picture_context_t) to see if it's going to lead to leaks of the
> picture_pool. Right now the ref counting is done outside of the picture_pool
> and the last clone released releases the pool.
>
> On 2020-06-30 19:50, Alexandre Janniaux wrote:
> > picture_Clone was copying the original picture context into the new
> > clone while picture_pool_Clone wasn't. As it is possible to create a
> > pool from existing pictures, it's quite handy to be able to associate
> > the graphical resources needed for each picture to their context so
> > that it can be retrieved after a call to picture_pool_Wait/Get.
> >
> > The rationale is that because resources like pixels are shared between
> > the original picture and the clone, we should probably do the same with
> > picture_context and have a unified way for picture producer to
> > reference such resources.
> >
> > This patch remove the special handling in picture_Clone and move it to
> > picture_InternalClone for that reason.
> > ---
> >   src/misc/picture.c      | 11 +++++++----
> >   src/misc/picture_pool.c |  6 ++++--
> >   2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/misc/picture.c b/src/misc/picture.c
> > index e00b1303253..047d4d22477 100644
> > --- a/src/misc/picture.c
> > +++ b/src/misc/picture.c
> > @@ -440,18 +440,21 @@ picture_t *picture_InternalClone(picture_t *picture,
> >       picture_t *clone = picture_NewFromResource(&picture->format, &res);
> >       if (likely(clone != NULL)) {
> >           ((picture_priv_t *)clone)->gc.opaque = opaque;
> > +
> > +        /* The picture context is responsible for potentially holding the
> > +         * video context attached to the picture if needed. */
> > +        if (picture->context != NULL)
> > +            clone->context = picture->context->copy(picture->context);
> > +
> >           picture_Hold(picture);
> >       }
> > +
> >       return clone;
> >   }
> >   picture_t *picture_Clone(picture_t *picture)
> >   {
> >       picture_t *clone = picture_InternalClone(picture, picture_DestroyClone, picture);
> > -    if (likely(clone != NULL)) {
> > -        if (picture->context != NULL)
> > -            clone->context = picture->context->copy(picture->context);
> > -    }
> >       return clone;
> >   }
> > diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> > index 5f0f41eba8e..1b979d5a23a 100644
> > --- a/src/misc/picture_pool.c
> > +++ b/src/misc/picture_pool.c
> > @@ -90,8 +90,10 @@ static picture_t *picture_pool_ClonePicture(picture_pool_t *pool,
> >       picture_t *picture = pool->picture[offset];
> >       uintptr_t sys = ((uintptr_t)pool) + offset;
> > -    return picture_InternalClone(picture, picture_pool_ReleasePicture,
> > -                                 (void*)sys);
> > +    picture_t *clone = picture_InternalClone(picture,
> > +                                             picture_pool_ReleasePicture,
> > +                                             (void*)sys);
> > +    return clone;
> >   }
> >   picture_pool_t *picture_pool_New(unsigned count, picture_t *const *tab)
> > --
> > 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