[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