diff mbox series

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

Message ID M6yDDCK--3-2@lynne.ee
State Accepted
Headers show
Series [FFmpeg-devel,1/2] hwcontext_vulkan: let users enable device and instance extensions using options | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lynne May 10, 2020, 10:54 a.m. UTC 
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.
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(-)

Comments

Mark Thompson May 10, 2020, 1:33 p.m. UTC | #1 
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@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
Lynne May 10, 2020, 2:23 p.m. UTC | #2 
May 10, 2020, 14:33 by sw@jkqxz.net:

> On 10/05/2020 11:54, Lynne wrote:
>
>>  */
>>  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.)
>

Done.



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

Done (it isn't required, and can be NULL).



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

Done, with a minor lavu bump and an APIchanges notice. Looks tidier with the fields at the bottom anyway.

Patch attached.
Mark Thompson May 10, 2020, 9:39 p.m. UTC | #3 
On 10/05/2020 15:23, Lynne wrote:
> May 10, 2020, 14:33 by sw@jkqxz.net:
>> On 10/05/2020 11:54, Lynne wrote:
>>>  ...
> 
> From 6ecc3547bcfcc450c8ffe8d93a3040fd863f6288 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Sun, 10 May 2020 11:47:50 +0100
> Subject: [PATCH 2/3] 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.
> ---
>  doc/APIchanges               |  4 ++++
>  libavutil/hwcontext_vulkan.c | 40 ++++++++++++++++++++++++++++--------
>  libavutil/hwcontext_vulkan.h | 21 ++++++++++++++++++-
>  libavutil/version.h          |  2 +-
>  4 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index b3e7e89412..75cfdb08b0 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +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
> +
>  2020-04-22 - 0e1db79e37 - lavc 58.81.100 - packet.h
>                          - lavu 56.43.100 - dovi_meta.h
>    Add AV_PKT_DATA_DOVI_CONF and AVDOVIDecoderConfigurationRecord.
> ...
> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
> index ebc28916f3..909f88a7e4 100644
> --- a/libavutil/hwcontext_vulkan.h
> +++ b/libavutil/hwcontext_vulkan.h
> @@ -39,7 +39,7 @@ typedef struct AVVulkanDeviceContext {
>       */
>      const VkAllocationCallbacks *alloc;
>      /**
> -     * Instance
> +     * Vulkan instance. Must be at least version 1.1.
>       */
>      VkInstance inst;
>      /**
> @@ -65,6 +65,25 @@ typedef struct AVVulkanDeviceContext {
>       * Queue family index for compute ops
>       */
>      int queue_family_comp_index;
> +    /**
> +     * Enabled instance extensions. By default, VK_KHR_surface is enabled if found.
> +     * If supplying your own device context, set this to an array of strings, with
> +     * each entry containing the specified Vulkan extension string to enable.
> +     * Duplicates are possible and accepted.
> +     * If no extensions are enabled, set these fields to NULL, and 0 respectively.
> +     */
> +    const char * const *enabled_inst_extensions;
> +    int num_enabled_inst_extensions;
> +    /**
> +     * 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.
> +     * If supplying your own device context, these fields takes the same format as
> +     * the above fields, with the same conditions that duplicates are possible
> +     * and accepted, and that NULL and 0 respectively means no extensions are enabled.
> +     */

That's much clearer, thank you :)

> +    const char * const *enabled_dev_extensions;
> +    int num_enabled_dev_extensions;

A minor stylistic niggle - fields like this are "nb_something" rather than "num_something" in pretty much all other ffmpeg headers.

>  } AVVulkanDeviceContext;
>  
>  /**
> diff --git a/libavutil/version.h b/libavutil/version.h
> index ea9363e8e9..48d8a38c42 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  43
> +#define LIBAVUTIL_VERSION_MINOR  44
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> -- 
> 2.26.2

LGTM with or without that change.

Thanks,

- Mark
Lynne May 10, 2020, 10:24 p.m. UTC | #4 
May 10, 2020, 22:39 by sw@jkqxz.net:

> On 10/05/2020 15:23, Lynne wrote:
>
>> May 10, 2020, 14:33 by sw@jkqxz.net:
>>
>>> On 10/05/2020 11:54, Lynne wrote:
>>>
>>>> ...
>>>>
>>
>> From 6ecc3547bcfcc450c8ffe8d93a3040fd863f6288 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Sun, 10 May 2020 11:47:50 +0100
>> Subject: [PATCH 2/3] 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.
>> ---
>>  doc/APIchanges               |  4 ++++
>>  libavutil/hwcontext_vulkan.c | 40 ++++++++++++++++++++++++++++--------
>>  libavutil/hwcontext_vulkan.h | 21 ++++++++++++++++++-
>>  libavutil/version.h          |  2 +-
>>  4 files changed, 56 insertions(+), 11 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index b3e7e89412..75cfdb08b0 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>  
>>  API changes, most recent first:
>>  
>> +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
>> +
>>  2020-04-22 - 0e1db79e37 - lavc 58.81.100 - packet.h
>>  - lavu 56.43.100 - dovi_meta.h
>>  Add AV_PKT_DATA_DOVI_CONF and AVDOVIDecoderConfigurationRecord.
>> ...
>> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
>> index ebc28916f3..909f88a7e4 100644
>> --- a/libavutil/hwcontext_vulkan.h
>> +++ b/libavutil/hwcontext_vulkan.h
>> @@ -39,7 +39,7 @@ typedef struct AVVulkanDeviceContext {
>>  */
>>  const VkAllocationCallbacks *alloc;
>>  /**
>> -     * Instance
>> +     * Vulkan instance. Must be at least version 1.1.
>>  */
>>  VkInstance inst;
>>  /**
>> @@ -65,6 +65,25 @@ typedef struct AVVulkanDeviceContext {
>>  * Queue family index for compute ops
>>  */
>>  int queue_family_comp_index;
>> +    /**
>> +     * Enabled instance extensions. By default, VK_KHR_surface is enabled if found.
>> +     * If supplying your own device context, set this to an array of strings, with
>> +     * each entry containing the specified Vulkan extension string to enable.
>> +     * Duplicates are possible and accepted.
>> +     * If no extensions are enabled, set these fields to NULL, and 0 respectively.
>> +     */
>> +    const char * const *enabled_inst_extensions;
>> +    int num_enabled_inst_extensions;
>> +    /**
>> +     * 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.
>> +     * If supplying your own device context, these fields takes the same format as
>> +     * the above fields, with the same conditions that duplicates are possible
>> +     * and accepted, and that NULL and 0 respectively means no extensions are enabled.
>> +     */
>>
>
> That's much clearer, thank you :)
>
>> +    const char * const *enabled_dev_extensions;
>> +    int num_enabled_dev_extensions;
>>
>
> A minor stylistic niggle - fields like this are "nb_something" rather than "num_something" in pretty much all other ffmpeg headers.
>

Pushed with that change, thanks for reviewing.
diff mbox series

Patch

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.
+     */
+    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.
+     */
+    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