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

[Lynne]

May 13, 2020, 00:17 by dev at lynne.ee:

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

Pushed, thanks for the review.