May 13, 2020, 00:17 by d...@lynne.ee:
> May 12, 2020, 23:16 by s...@jkqxz.net:
>
>> On 12/05/2020 20:16, Lynne wrote:
>>
>>> From 45ec8f730a183cd98b1d2d705e7a9582ef2f3f28 Mon Sep 17 00:00:00 2001
>>> From: Lynne <d...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
