[FFmpeg-devel] [PATCH] hwcontext: add av_hwdevice_ctx_create_derived2

[Lynne]

May 12, 2020, 23:16 by sw at jkqxz.net:

> On 12/05/2020 20:16, Lynne wrote:
>
>> From 45ec8f730a183cd98b1d2d705e7a9582ef2f3f28 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev at 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-hwcontext-add-av_hwdevice_ctx_create_derived_opts.patch
Type: text/x-patch
Size: 11641 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200513/438029d4/attachment.bin>