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

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(-)

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.

> +
>      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.)

> +
>      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.

> +    int flags;
> +} AVCUDAFramesContext;
> +
> +/**
> + * No actual allocation will happen, but otherwise behaves like normal.
> + *
> + * This is to be used if a AVHWFramesContext is required, but the actual
> + * allocation has to happen outside of it.
> + */
> +#define AV_CUDA_HWFRAMES_DUMMY_MODE (1 << 0)
>  
>  #endif /* AVUTIL_HWCONTEXT_CUDA_H */
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 5185454d9b..84409b1d69 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  18
> +#define LIBAVUTIL_VERSION_MINOR  19
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> 

This approach seems ok to me.

Thanks,

- Mark

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.

>> +
>>       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.

>> +
>>       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"?

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

2018-05-08 5:10 GMT+08:00 Timo Rothenpieler <timo@rothenpieler.org>:

> 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;
> +
>      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;
> +
>      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().
> +     */
> +    int flags;
> +} AVCUDAFramesContext;
> +
> +/**
> + * No actual allocation will happen, but otherwise behaves like normal.
> + *
> + * This is to be used if a AVHWFramesContext is required, but the actual
> + * allocation has to happen outside of it.
> + */
> +#define AV_CUDA_HWFRAMES_DUMMY_MODE (1 << 0)
>
>  #endif /* AVUTIL_HWCONTEXT_CUDA_H */
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 5185454d9b..84409b1d69 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  18
> +#define LIBAVUTIL_VERSION_MINOR  19
>  #define LIBAVUTIL_VERSION_MICRO 100
>
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> --
>

I think the flag should be added in avcodec, maybe AVCodecContext.flags2,
becuase
1. This flag is used by cuda decoder only, but not works avfilter and other
cuda apis.
2. Other hw codecs may support referencing the decoded memory instead of
copying it, for example, mmal decoder. see
https://github.com/wang-bin/FFmpeg/commit/74390ec836743dd337c92dd5da5f4f9ff638316b#diff-26a23a40f16babe0068c7340c6160472
3. the flag can be turned on/off at any time

> 
>> 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.

Can someone explain the actual use-case? We do direct mapping of cuvid/nvdec frames to transfer them to/from gl, but i have no idea how this is related to that...