[FFmpeg-devel] [PATCH] hwcontext: add av_hwdevice_ctx_create_derived2
Mark Thompson
sw at jkqxz.net
Wed May 13 01:16:08 EEST 2020
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
>
> 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
More information about the ffmpeg-devel
mailing list