diff mbox series

[FFmpeg-devel] hwcontext: add av_hwdevice_ctx_create_derived2

Message ID M72G5av--3-2@lynne.ee
State New
Headers show
Series [FFmpeg-devel] hwcontext: add av_hwdevice_ctx_create_derived2
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lynne May 11, 2020, 10:25 a.m. UTC
This allows for users who derive devices to set options for the 
new device context they derive.
The main use case of this is to allow users to enable extensions
(such as surface drawing extensions) in Vulkan while deriving from
the device their frames are on. That way, users don't need to write
any initialization code themselves, since currently Vulkan prevents
mixing instances and devices.
Also, with this, users can also set custom OpenCL extensions such
as cl_khr_gl_sharing and cl_khr_gl_depth_images.
Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
argument since they don't support options at all (or in VAAPI's case,
options are only used for device selection, which device_derive overrides).

Patch attached.
Subject: [PATCH] hwcontext: add av_hwdevice_ctx_create_derived2

This allows for users who derive devices to set options for the
new device context they derive.
The main use case of this is to allow users to enable extensions
(such as surface drawing extensions) in Vulkan while deriving from
the device their frames are on. That way, users don't need to write
any initialization code themselves, since currently Vulkan prevents
mixing instances and devices.
Also, with this, users can also set custom OpenCL extensions such
as cl_khr_gl_sharing and cl_khr_gl_depth_images.
Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
argument since they don't support options at all (or in VAAPI's case,
options are only used for device selection, which device_derive overrides).
---
 doc/APIchanges                 |  3 +++
 libavutil/hwcontext.c          | 16 +++++++++++++---
 libavutil/hwcontext.h          | 20 ++++++++++++++++++++
 libavutil/hwcontext_cuda.c     |  1 +
 libavutil/hwcontext_internal.h |  2 +-
 libavutil/hwcontext_opencl.c   | 13 +++++++------
 libavutil/hwcontext_qsv.c      |  2 +-
 libavutil/hwcontext_vaapi.c    |  2 +-
 libavutil/hwcontext_vulkan.c   |  8 ++++----
 libavutil/version.h            |  2 +-
 10 files changed, 52 insertions(+), 17 deletions(-)

Comments

James Almer May 11, 2020, 12:55 p.m. UTC | #1
On 5/11/2020 7:25 AM, Lynne wrote:
> This allows for users who derive devices to set options for the 
> new device context they derive.
> The main use case of this is to allow users to enable extensions
> (such as surface drawing extensions) in Vulkan while deriving from
> the device their frames are on. That way, users don't need to write
> any initialization code themselves, since currently Vulkan prevents
> mixing instances and devices.
> Also, with this, users can also set custom OpenCL extensions such
> as cl_khr_gl_sharing and cl_khr_gl_depth_images.
> Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
> argument since they don't support options at all (or in VAAPI's case,
> options are only used for device selection, which device_derive overrides).
> 
> Patch attached.

Could this be av_hwdevice_ctx_create_derived_dict() or similar, please?
The 2 suffix is pretty ugly and it would be nice if we can avoid adding
more of them.
Lynne May 11, 2020, 1:09 p.m. UTC | #2
May 11, 2020, 13:55 by jamrial@gmail.com:

> On 5/11/2020 7:25 AM, Lynne wrote:
>
>> This allows for users who derive devices to set options for the 
>> new device context they derive.
>> The main use case of this is to allow users to enable extensions
>> (such as surface drawing extensions) in Vulkan while deriving from
>> the device their frames are on. That way, users don't need to write
>> any initialization code themselves, since currently Vulkan prevents
>> mixing instances and devices.
>> Also, with this, users can also set custom OpenCL extensions such
>> as cl_khr_gl_sharing and cl_khr_gl_depth_images.
>> Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
>> argument since they don't support options at all (or in VAAPI's case,
>> options are only used for device selection, which device_derive overrides).
>>
>> Patch attached.
>>
>
> Could this be av_hwdevice_ctx_create_derived_dict() or similar, please?
> The 2 suffix is pretty ugly and it would be nice if we can avoid adding
> more of them.
>

Sure, changed locally to av_hwdevice_ctx_create_derived_opts.
Lynne May 12, 2020, 7:16 p.m. UTC | #3
May 11, 2020, 14:09 by dev@lynne.ee:

> May 11, 2020, 13:55 by jamrial@gmail.com:
>
>> On 5/11/2020 7:25 AM, Lynne wrote:
>>
>>> This allows for users who derive devices to set options for the 
>>> new device context they derive.
>>> The main use case of this is to allow users to enable extensions
>>> (such as surface drawing extensions) in Vulkan while deriving from
>>> the device their frames are on. That way, users don't need to write
>>> any initialization code themselves, since currently Vulkan prevents
>>> mixing instances and devices.
>>> Also, with this, users can also set custom OpenCL extensions such
>>> as cl_khr_gl_sharing and cl_khr_gl_depth_images.
>>> Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
>>> argument since they don't support options at all (or in VAAPI's case,
>>> options are only used for device selection, which device_derive overrides).
>>>
>>> Patch attached.
>>>
>>
>> Could this be av_hwdevice_ctx_create_derived_dict() or similar, please?
>> The 2 suffix is pretty ugly and it would be nice if we can avoid adding
>> more of them.
>>
>
> Sure, changed locally to av_hwdevice_ctx_create_derived_opts.
>

I've been testing and running this code for the past 2 days, planning to push this tomorrow if no one LGTMs.
The patch with said name change is attached.

Thanks.
Mark Thompson May 12, 2020, 10:16 p.m. UTC | #4
On 12/05/2020 20:16, Lynne wrote:
> From 45ec8f730a183cd98b1d2d705e7a9582ef2f3f28 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Mon, 11 May 2020 11:02:19 +0100
> Subject: [PATCH] hwcontext: add av_hwdevice_ctx_create_derived_opts
> 
> This allows for users who derive devices to set options for the
> new device context they derive.
> The main use case of this is to allow users to enable extensions
> (such as surface drawing extensions) in Vulkan while deriving from
> the device their frames are on. That way, users don't need to write
> any initialization code themselves, since currently Vulkan prevents
> mixing instances and devices.
> Also, with this, users can also set custom OpenCL extensions such
> as cl_khr_gl_sharing and cl_khr_gl_depth_images.
> Apart from OpenCL and Vulkan, other hwcontexts ignore the opts
> argument since they don't support options at all (or in VAAPI's case,
> options are only used for device selection, which device_derive overrides).
> ---
>  doc/APIchanges                 |  3 +++
>  libavutil/hwcontext.c          | 16 +++++++++++++---
>  libavutil/hwcontext.h          | 20 ++++++++++++++++++++
>  libavutil/hwcontext_cuda.c     |  1 +
>  libavutil/hwcontext_internal.h |  2 +-
>  libavutil/hwcontext_opencl.c   | 13 +++++++------
>  libavutil/hwcontext_qsv.c      |  2 +-
>  libavutil/hwcontext_vaapi.c    |  2 +-
>  libavutil/hwcontext_vulkan.c   |  8 ++++----
>  libavutil/version.h            |  2 +-
>  10 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 75cfdb08b0..9504da5fa4 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-05-11 - xxxxxxxxxx - lavu 56.45.100 - hwcontext.h
> +  Add av_hwdevice_ctx_create_derived_opts.
> +
>  2020-05-10 - xxxxxxxxxx - lavu 56.44.100 - hwcontext_vulkan.h
>    Add enabled_inst_extensions, num_enabled_inst_extensions, enabled_dev_extensions
>    and num_enabled_dev_extensions fields to AVVulkanDeviceContext
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index b01612de05..9d7b928a6d 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -643,9 +643,10 @@ fail:
>      return ret;
>  }
>  
> -int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> -                                   enum AVHWDeviceType type,
> -                                   AVBufferRef *src_ref, int flags)
> +int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> +                                        enum AVHWDeviceType type,
> +                                        AVDictionary *options,
> +                                        AVBufferRef *src_ref, int flags)
>  {
>      AVBufferRef *dst_ref = NULL, *tmp_ref;
>      AVHWDeviceContext *dst_ctx, *tmp_ctx;
> @@ -677,6 +678,7 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
>          tmp_ctx = (AVHWDeviceContext*)tmp_ref->data;
>          if (dst_ctx->internal->hw_type->device_derive) {
>              ret = dst_ctx->internal->hw_type->device_derive(dst_ctx,
> +                                                            options,
>                                                              tmp_ctx,
>                                                              flags);
>              if (ret == 0) {
> @@ -709,6 +711,14 @@ fail:
>      return ret;
>  }
>  
> +int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
> +                                   enum AVHWDeviceType type,
> +                                   AVBufferRef *src_ref, int flags)
> +{
> +    return av_hwdevice_ctx_create_derived_opts(dst_ref_ptr, type, NULL,
> +                                               src_ref, flags);
> +}
> +
>  static void ff_hwframe_unmap(void *opaque, uint8_t *data)
>  {
>      HWMapDescriptor *hwmap = (HWMapDescriptor*)data;
> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> index f874af9f8f..23e2135255 100644
> --- a/libavutil/hwcontext.h
> +++ b/libavutil/hwcontext.h
> @@ -328,6 +328,26 @@ int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
>                                     enum AVHWDeviceType type,
>                                     AVBufferRef *src_ctx, int flags);
>  
> +/**
> + * Create a new device of the specified type from an existing device.
> + *
> + * This function performs the same action as av_hwdevice_ctx_create_derived,
> + * however, it is able to set options for the new device to be derived.
> + *
> + * @param dst_ctx On success, a reference to the newly-created
> + *                AVHWDeviceContext.
> + * @param type    The type of the new device to create.
> + * @param options Options for the new device to create, same format as in
> + *                av_hwdevice_ctx_create.
> + * @param src_ctx A reference to an existing AVHWDeviceContext which will be
> + *                used to create the new device.
> + * @param flags   Currently unused; should be set to zero.
> + * @return        Zero on success, a negative AVERROR code on failure.
> + */
> +int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ref_ptr,
> +                                        enum AVHWDeviceType type,
> +                                        AVDictionary *options,
> +                                        AVBufferRef *src_ref, int flags);

Can you make the argument names and order consistent with the other functions here?

int av_hwdevice_ctx_create(AVBufferRef **device_ctx, enum AVHWDeviceType type,
                           const char *device, AVDictionary *opts, int flags);

int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
                                   enum AVHWDeviceType type,
                                   AVBufferRef *src_ctx, int flags);

int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ctx,
                                        enum AVHWDeviceType type,
                                        AVBufferRef *src_ctx,
                                        AVDictionary *options, int flags);

(Also make sure that the names match the documentation, which they do after my suggested change.)

>  
>  /**
>   * Allocate an AVHWFramesContext tied to a given device context.
> ...
> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
> index dba0f39944..9d66245a23 100644
> --- a/libavutil/hwcontext_internal.h
> +++ b/libavutil/hwcontext_internal.h
> @@ -66,7 +66,7 @@ typedef struct HWContextType {
>  
>      int              (*device_create)(AVHWDeviceContext *ctx, const char *device,
>                                        AVDictionary *opts, int flags);
> -    int              (*device_derive)(AVHWDeviceContext *dst_ctx,
> +    int              (*device_derive)(AVHWDeviceContext *dst_ctx, AVDictionary *opts,
>                                        AVHWDeviceContext *src_ctx, int flags);

Arguments in the same order as device_create, please.

>  
>      int              (*device_init)(AVHWDeviceContext *ctx);
> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
> index 41fdfe96f1..b5a282b55b 100644
> --- a/libavutil/hwcontext_opencl.c
> +++ b/libavutil/hwcontext_opencl.c
> @@ -1193,7 +1193,7 @@ static int opencl_filter_drm_arm_device(AVHWDeviceContext *hwdev,
>  }
>  #endif
>  
> -static int opencl_device_derive(AVHWDeviceContext *hwdev,
> +static int opencl_device_derive(AVHWDeviceContext *hwdev, AVDictionary *opts,
>                                  AVHWDeviceContext *src_ctx,
>                                  int flags)
>  {
> @@ -1207,16 +1207,17 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>              // Surface mapping works via DRM PRIME fds with no special
>              // initialisation required in advance.  This just finds the
>              // Beignet ICD by name.
> -            AVDictionary *opts = NULL;
> +            AVDictionary *selector_opts = NULL;
> +            av_dict_copy(&selector_opts, opts, 0);
>  
> -            err = av_dict_set(&opts, "platform_vendor", "Intel", 0);
> +            err = av_dict_set(&selector_opts, "platform_vendor", "Intel", 0);
>              if (err >= 0)
> -                err = av_dict_set(&opts, "platform_version", "beignet", 0);
> +                err = av_dict_set(&selector_opts, "platform_version", "beignet", 0);
>              if (err >= 0) {
>                  OpenCLDeviceSelector selector = {
>                      .platform_index      = -1,
>                      .device_index        = 0,
> -                    .context             = opts,
> +                    .context             = selector_opts,
>                      .enumerate_platforms = &opencl_enumerate_platforms,
>                      .filter_platform     = &opencl_filter_platform,
>                      .enumerate_devices   = &opencl_enumerate_devices,
> @@ -1224,7 +1225,7 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>                  };
>                  err = opencl_device_create_internal(hwdev, &selector, NULL);
>              }
> -            av_dict_free(&opts);
> +            av_dict_free(&selector_opts);
>          }
>          break;
>  #endif

Can you explain what this change to the code is trying to do?  I might be missing something, but the only additional effect that I can see of passing through the outer options is that it might unexpectedly prevent the search from finding the Beignet ICD (e.g. by having some conflicting device option).

> ...

> I've been testing and running this code for the past 2 days, planning to push this tomorrow if no one LGTMs.

For internal things, maybe.  When adding external API which is intended to last forever I don't think it is a good idea to be unilaterally pushing things after three days.

Thanks,

- Mark
Lynne May 12, 2020, 11:17 p.m. UTC | #5
May 12, 2020, 23:16 by sw@jkqxz.net:

> On 12/05/2020 20:16, Lynne wrote:
>
>> From 45ec8f730a183cd98b1d2d705e7a9582ef2f3f28 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Mon, 11 May 2020 11:02:19 +0100
>> Subject: [PATCH] hwcontext: add av_hwdevice_ctx_create_derived_opts
>>
>
> Can you make the argument names and order consistent with the other functions here?
>
> int av_hwdevice_ctx_create(AVBufferRef **device_ctx, enum AVHWDeviceType type,
>  const char *device, AVDictionary *opts, int flags);
>
> int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
>  enum AVHWDeviceType type,
>  AVBufferRef *src_ctx, int flags);
>
> int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ctx,
>  enum AVHWDeviceType type,
>  AVBufferRef *src_ctx,
>  AVDictionary *options, int flags);
>
> (Also make sure that the names match the documentation, which they do after my suggested change.)
>

Changed. I copied the arguments from av_hwdevice_ctx_create_derived in hwcontext.c and didn't
notice they were different to the ones in the header.



>>
>> /**
>>  * Allocate an AVHWFramesContext tied to a given device context.
>> ...
>> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
>> index dba0f39944..9d66245a23 100644
>> --- a/libavutil/hwcontext_internal.h
>> +++ b/libavutil/hwcontext_internal.h
>> @@ -66,7 +66,7 @@ typedef struct HWContextType {
>>  
>>  int              (*device_create)(AVHWDeviceContext *ctx, const char *device,
>>  AVDictionary *opts, int flags);
>> -    int              (*device_derive)(AVHWDeviceContext *dst_ctx,
>> +    int              (*device_derive)(AVHWDeviceContext *dst_ctx, AVDictionary *opts,
>>  AVHWDeviceContext *src_ctx, int flags);
>>
>
> Arguments in the same order as device_create, please.
>

Changed.



>>
>> int              (*device_init)(AVHWDeviceContext *ctx);
>> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
>> index 41fdfe96f1..b5a282b55b 100644
>> --- a/libavutil/hwcontext_opencl.c
>> +++ b/libavutil/hwcontext_opencl.c
>> @@ -1193,7 +1193,7 @@ static int opencl_filter_drm_arm_device(AVHWDeviceContext *hwdev,
>>  }
>>  #endif
>>  
>> -static int opencl_device_derive(AVHWDeviceContext *hwdev,
>> +static int opencl_device_derive(AVHWDeviceContext *hwdev, AVDictionary *opts,
>>  AVHWDeviceContext *src_ctx,
>>  int flags)
>>  {
>> @@ -1207,16 +1207,17 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>>  // Surface mapping works via DRM PRIME fds with no special
>>  // initialisation required in advance.  This just finds the
>>  // Beignet ICD by name.
>> -            AVDictionary *opts = NULL;
>> +            AVDictionary *selector_opts = NULL;
>> +            av_dict_copy(&selector_opts, opts, 0);
>>  
>> -            err = av_dict_set(&opts, "platform_vendor", "Intel", 0);
>> +            err = av_dict_set(&selector_opts, "platform_vendor", "Intel", 0);
>>  if (err >= 0)
>> -                err = av_dict_set(&opts, "platform_version", "beignet", 0);
>> +                err = av_dict_set(&selector_opts, "platform_version", "beignet", 0);
>>  if (err >= 0) {
>>  OpenCLDeviceSelector selector = {
>>  .platform_index      = -1,
>>  .device_index        = 0,
>> -                    .context             = opts,
>> +                    .context             = selector_opts,
>>  .enumerate_platforms = &opencl_enumerate_platforms,
>>  .filter_platform     = &opencl_filter_platform,
>>  .enumerate_devices   = &opencl_enumerate_devices,
>> @@ -1224,7 +1225,7 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>>  };
>>  err = opencl_device_create_internal(hwdev, &selector, NULL);
>>  }
>> -            av_dict_free(&opts);
>> +            av_dict_free(&selector_opts);
>>  }
>>  break;
>>  #endif
>>
>
> Can you explain what this change to the code is trying to do?  I might be missing something, but the only additional effect that I can see of passing through the outer options is that it might unexpectedly prevent the search from finding the Beignet ICD (e.g. by having some conflicting device option).
>

I thought the selector options were passed onto device_init through some mechanism I couldn't see,
where I thought they're set, and I mistakenly thought "device_extensions" in the opencl_device_params
was a list of all accepted keys for options, but I was wrong - they're only used as filters for devices.
Still, the selector code is... somewhat sphagetti, so its not that easy to figure out how it works.
I've removed this change, only kept the change of name from opts to selector_opts so it won't
conflict with the new argument.



>> ...
>>
>> I've been testing and running this code for the past 2 days, planning to push this tomorrow if no one LGTMs.
>>
>
> For internal things, maybe.  When adding external API which is intended to last forever I don't think it is a good idea to be unilaterally pushing things after three days.
>

Its not a large patch, and its been quiet for an API change.

I've attached a new version of the patch, rebased against current git master.
Lynne May 23, 2020, 6:09 p.m. UTC | #6
May 13, 2020, 00:17 by dev@lynne.ee:

> May 12, 2020, 23:16 by sw@jkqxz.net:
>
>> On 12/05/2020 20:16, Lynne wrote:
>>
>>> From 45ec8f730a183cd98b1d2d705e7a9582ef2f3f28 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev@lynne.ee>
>>> Date: Mon, 11 May 2020 11:02:19 +0100
>>> Subject: [PATCH] hwcontext: add av_hwdevice_ctx_create_derived_opts
>>>
>>
>> Can you make the argument names and order consistent with the other functions here?
>>
>> int av_hwdevice_ctx_create(AVBufferRef **device_ctx, enum AVHWDeviceType type,
>>  const char *device, AVDictionary *opts, int flags);
>>
>> int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
>>  enum AVHWDeviceType type,
>>  AVBufferRef *src_ctx, int flags);
>>
>> int av_hwdevice_ctx_create_derived_opts(AVBufferRef **dst_ctx,
>>  enum AVHWDeviceType type,
>>  AVBufferRef *src_ctx,
>>  AVDictionary *options, int flags);
>>
>> (Also make sure that the names match the documentation, which they do after my suggested change.)
>>
>
> Changed. I copied the arguments from av_hwdevice_ctx_create_derived in hwcontext.c and didn't
> notice they were different to the ones in the header.
>
>
>
>>>
>>> /**
>>>  * Allocate an AVHWFramesContext tied to a given device context.
>>> ...
>>> diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
>>> index dba0f39944..9d66245a23 100644
>>> --- a/libavutil/hwcontext_internal.h
>>> +++ b/libavutil/hwcontext_internal.h
>>> @@ -66,7 +66,7 @@ typedef struct HWContextType {
>>>  
>>>  int              (*device_create)(AVHWDeviceContext *ctx, const char *device,
>>>  AVDictionary *opts, int flags);
>>> -    int              (*device_derive)(AVHWDeviceContext *dst_ctx,
>>> +    int              (*device_derive)(AVHWDeviceContext *dst_ctx, AVDictionary *opts,
>>>  AVHWDeviceContext *src_ctx, int flags);
>>>
>>
>> Arguments in the same order as device_create, please.
>>
>
> Changed.
>
>
>
>>>
>>> int              (*device_init)(AVHWDeviceContext *ctx);
>>> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
>>> index 41fdfe96f1..b5a282b55b 100644
>>> --- a/libavutil/hwcontext_opencl.c
>>> +++ b/libavutil/hwcontext_opencl.c
>>> @@ -1193,7 +1193,7 @@ static int opencl_filter_drm_arm_device(AVHWDeviceContext *hwdev,
>>>  }
>>>  #endif
>>>  
>>> -static int opencl_device_derive(AVHWDeviceContext *hwdev,
>>> +static int opencl_device_derive(AVHWDeviceContext *hwdev, AVDictionary *opts,
>>>  AVHWDeviceContext *src_ctx,
>>>  int flags)
>>>  {
>>> @@ -1207,16 +1207,17 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>>>  // Surface mapping works via DRM PRIME fds with no special
>>>  // initialisation required in advance.  This just finds the
>>>  // Beignet ICD by name.
>>> -            AVDictionary *opts = NULL;
>>> +            AVDictionary *selector_opts = NULL;
>>> +            av_dict_copy(&selector_opts, opts, 0);
>>>  
>>> -            err = av_dict_set(&opts, "platform_vendor", "Intel", 0);
>>> +            err = av_dict_set(&selector_opts, "platform_vendor", "Intel", 0);
>>>  if (err >= 0)
>>> -                err = av_dict_set(&opts, "platform_version", "beignet", 0);
>>> +                err = av_dict_set(&selector_opts, "platform_version", "beignet", 0);
>>>  if (err >= 0) {
>>>  OpenCLDeviceSelector selector = {
>>>  .platform_index      = -1,
>>>  .device_index        = 0,
>>> -                    .context             = opts,
>>> +                    .context             = selector_opts,
>>>  .enumerate_platforms = &opencl_enumerate_platforms,
>>>  .filter_platform     = &opencl_filter_platform,
>>>  .enumerate_devices   = &opencl_enumerate_devices,
>>> @@ -1224,7 +1225,7 @@ static int opencl_device_derive(AVHWDeviceContext *hwdev,
>>>  };
>>>  err = opencl_device_create_internal(hwdev, &selector, NULL);
>>>  }
>>> -            av_dict_free(&opts);
>>> +            av_dict_free(&selector_opts);
>>>  }
>>>  break;
>>>  #endif
>>>
>>
>> Can you explain what this change to the code is trying to do?  I might be missing something, but the only additional effect that I can see of passing through the outer options is that it might unexpectedly prevent the search from finding the Beignet ICD (e.g. by having some conflicting device option).
>>
>
> I thought the selector options were passed onto device_init through some mechanism I couldn't see,
> where I thought they're set, and I mistakenly thought "device_extensions" in the opencl_device_params
> was a list of all accepted keys for options, but I was wrong - they're only used as filters for devices.
> Still, the selector code is... somewhat sphagetti, so its not that easy to figure out how it works.
> I've removed this change, only kept the change of name from opts to selector_opts so it won't
> conflict with the new argument.
>
>
>
>>> ...
>>>
>>> I've been testing and running this code for the past 2 days, planning to push this tomorrow if no one LGTMs.
>>>
>>
>> For internal things, maybe.  When adding external API which is intended to last forever I don't think it is a good idea to be unilaterally pushing things after three days.
>>
>
> Its not a large patch, and its been quiet for an API change.
>
> I've attached a new version of the patch, rebased against current git master.
>

Pushed, thanks for the review.
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 75cfdb08b0..1bb0677142 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2020-05-11 - xxxxxxxxxx - lavu 56.45.100 - hwcontext.h
+  Add av_hwdevice_ctx_create_derived2.
+
 2020-05-10 - xxxxxxxxxx - lavu 56.44.100 - hwcontext_vulkan.h
   Add enabled_inst_extensions, num_enabled_inst_extensions, enabled_dev_extensions
   and num_enabled_dev_extensions fields to AVVulkanDeviceContext
diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index b01612de05..77b7014c39 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -643,9 +643,10 @@  fail:
     return ret;
 }
 
-int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
-                                   enum AVHWDeviceType type,
-                                   AVBufferRef *src_ref, int flags)
+int av_hwdevice_ctx_create_derived2(AVBufferRef **dst_ref_ptr,
+                                    enum AVHWDeviceType type,
+                                    AVDictionary *options,
+                                    AVBufferRef *src_ref, int flags)
 {
     AVBufferRef *dst_ref = NULL, *tmp_ref;
     AVHWDeviceContext *dst_ctx, *tmp_ctx;
@@ -677,6 +678,7 @@  int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
         tmp_ctx = (AVHWDeviceContext*)tmp_ref->data;
         if (dst_ctx->internal->hw_type->device_derive) {
             ret = dst_ctx->internal->hw_type->device_derive(dst_ctx,
+                                                            options,
                                                             tmp_ctx,
                                                             flags);
             if (ret == 0) {
@@ -709,6 +711,14 @@  fail:
     return ret;
 }
 
+int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ref_ptr,
+                                   enum AVHWDeviceType type,
+                                   AVBufferRef *src_ref, int flags)
+{
+    return av_hwdevice_ctx_create_derived2(dst_ref_ptr, type, NULL,
+                                           src_ref, flags);
+}
+
 static void ff_hwframe_unmap(void *opaque, uint8_t *data)
 {
     HWMapDescriptor *hwmap = (HWMapDescriptor*)data;
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index f874af9f8f..4a94a2a8e4 100644
--- a/libavutil/hwcontext.h
+++ b/libavutil/hwcontext.h
@@ -328,6 +328,26 @@  int av_hwdevice_ctx_create_derived(AVBufferRef **dst_ctx,
                                    enum AVHWDeviceType type,
                                    AVBufferRef *src_ctx, int flags);
 
+/**
+ * Create a new device of the specified type from an existing device.
+ *
+ * This function performs the same action as av_hwdevice_ctx_create_derived,
+ * however, it is able to set options for the new device to be derived.
+ *
+ * @param dst_ctx On success, a reference to the newly-created
+ *                AVHWDeviceContext.
+ * @param type    The type of the new device to create.
+ * @param options Options for the new device to create, same format as in
+ *                av_hwdevice_ctx_create.
+ * @param src_ctx A reference to an existing AVHWDeviceContext which will be
+ *                used to create the new device.
+ * @param flags   Currently unused; should be set to zero.
+ * @return        Zero on success, a negative AVERROR code on failure.
+ */
+int av_hwdevice_ctx_create_derived2(AVBufferRef **dst_ref_ptr,
+                                    enum AVHWDeviceType type,
+                                    AVDictionary *options,
+                                    AVBufferRef *src_ref, int flags);
 
 /**
  * Allocate an AVHWFramesContext tied to a given device context.
diff --git a/libavutil/hwcontext_cuda.c b/libavutil/hwcontext_cuda.c
index 58d128a280..a4fbc3675a 100644
--- a/libavutil/hwcontext_cuda.c
+++ b/libavutil/hwcontext_cuda.c
@@ -395,6 +395,7 @@  error:
 }
 
 static int cuda_device_derive(AVHWDeviceContext *device_ctx,
+                              AVDictionart *options,
                               AVHWDeviceContext *src_ctx,
                               int flags) {
     AVCUDADeviceContext *hwctx = device_ctx->hwctx;
diff --git a/libavutil/hwcontext_internal.h b/libavutil/hwcontext_internal.h
index dba0f39944..9d66245a23 100644
--- a/libavutil/hwcontext_internal.h
+++ b/libavutil/hwcontext_internal.h
@@ -66,7 +66,7 @@  typedef struct HWContextType {
 
     int              (*device_create)(AVHWDeviceContext *ctx, const char *device,
                                       AVDictionary *opts, int flags);
-    int              (*device_derive)(AVHWDeviceContext *dst_ctx,
+    int              (*device_derive)(AVHWDeviceContext *dst_ctx, AVDictionary *opts,
                                       AVHWDeviceContext *src_ctx, int flags);
 
     int              (*device_init)(AVHWDeviceContext *ctx);
diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
index 41fdfe96f1..b5a282b55b 100644
--- a/libavutil/hwcontext_opencl.c
+++ b/libavutil/hwcontext_opencl.c
@@ -1193,7 +1193,7 @@  static int opencl_filter_drm_arm_device(AVHWDeviceContext *hwdev,
 }
 #endif
 
-static int opencl_device_derive(AVHWDeviceContext *hwdev,
+static int opencl_device_derive(AVHWDeviceContext *hwdev, AVDictionary *opts,
                                 AVHWDeviceContext *src_ctx,
                                 int flags)
 {
@@ -1207,16 +1207,17 @@  static int opencl_device_derive(AVHWDeviceContext *hwdev,
             // Surface mapping works via DRM PRIME fds with no special
             // initialisation required in advance.  This just finds the
             // Beignet ICD by name.
-            AVDictionary *opts = NULL;
+            AVDictionary *selector_opts = NULL;
+            av_dict_copy(&selector_opts, opts, 0);
 
-            err = av_dict_set(&opts, "platform_vendor", "Intel", 0);
+            err = av_dict_set(&selector_opts, "platform_vendor", "Intel", 0);
             if (err >= 0)
-                err = av_dict_set(&opts, "platform_version", "beignet", 0);
+                err = av_dict_set(&selector_opts, "platform_version", "beignet", 0);
             if (err >= 0) {
                 OpenCLDeviceSelector selector = {
                     .platform_index      = -1,
                     .device_index        = 0,
-                    .context             = opts,
+                    .context             = selector_opts,
                     .enumerate_platforms = &opencl_enumerate_platforms,
                     .filter_platform     = &opencl_filter_platform,
                     .enumerate_devices   = &opencl_enumerate_devices,
@@ -1224,7 +1225,7 @@  static int opencl_device_derive(AVHWDeviceContext *hwdev,
                 };
                 err = opencl_device_create_internal(hwdev, &selector, NULL);
             }
-            av_dict_free(&opts);
+            av_dict_free(&selector_opts);
         }
         break;
 #endif
diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index b1b67400de..e3b969c20f 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -1188,7 +1188,7 @@  fail:
     return ret;
 }
 
-static int qsv_device_derive(AVHWDeviceContext *ctx,
+static int qsv_device_derive(AVHWDeviceContext *ctx, AVDictionary *opts,
                              AVHWDeviceContext *child_device_ctx, int flags)
 {
     return qsv_device_derive_from_child(ctx, MFX_IMPL_HARDWARE_ANY,
diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
index b306965b4a..e2ab149622 100644
--- a/libavutil/hwcontext_vaapi.c
+++ b/libavutil/hwcontext_vaapi.c
@@ -1623,7 +1623,7 @@  static int vaapi_device_create(AVHWDeviceContext *ctx, const char *device,
     return vaapi_device_connect(ctx, display);
 }
 
-static int vaapi_device_derive(AVHWDeviceContext *ctx,
+static int vaapi_device_derive(AVHWDeviceContext *ctx, AVDictionary *opts,
                                AVHWDeviceContext *src_ctx, int flags)
 {
 #if HAVE_VAAPI_DRM
diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index a5983f2735..cb581c1468 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -936,7 +936,7 @@  static int vulkan_device_create(AVHWDeviceContext *ctx, const char *device,
     return vulkan_device_create_internal(ctx, &dev_select, opts, flags);
 }
 
-static int vulkan_device_derive(AVHWDeviceContext *ctx,
+static int vulkan_device_derive(AVHWDeviceContext *ctx, AVDictionary *opts,
                                 AVHWDeviceContext *src_ctx, int flags)
 {
     av_unused VulkanDeviceSelection dev_select = { 0 };
@@ -961,7 +961,7 @@  static int vulkan_device_derive(AVHWDeviceContext *ctx,
         if (strstr(vendor, "AMD"))
             dev_select.vendor_id = 0x1002;
 
-        return vulkan_device_create_internal(ctx, &dev_select, NULL, flags);
+        return vulkan_device_create_internal(ctx, &dev_select, opts, flags);
     }
 #endif
     case AV_HWDEVICE_TYPE_DRM: {
@@ -979,7 +979,7 @@  static int vulkan_device_derive(AVHWDeviceContext *ctx,
 
         drmFreeDevice(&drm_dev_info);
 
-        return vulkan_device_create_internal(ctx, &dev_select, NULL, flags);
+        return vulkan_device_create_internal(ctx, &dev_select, opts, flags);
     }
 #endif
 #if CONFIG_CUDA
@@ -998,7 +998,7 @@  static int vulkan_device_derive(AVHWDeviceContext *ctx,
 
         dev_select.has_uuid = 1;
 
-        return vulkan_device_create_internal(ctx, &dev_select, NULL, flags);
+        return vulkan_device_create_internal(ctx, &dev_select, opts, flags);
     }
 #endif
     default:
diff --git a/libavutil/version.h b/libavutil/version.h
index 48d8a38c42..77592fcd8e 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  56
-#define LIBAVUTIL_VERSION_MINOR  44
+#define LIBAVUTIL_VERSION_MINOR  45
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \