[FFmpeg-devel] [PATCH 2/2] hwcontext_vulkan: expose enabled device and instance extensions

[Mark Thompson]

On 10/05/2020 11:54, Lynne wrote:
> This solves a huge oversight - it lets users reliably use their own 
> AVVulkanDeviceContext. Otherwise, the extensions supplied and enabled
> are not discoverable by anything outside of hwcontext_vulkan.
> 
> Patch attached.
> 
> This, and the previous patch to enable extensions through the options are really needed
> to make the hwcontext useful and interoperable with other Vulkan API users, so I'm planning
> to push them later tonight.
> 
> 
> From 9a3169afafd1cc668f8f9f78fceef46e322963d6 Mon Sep 17 00:00:00 2001
> From: Lynne <dev at lynne.ee>
> Date: Sun, 10 May 2020 11:47:50 +0100
> Subject: [PATCH 2/2] hwcontext_vulkan: expose enabled device and instance
>  extensions
> 
> This solves a huge oversight - it lets users reliably use their own
> AVVulkanDeviceContext. Otherwise, the extensions supplied and enabled
> are not discoverable by anything outside of hwcontext_vulkan.
> ---
>  libavutil/hwcontext_vulkan.c | 27 ++++++++++++++++++++++-----
>  libavutil/hwcontext_vulkan.h | 12 ++++++++++++
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index a35c1d3a4f..4135cc5209 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -415,13 +415,11 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts)
>      /* Try to create the instance */
>      ret = vkCreateInstance(&inst_props, hwctx->alloc, &hwctx->inst);
>  
> -    /* Free used memory */
> -    av_free((void *)inst_props.ppEnabledExtensionNames);
> -
>      /* Check for errors */
>      if (ret != VK_SUCCESS) {
>          av_log(ctx, AV_LOG_ERROR, "Instance creation failure: %s\n",
>                 vk_ret2str(ret));
> +        av_free((void *)inst_props.ppEnabledExtensionNames);
>          return AVERROR_EXTERNAL;
>      }
>  
> @@ -444,6 +442,9 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts)
>                                             hwctx->alloc, &p->debug_ctx);
>      }
>  
> +    hwctx->enabled_inst_extensions = inst_props.ppEnabledExtensionNames;
> +    hwctx->num_enabled_inst_extensions = inst_props.enabledExtensionCount;
> +
>      return 0;
>  }
>  
> @@ -749,6 +750,9 @@ static void vulkan_device_free(AVHWDeviceContext *ctx)
>      }
>  
>      vkDestroyInstance(hwctx->inst, hwctx->alloc);
> +
> +    av_free((void *)hwctx->enabled_inst_extensions);
> +    av_free((void *)hwctx->enabled_dev_extensions);
>  }
>  
>  static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
> @@ -809,11 +813,10 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>      ret = vkCreateDevice(hwctx->phys_dev, &dev_info, hwctx->alloc,
>                           &hwctx->act_dev);
>  
> -    av_free((void *)dev_info.ppEnabledExtensionNames);
> -
>      if (ret != VK_SUCCESS) {
>          av_log(ctx, AV_LOG_ERROR, "Device creation failure: %s\n",
>                 vk_ret2str(ret));
> +        av_free((void *)dev_info.ppEnabledExtensionNames);
>          err = AVERROR_EXTERNAL;
>          goto end;
>      }
> @@ -823,6 +826,9 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>      if (opt_d)
>          p->use_linear_images = strtol(opt_d->value, NULL, 10);
>  
> +    hwctx->enabled_dev_extensions = dev_info.ppEnabledExtensionNames;
> +    hwctx->num_enabled_dev_extensions = dev_info.enabledExtensionCount;
> +
>  end:
>      return err;
>  }
> @@ -834,6 +840,17 @@ static int vulkan_device_init(AVHWDeviceContext *ctx)
>      AVVulkanDeviceContext *hwctx = ctx->hwctx;
>      VulkanDevicePriv *p = ctx->internal->priv;
>  
> +    /* Set device extension flags */
> +    for (int i = 0; i < hwctx->num_enabled_dev_extensions; i++) {
> +        for (int j = 0; j < FF_ARRAY_ELEMS(optional_device_exts); j++) {
> +            if (!strcmp(hwctx->enabled_dev_extensions[i],
> +                        optional_device_exts[j].name)) {
> +                p->extensions |= optional_device_exts[j].flag;
> +                break;
> +            }
> +        }
> +    }
> +
>      vkGetPhysicalDeviceQueueFamilyProperties(hwctx->phys_dev, &queue_num, NULL);
>      if (!queue_num) {
>          av_log(ctx, AV_LOG_ERROR, "Failed to get queues!\n");
> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
> index ebc28916f3..2f9f92a3a2 100644
> --- a/libavutil/hwcontext_vulkan.h
> +++ b/libavutil/hwcontext_vulkan.h
> @@ -42,6 +42,11 @@ typedef struct AVVulkanDeviceContext {
>       * Instance
>       */
>      VkInstance inst;
> +    /**
> +     * Enabled instance extensions. By default, VK_KHR_surface is enabled if found.
> +     */

Clarify how this should be set by the user if they are supplying the instance?  (From code, looks like it's not used.)

> +    const char * const *enabled_inst_extensions;
> +    int num_enabled_inst_extensions;
>      /**
>       * Physical device
>       */
> @@ -50,6 +55,13 @@ typedef struct AVVulkanDeviceContext {
>       * Active device
>       */
>      VkDevice act_dev;
> +    /**
> +     * Enabled device extensions. By default, VK_KHR_external_memory_fd,
> +     * VK_EXT_external_memory_dma_buf, VK_EXT_image_drm_format_modifier and
> +     * VK_KHR_external_semaphore_fd are enabled if found.
> +     */

Clarify how this should be set by the user if they are supplying the device?  (From code, looks like it's required.)

> +    const char * const *enabled_dev_extensions;
> +    int num_enabled_dev_extensions;
>      /**
>       * Queue family index for graphics
>       * @note av_hwdevice_create() will set all 3 queue indices if unset
> -- 
> 2.26.2

I think you need to put the new fields at the end to avoid the worst ABI break (upgrade libavutil and crash trying to use the device fields because they've moved).

The API change with the new field being required is ugly given that it's months after the usual cutoff, but the effect hasn't changed (they wouldn't have had any extensions) so it's probably ok.  Needs a notice in APIchanges, though.

Thanks,

- Mark