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

[Thomas Guillem]

On Tue, Jul 12, 2016, at 10:19, Rémi Denis-Courmont wrote:
> 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.

Ok, I see. There is no way to be sure that the buffer will be unlocked
when I release the picture from the Display callback since the picture
can be hold by something else.

One way to fix it is to remove the warning comment:
 * \warning A picture buffer is unlocked after the picture is decoded,
 * but before the picture is displayed.

Since we can't know when a picture will be unlocked, but we are sure it
will be unlocked before it's locked again.

> 
> > 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/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel