[vlc-devel] [PATCH 6/6] nvdec: add reference-count to the picture pool

[Quentin Chateau]

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