[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 10:00:38 CET 2020
Hi,
On Mon, Nov 09, 2020 at 09:15:06AM +0100, Steve Lhomme wrote:
> On 2020-11-09 9:08, Alexandre Janniaux wrote:
> > 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. :)
>
> I'm replying to you. A drain is not needed here. You just refcount the pool
> and let it die with the last picture.
I meant, I know how the picture pool is working, I've even mentioned
what you're saying in my original mail. My suggestion wasn't about
this. ;)
Refcounted is handled correctly, however releasing pictures that won't
be used anymore is deferred to the release of the last picture in the
pool (+ the pool itself, but it doesn't matter when switching pool).
My point was that instead of adding a new mechanism and complexifying
the pool API and picture handling, creating a new pool is much simpler
just like Rémi highlighted it. The only problem you could get with that
is additional picture consumption because of the unused pictures but it
would be avoidable with what I suggested or another ad-hoc mechanism
without adding complexity in the API. Maybe such mechanism is not
needed but then I don't understand why format change in picture pool
would be needed at all.
Regards,
--
Alexandre Janniaux
Videolabs
Regards,
--
Alexandre Janniaux
Videolabs
> > > > 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
> > _______________________________________________
> > 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