[vlc-devel] [PATCH 6/6] nvdec: add reference-count to the picture pool
Quentin Chateau
quentin.chateau at deepskycorp.com
Thu Mar 26 14:31:38 CET 2020
Oh right I think I understand. It seems things changed quite a lot since
nvdec was written.
To be fair I would really like to see this patchset in master: currently
there is a use-after free issue because pictures outlive the CUDA memory
pool. In most cases CUDA detects it, but I have seen VLC freeze because
of this. I understand using a picture_pool_t is not the recommended way
of doing this now that you changed the model, but I hope to make nvdec
strictly better than it was before with this patchset.
Removing the picture_pool_t can always be done in a later patchset.
Please let me know if you agree and if you have any more comments. I
already have an updated patchset addressing all the comments you made
ready to be sent for a second review.
On 26/03/2020 13:44, Steve Lhomme wrote:
> 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
> _______________________________________________
> 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