[vlc-devel] [PATCH 6/6] nvdec: add reference-count to the picture pool
Steve Lhomme
robux4 at ycbcr.xyz
Thu Mar 26 13:44:02 CET 2020
OK, this should be working as well. One of the current drawback of
picture pools is that the video_format_t is fixed. So you may have to
update the pictures coming out of it each time when the format changes.
It was especially a problem in the pull model from the vout where we had
no control of the display module. Now decoders don't even have to use a
pool of pictures anymore, only surfaces. Hardware decoders can cheaply
allocate pictures on the fly.
On 2020-03-26 12:26, Quentin Chateau wrote:
> Hi,
>
> I have a different point of view on the picture pool, please tell me if
> I misunderstood something.
>
> Comments below.
>
> On 26/03/2020 11:19, Steve Lhomme wrote:
>> It seems odd that the picture created by the pool is responsible to
>> release the pool when it's released.
> It seems to me that this is way picture_pool_t is designed: on-flight
> picture increment the pool refcount.
>> That's why I went with va_surface which holds the buffers, not the
>> pictures. The pictures are only managed by the pool.
> I did not go this way because I wanted to avoid coding yet another
> memory pool. This also avoid having two levels of pools, one for the
> pictures and one for the device buffers.
>>
>> Looking at the picture_Destroy() code it seems that it's not safe.
>> PictureDestroyContext destroys the context which will decrement the
>> pool refcount in your code. When it's the last picture, after the
>> decoder is closed, it will release the pool.
> The nvdec_pool_t will be destroyed, and picture_pool_Release will be
> called. But the picture_pool_t itself will not destroyed:
> picture_pool_Wait incremented the picture_pool_t refcount, and the call
> to picture_pool_ReleasePicture will perform the matching decrement. As
> you said, this function has not been called yet.
>> Then priv->gc.destroy is called which should call the
>> picture_pool_ReleasePicture() which will also free the pool.
> The picture_pool_ReleasePicture call will therefore free the pool, which
> was not free at the time nvdec_pool_t was freed.
>>
>> va_surface removes the pool from the equation. Only the buffers and
>> pictures exist. When the decoder is released the pool is release too.
>> Only the buffers and in-flight pictures still exist. The last picture
>> released releases the buffers.
>>
>> On 2020-03-25 18:57, quentin.chateau at deepskycorp.com wrote:
>>> From: Quentin Chateau <quentin.chateau at deepskycorp.com>
>>>
>>> ---
>>> modules/hw/nvdec/nvdec.c | 52 +++++++++++++++++++++++++++++++---------
>>> 1 file changed, 41 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/modules/hw/nvdec/nvdec.c b/modules/hw/nvdec/nvdec.c
>>> index d687f8ba3d..88c8709d81 100644
>>> --- a/modules/hw/nvdec/nvdec.c
>>> +++ b/modules/hw/nvdec/nvdec.c
>>> @@ -29,6 +29,7 @@
>>> #include <vlc_codec.h>
>>> #include <vlc_messages.h>
>>> #include <vlc_picture_pool.h>
>>> +#include <vlc_atomic.h>
>>> #define FFNV_LOG_FUNC(logctx, msg, ...)
>>> msg_Err((vlc_object_t*)logctx, msg, __VA_ARGS__)
>>> #define FFNV_DEBUG_LOG_FUNC(logctx, msg, ...)
>>> msg_Dbg((vlc_object_t*)logctx, msg, __VA_ARGS__)
>>> @@ -83,8 +84,15 @@ typedef struct nvdec_pool_t {
>>> CUdeviceptr device_ptrs[MAX_POOL_SIZE];
>>> picture_pool_t *picture_pool;
>>> +
>>> + vlc_atomic_rc_t rc;
>>> } nvdec_pool_t;
>>> +typedef struct pic_pool_context_nvdec_t {
>>> + pic_context_nvdec_t ctx;
>>> + nvdec_pool_t *pool;
>>> +} pic_pool_context_nvdec_t;
>>> +
>>> typedef struct nvdec_ctx {
>>> decoder_device_nvdec_t *devsys;
>>> CuvidFunctions *cuvidFunctions;
>>> @@ -115,6 +123,9 @@ typedef struct nvdec_ctx {
>>> #define CALL_CUVID(func, ...) CudaCheckErr(VLC_OBJECT(p_dec),
>>> p_sys->devsys->cudaFunctions,
>>> p_sys->cuvidFunctions->func(__VA_ARGS__), #func)
>>> #define CALL_CUDA_POOL(func, ...)
>>> pool->nvdec_dev->cudaFunctions->func(__VA_ARGS__)
>>> +#define NVDEC_PICPOOLCTX_FROM_PICCTX(pic_ctx) \
>>> + container_of(NVDEC_PICCTX_FROM_PICCTX(pic_ctx),
>>> pic_pool_context_nvdec_t, ctx)
>>> +
>>> static void nvdec_pool_Destroy(nvdec_pool_t *pool)
>>> {
>>> @@ -127,6 +138,19 @@ static void nvdec_pool_Destroy(nvdec_pool_t *pool)
>>> vlc_video_context_Release(pool->vctx);
>>> }
>>> +static void nvdec_pool_AddRef(nvdec_pool_t *pool)
>>> +{
>>> + vlc_atomic_rc_inc(&pool->rc);
>>> +}
>>> +
>>> +static void nvdec_pool_Release(nvdec_pool_t *pool)
>>> +{
>>> + if (!vlc_atomic_rc_dec(&pool->rc))
>>> + return;
>>> +
>>> + nvdec_pool_Destroy(pool);
>>> +}
>>> +
>>> static nvdec_pool_t* nvdec_pool_Create(vlc_video_context *vctx,
>>> const video_format_t *fmt,
>>> size_t buffer_pitch,
>>> @@ -176,6 +200,7 @@ static nvdec_pool_t*
>>> nvdec_pool_Create(vlc_video_context *vctx,
>>> pool->vctx = vctx;
>>> vlc_video_context_Hold(pool->vctx);
>>> + vlc_atomic_rc_init(&pool->rc);
>>> return pool;
>>> free_pool:
>>> @@ -197,20 +222,22 @@ error:
>>> static void nvdec_picture_CtxDestroy(struct picture_context_t
>>> *picctx)
>>> {
>>> - pic_context_nvdec_t *srcpic = NVDEC_PICCTX_FROM_PICCTX(picctx);
>>> + pic_pool_context_nvdec_t *srcpic =
>>> NVDEC_PICPOOLCTX_FROM_PICCTX(picctx);
>>> + nvdec_pool_Release(srcpic->pool);
>>> free(srcpic);
>>> }
>>> static struct picture_context_t *nvdec_picture_CtxClone(struct
>>> picture_context_t *srcctx)
>>> {
>>> - pic_context_nvdec_t *clonectx = malloc(sizeof(*clonectx));
>>> + pic_pool_context_nvdec_t *clonectx = malloc(sizeof(*clonectx));
>>> if (unlikely(clonectx == NULL))
>>> return NULL;
>>> - pic_context_nvdec_t *srcpic = NVDEC_PICCTX_FROM_PICCTX(srcctx);
>>> + pic_pool_context_nvdec_t *srcpic =
>>> NVDEC_PICPOOLCTX_FROM_PICCTX(srcctx);
>>> *clonectx = *srcpic;
>>> - vlc_video_context_Hold(clonectx->ctx.vctx);
>>> - return &clonectx->ctx;
>>> + vlc_video_context_Hold(clonectx->ctx.ctx.vctx);
>>> + nvdec_pool_AddRef(clonectx->pool);
>>> + return &clonectx->ctx.ctx;
>>> }
>>> static picture_t* nvdec_pool_Wait(nvdec_pool_t *pool)
>>> @@ -219,18 +246,21 @@ static picture_t* nvdec_pool_Wait(nvdec_pool_t
>>> *pool)
>>> if (!pic)
>>> return NULL;
>>> - pic_context_nvdec_t *picctx = malloc(sizeof(*picctx));
>>> + pic_pool_context_nvdec_t *picctx = malloc(sizeof(*picctx));
>>> if (!picctx)
>>> goto error;
>>> - picctx->ctx = (picture_context_t) {
>>> + picctx->ctx.ctx = (picture_context_t) {
>>> nvdec_picture_CtxDestroy,
>>> nvdec_picture_CtxClone,
>>> pool->vctx,
>>> };
>>> - vlc_video_context_Hold(picctx->ctx.vctx);
>>> + vlc_video_context_Hold(picctx->ctx.ctx.vctx);
>>> +
>>> + picctx->pool = pool;
>>> + nvdec_pool_AddRef(picctx->pool);
>>> - pic->context = &picctx->ctx;
>>> + pic->context = &picctx->ctx.ctx;
>>> return pic;
>>> error:
>>> @@ -313,7 +343,7 @@ static int CUDAAPI HandleVideoSequence(void
>>> *p_opaque, CUVIDEOFORMAT *p_format)
>>> {
>>> if (p_sys->out_pool)
>>> {
>>> - nvdec_pool_Destroy(p_sys->out_pool);
>>> + nvdec_pool_Release(p_sys->out_pool);
>>> p_sys->out_pool = NULL;
>>> }
>>> }
>>> @@ -1059,7 +1089,7 @@ static void CloseDecoder(vlc_object_t *p_this)
>>> CALL_CUDA_DEC(cuCtxPopCurrent, NULL);
>>> if (p_sys->out_pool)
>>> - nvdec_pool_Destroy(p_sys->out_pool);
>>> + nvdec_pool_Release(p_sys->out_pool);
>>> if (p_sys->cudecoder)
>>> CALL_CUVID(cuvidDestroyDecoder, p_sys->cudecoder);
>>> if (p_sys->cuparser)
>>> --
>>> 2.17.1
>>>
>>> _______________________________________________
>>> 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