[vlc-devel] [PATCH 2/7] picture_pool: add the video format requested when waiting for a picture
Alexandre Janniaux
ajanni at videolabs.io
Mon Nov 9 09:08:52 CET 2020
Hi,
On Mon, Nov 09, 2020 at 09:07:02AM +0100, Steve Lhomme wrote:
> On 2020-11-06 17:02, Alexandre Janniaux wrote:
> > Hi,
> >
> > I'm not sure this is the correct level of abstraction
> > regarding how we discussed this originally. From what
> > I understood, we want to change the pool when the format
> > change, not adapt the previous pool. The picture_pool
> > even facilitates this by being reference-counted.
> >
> > I guess your main issue with this is that previous pictures
> > would not be released until all pictures are released.
> > Maybe picture_pool_Release could implement some sort of
> > drain to release any current and future requeued picture
> > to handle this case smoothly instead? It would highly
> > simplify the API for handling this.
>
> This is already handled in the other pool helpers. They are refcounted and
> released when a new one is needed (which is rare, as it's only when the
> surface/buffer characteristics change). When the last picture of the defunct
> pool is released, the pool resources are released as well.
I'm not sure to understand whether your answer relates to
mine or not, it looks like we have a misunderstanding here. :)
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Fri, Nov 06, 2020 at 03:42:09PM +0100, Steve Lhomme wrote:
> > > In push mode it's convenient to keep the same pool of buffers when only small
> > > changes to the video format occur.
> > >
> > > Passing NULL for the video format means no change to the original format is
> > > required.
> > > ---
> > > include/vlc_picture_pool.h | 2 +-
> > > modules/hw/nvdec/hw_pool.c | 4 ++--
> > > modules/hw/nvdec/hw_pool.h | 2 +-
> > > modules/hw/nvdec/nvdec.c | 2 +-
> > > modules/hw/vaapi/chroma.c | 2 +-
> > > modules/hw/vaapi/filters.c | 2 +-
> > > src/input/decoder.c | 2 +-
> > > src/misc/picture.c | 7 ++++---
> > > src/misc/picture.h | 2 +-
> > > src/misc/picture_pool.c | 13 +++++++------
> > > src/test/picture_pool.c | 2 +-
> > > src/video_output/video_output.c | 2 +-
> > > 12 files changed, 22 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/vlc_picture_pool.h b/include/vlc_picture_pool.h
> > > index aa5883ee521..8e325392d2c 100644
> > > --- a/include/vlc_picture_pool.h
> > > +++ b/include/vlc_picture_pool.h
> > > @@ -102,7 +102,7 @@ VLC_API picture_t * picture_pool_Get( picture_pool_t * ) VLC_USED;
> > > *
> > > * @note This function is thread-safe.
> > > */
> > > -VLC_API picture_t *picture_pool_Wait(picture_pool_t *) VLC_USED;
> > > +VLC_API picture_t *picture_pool_Wait(picture_pool_t *, const video_format_t *) VLC_USED;
> > >
> > > /**
> > > * Cancel the picture pool.
> > > diff --git a/modules/hw/nvdec/hw_pool.c b/modules/hw/nvdec/hw_pool.c
> > > index 63c6f1911f3..3d117d66ca2 100644
> > > --- a/modules/hw/nvdec/hw_pool.c
> > > +++ b/modules/hw/nvdec/hw_pool.c
> > > @@ -103,9 +103,9 @@ error:
> > > return NULL;
> > > }
> > >
> > > -picture_t* nvdec_pool_Wait(nvdec_pool_t *pool)
> > > +picture_t* nvdec_pool_Wait(nvdec_pool_t *pool, const video_format_t *fmt)
> > > {
> > > - picture_t *pic = picture_pool_Wait(pool->picture_pool);
> > > + picture_t *pic = picture_pool_Wait(pool->picture_pool, fmt);
> > > if (!pic)
> > > return NULL;
> > >
> > > diff --git a/modules/hw/nvdec/hw_pool.h b/modules/hw/nvdec/hw_pool.h
> > > index 59362116b3d..15bedfde868 100644
> > > --- a/modules/hw/nvdec/hw_pool.h
> > > +++ b/modules/hw/nvdec/hw_pool.h
> > > @@ -45,4 +45,4 @@ void nvdec_pool_Release(nvdec_pool_t *);
> > > *
> > > * The picture.p_sys is always NULL.
> > > */
> > > -picture_t* nvdec_pool_Wait(nvdec_pool_t *);
> > > +picture_t* nvdec_pool_Wait(nvdec_pool_t *, const video_format_t *);
> > > diff --git a/modules/hw/nvdec/nvdec.c b/modules/hw/nvdec/nvdec.c
> > > index ebed90be51b..f16702f89ae 100644
> > > --- a/modules/hw/nvdec/nvdec.c
> > > +++ b/modules/hw/nvdec/nvdec.c
> > > @@ -419,7 +419,7 @@ static int CUDAAPI HandlePictureDisplay(void *p_opaque, CUVIDPARSERDISPINFO *p_d
> > >
> > > if ( is_nvdec_opaque(p_dec->fmt_out.video.i_chroma) )
> > > {
> > > - p_pic = nvdec_pool_Wait(p_sys->out_pool);
> > > + p_pic = nvdec_pool_Wait(p_sys->out_pool, &p_dec->fmt_out.video);
> > > if (unlikely(p_pic == NULL))
> > > return 0;
> > > pic_context_nvdec_t *picctx = NVDEC_PICCONTEXT_FROM_PICCTX(p_pic->context);
> > > diff --git a/modules/hw/vaapi/chroma.c b/modules/hw/vaapi/chroma.c
> > > index f43e0dc1cd0..ff33c4a2f26 100644
> > > --- a/modules/hw/vaapi/chroma.c
> > > +++ b/modules/hw/vaapi/chroma.c
> > > @@ -242,7 +242,7 @@ UploadSurface(filter_t *filter, picture_t *src)
> > > VADisplay const va_dpy = p_sys->dpy;
> > > VAImage dest_img;
> > > void * dest_buf;
> > > - picture_t * dest_pic = picture_pool_Wait(p_sys->dest_pics);
> > > + picture_t * dest_pic = picture_pool_Wait(p_sys->dest_pics, NULL);
> > >
> > > if (!dest_pic)
> > > {
> > > diff --git a/modules/hw/vaapi/filters.c b/modules/hw/vaapi/filters.c
> > > index bd933ac8c63..9174e21cd4d 100644
> > > --- a/modules/hw/vaapi/filters.c
> > > +++ b/modules/hw/vaapi/filters.c
> > > @@ -211,7 +211,7 @@ Filter(filter_t * filter, picture_t * src,
> > > {
> > > filter_sys_t *const filter_sys = filter->p_sys;
> > > VABufferID pipeline_buf = VA_INVALID_ID;
> > > - picture_t *const dest = picture_pool_Wait(filter_sys->dest_pics);
> > > + picture_t *const dest = picture_pool_Wait(filter_sys->dest_pics, NULL);
> > > if (!dest)
> > > return NULL;
> > >
> > > diff --git a/src/input/decoder.c b/src/input/decoder.c
> > > index 77ccd67e530..1610d300ae2 100644
> > > --- a/src/input/decoder.c
> > > +++ b/src/input/decoder.c
> > > @@ -620,7 +620,7 @@ static picture_t *ModuleThread_NewVideoBuffer( decoder_t *p_dec )
> > > assert( p_owner->p_vout );
> > > assert( p_owner->out_pool );
> > >
> > > - picture_t *pic = picture_pool_Wait( p_owner->out_pool );
> > > + picture_t *pic = picture_pool_Wait( p_owner->out_pool, &p_dec->fmt_out.video );
> > > if (pic)
> > > picture_Reset( pic );
> > > return pic;
> > > diff --git a/src/misc/picture.c b/src/misc/picture.c
> > > index e00b1303253..6902b9a8fcd 100644
> > > --- a/src/misc/picture.c
> > > +++ b/src/misc/picture.c
> > > @@ -423,7 +423,7 @@ static void picture_DestroyClone(picture_t *clone)
> > > picture_Release(picture);
> > > }
> > >
> > > -picture_t *picture_InternalClone(picture_t *picture,
> > > +picture_t *picture_InternalClone(picture_t *picture, const video_format_t *fmt,
> > > void (*pf_destroy)(picture_t *), void *opaque)
> > > {
> > > picture_resource_t res = {
> > > @@ -437,7 +437,7 @@ picture_t *picture_InternalClone(picture_t *picture,
> > > res.p[i].i_pitch = picture->p[i].i_pitch;
> > > }
> > >
> > > - picture_t *clone = picture_NewFromResource(&picture->format, &res);
> > > + picture_t *clone = picture_NewFromResource(fmt ? fmt : &picture->format, &res);
> > > if (likely(clone != NULL)) {
> > > ((picture_priv_t *)clone)->gc.opaque = opaque;
> > > picture_Hold(picture);
> > > @@ -447,7 +447,8 @@ picture_t *picture_InternalClone(picture_t *picture,
> > >
> > > picture_t *picture_Clone(picture_t *picture)
> > > {
> > > - picture_t *clone = picture_InternalClone(picture, picture_DestroyClone, picture);
> > > + picture_t *clone = picture_InternalClone(picture, NULL,
> > > + picture_DestroyClone, picture);
> > > if (likely(clone != NULL)) {
> > > if (picture->context != NULL)
> > > clone->context = picture->context->copy(picture->context);
> > > diff --git a/src/misc/picture.h b/src/misc/picture.h
> > > index 684eb2ada69..a815bb8a316 100644
> > > --- a/src/misc/picture.h
> > > +++ b/src/misc/picture.h
> > > @@ -36,4 +36,4 @@ typedef struct
> > > void *picture_Allocate(int *, size_t);
> > > void picture_Deallocate(int, void *, size_t);
> > >
> > > -picture_t * picture_InternalClone(picture_t *, void (*pf_destroy)(picture_t *), void *);
> > > +picture_t * picture_InternalClone(picture_t *, const video_format_t *, void (*pf_destroy)(picture_t *), void *);
> > > diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> > > index d2df4bf2eb0..7438e5f1d6a 100644
> > > --- a/src/misc/picture_pool.c
> > > +++ b/src/misc/picture_pool.c
> > > @@ -85,13 +85,14 @@ static void picture_pool_ReleaseClone(picture_t *clone)
> > > }
> > >
> > > static picture_t *picture_pool_ClonePicture(picture_pool_t *pool,
> > > - unsigned offset)
> > > + unsigned offset,
> > > + const video_format_t *fmt)
> > > {
> > > picture_t *picture = pool->picture[offset];
> > > uintptr_t sys = ((uintptr_t)pool) + offset;
> > >
> > > - picture_t *clone = picture_InternalClone(picture, picture_pool_ReleaseClone,
> > > - (void*)sys);
> > > + picture_t *clone = picture_InternalClone(picture, fmt,
> > > + picture_pool_ReleaseClone, (void*)sys);
> > > if (clone != NULL) {
> > > assert(!picture_HasChainedPics(clone));
> > > atomic_fetch_add_explicit(&pool->refs, 1, memory_order_relaxed);
> > > @@ -191,14 +192,14 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
> > > vlc_mutex_unlock(&pool->lock);
> > > available &= ~(1ULL << i);
> > >
> > > - return picture_pool_ClonePicture(pool, i);
> > > + return picture_pool_ClonePicture(pool, i, NULL);
> > > }
> > >
> > > vlc_mutex_unlock(&pool->lock);
> > > return NULL;
> > > }
> > >
> > > -picture_t *picture_pool_Wait(picture_pool_t *pool)
> > > +picture_t *picture_pool_Wait(picture_pool_t *pool, const video_format_t *fmt)
> > > {
> > > vlc_mutex_lock(&pool->lock);
> > > assert(pool->refs > 0);
> > > @@ -217,7 +218,7 @@ picture_t *picture_pool_Wait(picture_pool_t *pool)
> > > pool->available &= ~(1ULL << i);
> > > vlc_mutex_unlock(&pool->lock);
> > >
> > > - return picture_pool_ClonePicture(pool, i);
> > > + return picture_pool_ClonePicture(pool, i, fmt);
> > > }
> > >
> > > void picture_pool_Cancel(picture_pool_t *pool, bool canceled)
> > > diff --git a/src/test/picture_pool.c b/src/test/picture_pool.c
> > > index 9b43c25fdd4..e4b5491c241 100644
> > > --- a/src/test/picture_pool.c
> > > +++ b/src/test/picture_pool.c
> > > @@ -75,7 +75,7 @@ static void test(bool zombie)
> > > picture_Release(pics[i]);
> > >
> > > for (unsigned i = 0; i < PICTURES; i++) {
> > > - pics[i] = picture_pool_Wait(pool);
> > > + pics[i] = picture_pool_Wait(pool, NULL);
> > > assert(pics[i] != NULL);
> > > }
> > >
> > > diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> > > index 20e569e173c..de515b4546e 100644
> > > --- a/src/video_output/video_output.c
> > > +++ b/src/video_output/video_output.c
> > > @@ -468,7 +468,7 @@ picture_t *vout_GetPicture(vout_thread_t *vout)
> > > {
> > > vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
> > > assert(!sys->dummy);
> > > - picture_t *picture = picture_pool_Wait(sys->sys.private.display_pool);
> > > + picture_t *picture = picture_pool_Wait(sys->sys.private.display_pool, NULL);
> > > if (likely(picture != NULL)) {
> > > picture_Reset(picture);
> > > video_format_CopyCropAr(&picture->format, &sys->original);
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> 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