[vlc-devel] [PATCH 1/4] vmem: fixed picture pool use

[Rémi Denis-Courmont]

Le 2016-01-30 10:27, Sergey Radionov a écrit :
> ---
>  modules/video_output/vmem.c | 36 
> +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/modules/video_output/vmem.c 
> b/modules/video_output/vmem.c
> index 5a6d869..ce9584d 100644
> --- a/modules/video_output/vmem.c
> +++ b/modules/video_output/vmem.c
> @@ -3,6 +3,7 @@
>
> 
> *****************************************************************************
>   * Copyright (C) 2008 VLC authors and VideoLAN
>   * Copyrgiht (C) 2010 Rémi Denis-Courmont
> + * Copyrgiht (C) 2016 Sergey Radionov <rsatom at gmail.com>
>   *
>   * Authors: Sam Hocevar <sam at zoy.org>
>   *
> @@ -109,32 +110,37 @@ static void           Prepare(vout_display_t *,
> picture_t *, subpicture_t *);
>  static void           Display(vout_display_t *, picture_t *,
> subpicture_t *);
>  static int            Control(vout_display_t *, int, va_list);
>
> -static void Lock(void *data, picture_t *pic)
> +static int Lock(picture_t *pic)
>  {
> -    vout_display_sys_t *sys = data;
>      picture_sys_t *picsys = pic->p_sys;
> +    vout_display_sys_t *sys = picsys->sys;
>      void *planes[PICTURE_PLANE_MAX];
> +    memset(planes, 0, sizeof(planes));

memset()ing pointers does not make much sense.

>
>      picsys->id = sys->lock(sys->opaque, planes);
>
> +    for (int i = 0; i < pic->i_planes; ++i) {
> +        if (!planes[i])
> +            return VLC_EGENERIC;

Wut? Why?

> +    }
> +
>      for (int i = 0; i < pic->i_planes; i++)
>          pic->p[i].p_pixels = planes[i];
> +
> +    return VLC_SUCCESS;
>  }
>
> -static void Unlock(void *data, picture_t *pic)
> +static void Unlock(picture_t *pic)
>  {
> -    vout_display_sys_t *sys = data;
>      picture_sys_t *picsys = pic->p_sys;
> +    vout_display_sys_t *sys = picsys->sys;
>      void *planes[PICTURE_PLANE_MAX];
>
> -    assert(!picture_IsReferenced(pic));
> -
>      for (int i = 0; i < pic->i_planes; i++)
>          planes[i] = pic->p[i].p_pixels;
>
>      if (sys->unlock != NULL)
>          sys->unlock(sys->opaque, picsys->id, planes);
> -
>  }
>
>
> 
> /*****************************************************************************
> @@ -269,7 +275,6 @@ static void Close(vlc_object_t *object)
>
>      if (sys->pool)
>      {
> -        picture_pool_Enum(sys->pool, Unlock, sys);
>          picture_pool_Release(sys->pool);
>      }
>      free(sys);
> @@ -315,23 +320,29 @@ static picture_pool_t *Pool(vout_display_t *vd,
> unsigned count)
>      }
>
>      /* */
> -    sys->pool = picture_pool_New(count, pictures);
> +    picture_pool_configuration_t cfg = {
> +        .picture_count = count,
> +        .picture = pictures,
> +        .lock = Lock,
> +        .unlock = Unlock

lock and unlock callbacks are quite broken, and mostly unfixable or not 
worth fixing. I would rather not use them.

> +    };
> +
> +    sys->pool = picture_pool_NewExtended(&cfg);
>      if (!sys->pool) {
>          for (unsigned i = 0; i < count; i++)
>              picture_Release(pictures[i]);
>      }
>
> -    picture_pool_Enum(sys->pool, Lock, sys);
>      return sys->pool;
>  }
>
>  static void Prepare(vout_display_t *vd, picture_t *pic, subpicture_t
> *subpic)
>  {
> -    Unlock(vd->sys, pic);
> +    VLC_UNUSED(vd);
> +    VLC_UNUSED(pic);
>      VLC_UNUSED(subpic);
>  }
>
> -
>  static void Display(vout_display_t *vd, picture_t *pic, subpicture_t
> *subpic)
>  {
>      vout_display_sys_t *sys = vd->sys;
> @@ -339,7 +350,6 @@ static void Display(vout_display_t *vd, picture_t
> *pic, subpicture_t *subpic)
>      if (sys->display != NULL)
>          sys->display(sys->opaque, pic->p_sys->id);
>
> -    Lock(sys, pic);
>      picture_Release(pic);
>      VLC_UNUSED(subpic);
>  }

-- 
Rémi Denis-Courmont
http://www.remlab.net/