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

[Steve Lhomme]

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
>