[vlc-devel] [PATCH] vmem: lock/unlock pictures from pool callback
Thomas Guillem
thomas at gllm.fr
Mon Jul 18 16:25:46 CEST 2016
On Wed, Jul 13, 2016, at 16:53, Rémi Denis-Courmont wrote:
> 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.
I don't understand your second sequence.
>
> 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
1/ My current patch break the API promise. We should say that we can't
know when the unlock callback is called (but it must be called before
the lock() of the same picture id).
> or
> 2) copy memory
A memcpy seems mandatory. Either the core does it, or the user of the
API (I prefer the last).
> 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/
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list