[vlc-devel] [PATCH] vmem: lock/unlock pictures from pool callback
Rémi Denis-Courmont
remi at remlab.net
Wed Jul 13 16:53:03 CEST 2016
Le tiistaina 12. heinäkuuta 2016, 10.47.03 EEST Thomas Guillem a écrit :
> 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.
This is but a clarification. As far as I can tell, the rest of the
documentation also imply that behaviour, and some applications expect it.
The problem is, we can invoke unlock in prepare() insofar as VLC will not
modify the picture buffer past that point. But we cannot invoke unlock safely
ever insofar as VLC will not access (read) the picture buffer, until after the
pool is destroyed and all pending references released.
The LibVLC API expects the latter, and presumably some applications rely on
it. So I think the choices boil down to:
1) break the API promise
or
2) copy memory
or
both.
> 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);
> > >
> > > }
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list