[vlc-devel] [PATCH] picture: refactor to use common code in cloning

[Rémi Denis-Courmont]

	Hello,

Le lauantaina 3. maaliskuuta 2018, 13.39.48 EET Matthew Whitworth a écrit :
> refactor to remove dup clone code per TODO comment
> ---
>  src/misc/picture.c      | 26 ++++++++++++++++++--------
>  src/misc/picture.h      |  2 ++
>  src/misc/picture_pool.c | 19 ++-----------------
>  3 files changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/src/misc/picture.c b/src/misc/picture.c
> index 7506e47725..ace931f34a 100644
> --- a/src/misc/picture.c
> +++ b/src/misc/picture.c
> @@ -403,27 +403,37 @@ static void picture_DestroyClone(picture_t *clone)
>      picture_Release(picture);
>  }
> 
> -picture_t *picture_Clone(picture_t *picture)
> +picture_t *picture_CloneFromResourceTemplate(picture_t *picture,

That's a rather poor choice of name IMO, given that there is no 
picture_resource_t parameter.

> picture_sys_t *sys, void (*pf_destroy)(picture_t *), void* opaque) {
> -    /* TODO: merge common code with picture_pool_ClonePicture(). */
>      picture_resource_t res = {
> -        .p_sys = picture->p_sys,
> -        .pf_destroy = picture_DestroyClone,
> +        .p_sys = sys,
> +        .pf_destroy = pf_destroy
>      };
> 
> +    picture_t* clone;
> +
>      for (int i = 0; i < picture->i_planes; i++) {
>          res.p[i].p_pixels = picture->p[i].p_pixels;
>          res.p[i].i_lines = picture->p[i].i_lines;
>          res.p[i].i_pitch = picture->p[i].i_pitch;
>      }
> 
> -    picture_t *clone = picture_NewFromResource(&picture->format, &res);
> +    clone = picture_NewFromResource(&picture->format, &res);
> +
>      if (likely(clone != NULL)) {

Simpler to just return NULL on the failure case and deindent. Ditto below.

> -        ((picture_priv_t *)clone)->gc.opaque = picture;
> +        ((picture_priv_t *)clone)->gc.opaque = opaque;
>          picture_Hold(picture);
> +    }
> +
> +    return clone;
> +}
> +
> +picture_t *picture_Clone(picture_t *picture)
> +{
> +    picture_t *clone = picture_CloneFromResourceTemplate(picture,
> picture->p_sys, picture_DestroyClone, picture);
> 
> -        if (picture->context != NULL)
> -            clone->context = picture->context->copy(picture->context);
> +    if (likely(clone != NULL) && picture->context != NULL) {
> +        clone->context = picture->context->copy(picture->context);
>      }
>      return clone;
>  }
> diff --git a/src/misc/picture.h b/src/misc/picture.h
> index 70ee64878d..b1dd46572f 100644
> --- a/src/misc/picture.h
> +++ b/src/misc/picture.h
> @@ -32,3 +32,5 @@ typedef struct
>          void *opaque;
>      } gc;
>  } picture_priv_t;
> +
> +picture_t *picture_CloneFromResourceTemplate(picture_t *picture,
> picture_sys_t *sys, void (*pf_destroy)(picture_t *), void* opaque); diff
> --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> index 74712c81f5..a6452251f1 100644
> --- a/src/misc/picture_pool.c
> +++ b/src/misc/picture_pool.c
> @@ -95,24 +95,9 @@ static picture_t
> *picture_pool_ClonePicture(picture_pool_t *pool, unsigned offset)
>  {
>      picture_t *picture = pool->picture[offset];
> -    uintptr_t sys = ((uintptr_t)pool) + offset;
> -    picture_resource_t res = {
> -        .p_sys = picture->p_sys,
> -        .pf_destroy = picture_pool_ReleasePicture,
> -    };
> -
> -    for (int i = 0; i < picture->i_planes; i++) {
> -        res.p[i].p_pixels = picture->p[i].p_pixels;
> -        res.p[i].i_lines = picture->p[i].i_lines;
> -        res.p[i].i_pitch = picture->p[i].i_pitch;
> -    }
> +    void* opaque = (void *)(((uintptr_t)pool) + offset);
> 
> -    picture_t *clone = picture_NewFromResource(&picture->format, &res);
> -    if (likely(clone != NULL)) {
> -        ((picture_priv_t *)clone)->gc.opaque = (void *)sys;
> -        picture_Hold(picture);
> -    }
> -    return clone;
> +    return picture_CloneFromResourceTemplate(picture, picture->p_sys,
> picture_pool_ReleasePicture, opaque); }
> 
>  picture_pool_t *picture_pool_NewExtended(const picture_pool_configuration_t
> *cfg)


-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/