[FFmpeg-devel] [PATCH] avutil/hwcontext_cuda: add AVCUDAFramesContext and AVCUDAFramesContext.flags

Mark Thompson sw at jkqxz.net
Tue May 8 02:20:04 EEST 2018


On 07/05/18 22:28, Timo Rothenpieler wrote:
> Am 07.05.2018 um 23:22 schrieb Mark Thompson:
>> On 07/05/18 22:10, Timo Rothenpieler wrote:
>>> Frames can be mapped from nvdec/cuvid, not needing any actual memory
>>> allocation, but all other features of the hw_frames_ctx.
>>> Hence the dummy-mode, which does not allocate any (notable amounts of)
>>> memory but otherwise behaves the exact same.
>>> ---
>>>   doc/APIchanges             |  3 +++
>>>   libavutil/hwcontext_cuda.c | 10 ++++++++++
>>>   libavutil/hwcontext_cuda.h | 18 +++++++++++++++++-
>>>   libavutil/version.h        |  2 +-
>>>   4 files changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index ede5b186ae..82ec888fd8 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>     API changes, most recent first:
>>>   +2018-05-xx - xxxxxxxxxx - lavu 56.19.100 - hwcontext.h
>>> +  Add AVCUDAFramesContext and AVCUDAFramesContext.flags.
>>> +
>>>   2018-04-xx - xxxxxxxxxx - lavu 56.18.100 - pixdesc.h
>>>     Add AV_PIX_FMT_FLAG_ALPHA to AV_PIX_FMT_PAL8.
>>>   diff --git a/libavutil/hwcontext_cuda.c b/libavutil/hwcontext_cuda.c
>>> index 37827a770c..0d867ef0f5 100644
>>> --- a/libavutil/hwcontext_cuda.c
>>> +++ b/libavutil/hwcontext_cuda.c
>>> @@ -83,6 +83,7 @@ static void cuda_buffer_free(void *opaque, uint8_t *data)
>>>   static AVBufferRef *cuda_pool_alloc(void *opaque, int size)
>>>   {
>>>       AVHWFramesContext     *ctx = opaque;
>>> +    AVCUDAFramesContext *frctx = ctx->hwctx;
>>>       AVCUDADeviceContext *hwctx = ctx->device_ctx->hwctx;
>>>       CudaFunctions          *cu = hwctx->internal->cuda_dl;
>>>   @@ -97,6 +98,10 @@ static AVBufferRef *cuda_pool_alloc(void *opaque, int size)
>>>           return NULL;
>>>       }
>>>   +    // A lot of places expect the pointer to be !=NULL, so make minimum allocation instead.
>>> +    if (frctx->flags & AV_CUDA_HWFRAMES_DUMMY_MODE)
>>> +        size = 1;
>>
>> What are those places - can we get rid of them instead?  Allocating a single byte here is rather yucky.
> 
> That'd be AVBuffer/Ref itself interpreting its alloc function returning NULL as ENOMEM. Doubt changing it is a good idea. And as any other return value than NULL indicates a valid pointer, this approach is what I picked.

You could make a new function called, say, "cuda_pool_dummy_alloc" which always returns a non-null pointer (to some random static variable).  Then set that as the allocation function for the buffer pool in dummy mode and drop this changing of size.  (That seems cleaner?  Or maybe I'm overthinking this...)

>>> +
>>>       err = cu->cuMemAlloc(&data, size);
>>>       if (err != CUDA_SUCCESS)
>>>           goto fail;
>>> @@ -161,6 +166,7 @@ static int cuda_frames_init(AVHWFramesContext *ctx)
>>>     static int cuda_get_buffer(AVHWFramesContext *ctx, AVFrame *frame)
>>>   {
>>> +    AVCUDAFramesContext *frctx = ctx->hwctx;
>>>       int aligned_width;
>>>       int width_in_bytes = ctx->width;
>>>   @@ -210,6 +216,9 @@ static int cuda_get_buffer(AVHWFramesContext *ctx, AVFrame *frame)
>>>       frame->width  = ctx->width;
>>>       frame->height = ctx->height;
>>>   +    if (frctx->flags & AV_CUDA_HWFRAMES_DUMMY_MODE)
>>> +        frame->data[0] = frame->data[1] = frame->data[2] = NULL;
>>
>> Are you intending linesize and buf[0] to have been filled with specific values here?
>>
>> A comment saying exactly what is set and what isn't might help.  (Maybe the comment goes with the flag itself.)
> 
> Yes, that's intended. I'm only explicitly cleaning out those as they're dangerous to have pointing to buffers that are smaller than the rest of the frame advertises.
> 
> Describing this fact on the flag is probably a good idea.

Ok, sounds good.

>>> +
>>>       return 0;
>>>   }
>>>   @@ -402,6 +411,7 @@ const HWContextType ff_hwcontext_type_cuda = {
>>>       .name                 = "CUDA",
>>>         .device_hwctx_size    = sizeof(AVCUDADeviceContext),
>>> +    .frames_hwctx_size    = sizeof(AVCUDAFramesContext),
>>>       .frames_priv_size     = sizeof(CUDAFramesContext),
>>>         .device_create        = cuda_device_create,
>>> diff --git a/libavutil/hwcontext_cuda.h b/libavutil/hwcontext_cuda.h
>>> index 12dae8449e..388d6f8f1c 100644
>>> --- a/libavutil/hwcontext_cuda.h
>>> +++ b/libavutil/hwcontext_cuda.h
>>> @@ -45,7 +45,23 @@ typedef struct AVCUDADeviceContext {
>>>   } AVCUDADeviceContext;
>>>     /**
>>> - * AVHWFramesContext.hwctx is currently not used
>>> + * This struct is allocated as AVHWFramesContext.hwctx
>>>    */
>>> +typedef struct AVCUDAFramesContext {
>>> +    /**
>>> +     * Special implementation-specific flags.
>>> +     *
>>> +     * Must be set by the user before calling av_hwframe_ctx_init().
>>> +     */
>>
>> This makes it sound like you /must/ write to the field.  Leaving it as zero is also fine.
> 
> Simply replacing "Must" with "May"?

Sure.  Looking at the other hwcontext implementations they generally don't comment on this unless things really must be set, but whatever you think is clearest.

- Mark


More information about the ffmpeg-devel mailing list