[vlc-devel] [PATCH] vmem: lock/unlock pictures from pool callback

[Rémi Denis-Courmont]

Le 2016-07-12 09:59, Thomas Guillem a écrit :
> Pictures are now locked just before decoding, and unlocked before the 
> picture
> is displayed. This is the behavior expected by users and by the
> libvlc_video_lock_cb/libvlc_video_unlock_cb documentation.

I don't think that this is generally feasible without memory copying 
inside vmem, as the picture buffers might have other references. 
Specifically, I don't think the patch makes the code do what this 
comment implies.

> Furthermore, lock callbacks can now fail (planes[0] is checked before
> continuing).
> ---
>  modules/video_output/vmem.c | 73
> +++++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/modules/video_output/vmem.c 
> b/modules/video_output/vmem.c
> index 64aefa1..fa05360 100644
> --- a/modules/video_output/vmem.c
> +++ b/modules/video_output/vmem.c
> @@ -83,6 +83,9 @@ vlc_module_end()
>
> 
> *****************************************************************************/
>  struct picture_sys_t {
>      void *id;
> +    void *opaque;
> +    void *(*lock)(void *sys, void **plane);
> +    void (*unlock)(void *sys, void *id, void *const *plane);
>  };
>
>  /* NOTE: the callback prototypes must match those of LibVLC */
> @@ -107,20 +110,6 @@ static picture_pool_t *Pool  (vout_display_t *,
> unsigned);
>  static void           Display(vout_display_t *, picture_t *,
> subpicture_t *);
>  static int            Control(vout_display_t *, int, va_list);
>
> -static void Unlock(void *data, picture_t *pic)
> -{
> -    vout_display_sys_t *sys = data;
> -    picture_sys_t *picsys = pic->p_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;
> -
> -    sys->unlock(sys->opaque, picsys->id, planes);
> -}
> -
>
> 
> /*****************************************************************************
>   * Open: allocates video thread
>
> 
> *****************************************************************************
> @@ -252,14 +241,39 @@ static void Close(vlc_object_t *object)
>          sys->cleanup(sys->opaque);
>
>      if (sys->pool)
> -    {
> -        if (sys->unlock != NULL)
> -            picture_pool_Enum(sys->pool, Unlock, sys);
>          picture_pool_Release(sys->pool);
> -    }
>      free(sys);
>  }
>
> +static int PoolLockPicture(picture_t *pic)
> +{
> +    picture_sys_t *picsys = pic->p_sys;
> +    void *planes[PICTURE_PLANE_MAX] = {};
> +
> +    picsys->id = picsys->lock(picsys->opaque, planes);
> +    if (planes[0] == NULL)
> +        return -1;
> +
> +    for (int i = 0; i < pic->i_planes; i++)
> +        pic->p[i].p_pixels = planes[i];
> +    return 0;
> +
> +}
> +
> +static void PoolUnlockPicture(picture_t *pic)
> +{
> +    picture_sys_t *picsys = pic->p_sys;
> +
> +    if (picsys->unlock != NULL)
> +    {
> +        void *planes[PICTURE_PLANE_MAX] = {};
> +        for (int i = 0; i < pic->i_planes; i++)
> +            planes[i] = pic->p[i].p_pixels;
> +
> +        picsys->unlock(picsys->opaque, picsys->id, planes);
> +    }
> +}
> +
>  static picture_pool_t *Pool(vout_display_t *vd, unsigned count)
>  {
>      vout_display_sys_t *sys = vd->sys;
> @@ -280,14 +294,14 @@ static picture_pool_t *Pool(vout_display_t *vd,
> unsigned count)
>              break;
>          }
>          picsys->id = NULL;
> +        picsys->opaque = sys->opaque;
> +        picsys->lock = sys->lock;
> +        picsys->unlock = sys->unlock;
>
>          picture_resource_t rsc = { .p_sys = picsys };
> -        void *planes[PICTURE_PLANE_MAX];
> -
> -        picsys->id = sys->lock(sys->opaque, planes);
>
>          for (unsigned i = 0; i < PICTURE_PLANE_MAX; i++) {
> -            rsc.p[i].p_pixels = planes[i];
> +            rsc.p[i].p_pixels = NULL;
>              rsc.p[i].i_lines  = sys->lines[i];
>              rsc.p[i].i_pitch  = sys->pitches[i];
>          }
> @@ -301,7 +315,13 @@ static picture_pool_t *Pool(vout_display_t *vd,
> unsigned count)
>      }
>
>      /* */
> -    sys->pool = picture_pool_New(count, pictures);
> +    picture_pool_configuration_t pool_cfg = {
> +        .picture_count = count,
> +        .picture = pictures,
> +        .lock = PoolLockPicture,
> +        .unlock = PoolUnlockPicture,
> +    };
> +    sys->pool = picture_pool_NewExtended(&pool_cfg);
>      if (!sys->pool) {
>          for (unsigned i = 0; i < count; i++)
>              picture_Release(pictures[i]);
> @@ -314,10 +334,13 @@ static void Display(vout_display_t *vd,
> picture_t *pic, subpicture_t *subpic)
>  {
>      vout_display_sys_t *sys = vd->sys;
>
> +    /* Unlock the picture buffer before the picture is displayed */
> +    void *id = pic->p_sys->id;
> +    picture_Release(pic);
> +
>      if (sys->display != NULL)
> -        sys->display(sys->opaque, pic->p_sys->id);
> +        sys->display(sys->opaque, id);
>
> -    picture_Release(pic);
>      VLC_UNUSED(subpic);
>  }

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