[FFmpeg-devel] [PATCH 1/2] hwcontext_vulkan: let users enable device and instance extensions using options

[Mark Thompson]

On 10/05/2020 11:51, Lynne wrote:
> Also documents all options supported by the hwdevice. 
> This lets users enable all extensions they need without writing their own
> instance initialization code.
> 
> Patch attached.
> 
> 
> From cf91acc2a907a5ff7af753bf3b2ab495dbc37db0 Mon Sep 17 00:00:00 2001
> From: Lynne <dev at lynne.ee>
> Date: Sun, 10 May 2020 11:26:40 +0100
> Subject: [PATCH 1/2] hwcontext_vulkan: let users enable device and instance
>  extensions using options
> 
> Also documents all options supported by the hwdevice.
> This lets users enable all extensions they need without writing their own
> instance initialization code.
> ---
>  doc/ffmpeg.texi              | 15 +++++++++++++++
>  libavutil/hwcontext_vulkan.c | 31 ++++++++++++++++++++++++++++---
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index 29753f06ca..d2a30fc868 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -1034,6 +1034,18 @@ If @var{device} is an integer, it selects the device by its index in a
>  system-dependent list of devices.  If @var{device} is any other string, it
>  selects the first device with a name containing that string as a substring.
>  
> +The following options are recognized:
> + at table @option
> + at item debug
> +If set to 1, enables the validation layer, if installed.
> + at item linear_images
> +If set to 1, images allocated by the hwcontext will be linear and locally mappable.
> + at item instance_extensions
> +A vertical bar separated list of additional instance extensions to enable.
> + at item device_extensions
> +A vertical bar separated list of additional device extensions to enable.
> + at end table
> +
>  Examples:
>  @table @emph
>  @item -init_hw_device vulkan:1
> @@ -1041,6 +1053,9 @@ Choose the second device on the system.
>  
>  @item -init_hw_device vulkan:RADV
>  Choose the first device with a name containing the string @emph{RADV}.
> +
> + at item -init_hw_device vulkan:0,instance_extensions=VK_KHR_wayland_surface|VK_KHR_xcb_surface
> +Choose the first device and enable the Wayland and XCB instance extensions.
>  @end table
>  
>  @end table
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index 43e7cddbc5..a35c1d3a4f 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -261,7 +261,7 @@ static VkBool32 vk_dbg_callback(VkDebugUtilsMessageSeverityFlagBitsEXT severity,
>      return 0;
>  }
>  
> -static int check_extensions(AVHWDeviceContext *ctx, int dev,
> +static int check_extensions(AVHWDeviceContext *ctx, int dev, AVDictionary *opts,
>                              const char * const **dst, uint32_t *num, int debug)
>  {
>      const char *tstr;
> @@ -274,12 +274,14 @@ static int check_extensions(AVHWDeviceContext *ctx, int dev,
>      int optional_exts_num;
>      uint32_t sup_ext_count;
>      VkExtensionProperties *sup_ext;
> +    AVDictionaryEntry *user_exts = NULL;
>      const VulkanOptExtension *optional_exts;
>  
>      if (!dev) {
>          mod = "instance";
>          optional_exts = optional_instance_exts;
>          optional_exts_num = FF_ARRAY_ELEMS(optional_instance_exts);
> +        user_exts = av_dict_get(opts, "instance_extensions", NULL, 0);w
>          vkEnumerateInstanceExtensionProperties(NULL, &sup_ext_count, NULL);
>          sup_ext = av_malloc_array(sup_ext_count, sizeof(VkExtensionProperties));
>          if (!sup_ext)
> @@ -289,6 +291,7 @@ static int check_extensions(AVHWDeviceContext *ctx, int dev,
>          mod = "device";
>          optional_exts = optional_device_exts;
>          optional_exts_num = FF_ARRAY_ELEMS(optional_device_exts);
> +        user_exts = av_dict_get(opts, "device_extensions", NULL, 0);
>          vkEnumerateDeviceExtensionProperties(hwctx->phys_dev, NULL,
>                                               &sup_ext_count, NULL);
>          sup_ext = av_malloc_array(sup_ext_count, sizeof(VkExtensionProperties));
> @@ -345,6 +348,28 @@ static int check_extensions(AVHWDeviceContext *ctx, int dev,
>          }
>      }
>  
> +    if (user_exts) {
> +        char *save, *token = av_strtok(user_exts->value, "|", &save);

Using strtok modifies the string, but that isn't allowed.  Maybe strdup() first?

> +        while (token) {
> +            found = 0;
> +            for (int j = 0; j < sup_ext_count; j++) {
> +                if (!strcmp(token, sup_ext[j].extensionName)) {
> +                    found = 1;
> +                    break;
> +                }
> +            }
> +            if (found) {
> +                ADD_VAL_TO_LIST(extension_names, extensions_found, token);
> +            } else {
> +                av_log(ctx, AV_LOG_ERROR, "%s extension \"%s\" not found!\n",
> +                       mod, token);
> +                err = AVERROR(EINVAL);
> +                goto end;
> +            }
> +            token = av_strtok(NULL, "|", &save);
> +        }
> +    }
> +
>      *dst = extension_names;
>      *num = extensions_found;
>  
> @@ -376,7 +401,7 @@ static int create_instance(AVHWDeviceContext *ctx, AVDictionary *opts)
>      };
>  
>      /* Check for present/missing extensions */
> -    err = check_extensions(ctx, 0, &inst_props.ppEnabledExtensionNames,
> +    err = check_extensions(ctx, 0, opts, &inst_props.ppEnabledExtensionNames,
>                             &inst_props.enabledExtensionCount, debug_mode);
>      if (err < 0)
>          return err;
> @@ -777,7 +802,7 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>      if ((err = search_queue_families(ctx, &dev_info)))
>          goto end;
>  
> -    if ((err = check_extensions(ctx, 1, &dev_info.ppEnabledExtensionNames,
> +    if ((err = check_extensions(ctx, 1, opts, &dev_info.ppEnabledExtensionNames,
>                                  &dev_info.enabledExtensionCount, 0)))
>          goto end;
>  
> -- 
> 2.26.2
> 

A nitpick: '+' separation might be nicer than '|'?  It's a combination of values which should be combined like "plus" (as in "flags=a+b+c"), rather than a set you pick one of like "or" (as in "format=a|b|c").

Rest all looks good to me.

Thanks,

- Mark