[vlc-devel] [PATCH 2/7] picture_pool: add the video format requested when waiting for a picture
Steve Lhomme
robux4 at ycbcr.xyz
Mon Nov 9 09:15:06 CET 2020
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.
>>> 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
>
More information about the vlc-devel
mailing list