[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 10:29:28 CET 2020
On 2020-11-09 10:00, Alexandre Janniaux wrote:
> 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).
Ah yes, I always forget about that. The issue with this "bare" pool
release is that the related resources (video context, decoder device)
and not released when the pool is released. Hence the nvdec_pool_t API
(which should be renamed hw_pool_t) which does the same as the picture
pool but also releases the related resources. That's why I consider the
picture pool is not releasing things properly with the last picture.
libavcodec and the va modules use the picture pool from decoder helper
with custom destructors. I'm not sure all va modules can handle a re-set
of the surfaces during playback.
This is also a problem when metadata like HDR metadata change, aspect
ratio, etc. The new frames may reference older decoded frames (they may
still be in the same GOP). If you remove them the decoding may fail or
produce garbage. At least in DXVA there's no way to know when a surface
is referenced or not. So it's better to keep the surfaces when we can
(on top of not using more memory unnecessarily).
> 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.
It's the other way around. The format is not needed at all in the
picture pool, only when grabbing a buffer/surface from it. It's just
that the API deals with pictures and so there is a video_format_t
attached by default.
> 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
> _______________________________________________
> 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