[FFmpeg-devel,1/2] hwcontext_vulkan: expose the amount of queues for each queue family

This, along with the next patch, are the last missing pieces to being 
interoperable with libplacebo.
There is no danger of users running into this API break because there are none,
and API was completely backwards-incompatibly changed just 2 days ago.
This is needed so we won't have to break the API anymore in the future.

Patch attached.
Subject: [PATCH 1/2] hwcontext_vulkan: expose the amount of queues for each
 queue family

This, along with the next patch, are the last missing pieces to being
interoperable with libplacebo.
---
 libavutil/hwcontext_vulkan.c |  3 +++
 libavutil/hwcontext_vulkan.h | 14 ++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

On 13/05/2020 16:53, Lynne wrote:
> This, along with the next patch, are the last missing pieces to being 
> interoperable with libplacebo.
> There is no danger of users running into this API break because there are none,
> and API was completely backwards-incompatibly changed just 2 days ago.
> This is needed so we won't have to break the API anymore in the future.

?  The previous change wasn't an ABI break after the fields were rearranged properly.

> From 63a95c6848e44f365256dbe10c0e1fa595a0f679 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Wed, 13 May 2020 16:20:15 +0100
> Subject: [PATCH 1/2] hwcontext_vulkan: expose the amount of queues for each
>  queue family
> 
> This, along with the next patch, are the last missing pieces to being
> interoperable with libplacebo.
> ---
>  libavutil/hwcontext_vulkan.c |  3 +++
>  libavutil/hwcontext_vulkan.h | 14 ++++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index 8f3f3fdd2a..82ceb7013a 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -679,16 +679,19 @@ static int search_queue_families(AVHWDeviceContext *ctx, VkDeviceCreateInfo *cd)
>      hwctx->queue_family_index      = graph_index;
>      hwctx->queue_family_comp_index = graph_index;
>      hwctx->queue_family_tx_index   = graph_index;
> +    hwctx->nb_graphics_queues      = qs[graph_index].queueCount;
>  
>      if (comp_index != -1) {
>          ADD_QUEUE(comp_index, 0, 1, tx_index < 0)
>          hwctx->queue_family_tx_index   = comp_index;
>          hwctx->queue_family_comp_index = comp_index;
> +        hwctx->nb_comp_queues          = qs[comp_index].queueCount;
>      }
>  
>      if (tx_index != -1) {
>          ADD_QUEUE(tx_index, 0, 0, 1)
>          hwctx->queue_family_tx_index = tx_index;
> +        hwctx->nb_tx_queues          = qs[tx_index].queueCount;
>      }
>  
>  #undef ADD_QUEUE
> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
> index c3fc14af70..9fbe8b9dcb 100644
> --- a/libavutil/hwcontext_vulkan.h
> +++ b/libavutil/hwcontext_vulkan.h
> @@ -55,16 +55,26 @@ typedef struct AVVulkanDeviceContext {
>       * @note av_hwdevice_create() will set all 3 queue indices if unset
>       * If there is no dedicated queue for compute or transfer operations,
>       * they will be set to the graphics queue index which can handle both.
> +     * nb_graphics_queues indicates how many queues were enabled for the
> +     * graphics queue (must be at least 1)
>       */
>      int queue_family_index;
> +    int nb_graphics_queues;
>      /**
> -     * Queue family index for transfer ops only
> +     * Queue family index to use for transfer operations, and the amount of queues
> +     * enabled. In case there is no dedicated transfer queue, nb_tx_queues
> +     * must be 0 and queue_family_tx_index must be the same as either the graphics
> +     * queue or the compute queue, if available.
>       */
>      int queue_family_tx_index;
> +    int nb_tx_queues;
>      /**
> -     * Queue family index for compute ops
> +     * Queue family index for compute ops, and the amount of queues enabled.
> +     * In case there are no dedicated compute queues, nb_comp_queues must be
> +     * 0 and its queue family index must be set to the graphics queue.
>       */
>      int queue_family_comp_index;
> +    int nb_comp_queues;
>      /**
>       * 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
> -- 
> 2.26.2

Put the new fields at the end.  It looks like that combined with zero implying the number of queues available in that queue family would be sufficient to maintain compatibility?

Under what circumstances would a user create an AVVulkanDeviceContext which does not set these to their maximum value?  Will you want to expose some option so that a device created by lavu can do the same thing?

- Mark

May 19, 2020, 20:50 by sw@jkqxz.net:

> On 13/05/2020 16:53, Lynne wrote:
>
>> This, along with the next patch, are the last missing pieces to being 
>> interoperable with libplacebo.
>> There is no danger of users running into this API break because there are none,
>> and API was completely backwards-incompatibly changed just 2 days ago.
>> This is needed so we won't have to break the API anymore in the future.
>>
>
> ?  The previous change wasn't an ABI break after the fields were rearranged properly.
>

The number of fences per image was what I was referring to.



>> From 63a95c6848e44f365256dbe10c0e1fa595a0f679 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Wed, 13 May 2020 16:20:15 +0100
>> Subject: [PATCH 1/2] hwcontext_vulkan: expose the amount of queues for each
>>  queue family
>>
>> This, along with the next patch, are the last missing pieces to being
>> interoperable with libplacebo.
>> ---
>>  libavutil/hwcontext_vulkan.c |  3 +++
>>  libavutil/hwcontext_vulkan.h | 14 ++++++++++++--
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>> index 8f3f3fdd2a..82ceb7013a 100644
>> --- a/libavutil/hwcontext_vulkan.c
>> +++ b/libavutil/hwcontext_vulkan.c
>> @@ -679,16 +679,19 @@ static int search_queue_families(AVHWDeviceContext *ctx, VkDeviceCreateInfo *cd)
>>  hwctx->queue_family_index      = graph_index;
>>  hwctx->queue_family_comp_index = graph_index;
>>  hwctx->queue_family_tx_index   = graph_index;
>> +    hwctx->nb_graphics_queues      = qs[graph_index].queueCount;
>>  
>>  if (comp_index != -1) {
>>  ADD_QUEUE(comp_index, 0, 1, tx_index < 0)
>>  hwctx->queue_family_tx_index   = comp_index;
>>  hwctx->queue_family_comp_index = comp_index;
>> +        hwctx->nb_comp_queues          = qs[comp_index].queueCount;
>>  }
>>  
>>  if (tx_index != -1) {
>>  ADD_QUEUE(tx_index, 0, 0, 1)
>>  hwctx->queue_family_tx_index = tx_index;
>> +        hwctx->nb_tx_queues          = qs[tx_index].queueCount;
>>  }
>>  
>>  #undef ADD_QUEUE
>> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
>> index c3fc14af70..9fbe8b9dcb 100644
>> --- a/libavutil/hwcontext_vulkan.h
>> +++ b/libavutil/hwcontext_vulkan.h
>> @@ -55,16 +55,26 @@ typedef struct AVVulkanDeviceContext {
>>  * @note av_hwdevice_create() will set all 3 queue indices if unset
>>  * If there is no dedicated queue for compute or transfer operations,
>>  * they will be set to the graphics queue index which can handle both.
>> +     * nb_graphics_queues indicates how many queues were enabled for the
>> +     * graphics queue (must be at least 1)
>>  */
>>  int queue_family_index;
>> +    int nb_graphics_queues;
>>  /**
>> -     * Queue family index for transfer ops only
>> +     * Queue family index to use for transfer operations, and the amount of queues
>> +     * enabled. In case there is no dedicated transfer queue, nb_tx_queues
>> +     * must be 0 and queue_family_tx_index must be the same as either the graphics
>> +     * queue or the compute queue, if available.
>>  */
>>  int queue_family_tx_index;
>> +    int nb_tx_queues;
>>  /**
>> -     * Queue family index for compute ops
>> +     * Queue family index for compute ops, and the amount of queues enabled.
>> +     * In case there are no dedicated compute queues, nb_comp_queues must be
>> +     * 0 and its queue family index must be set to the graphics queue.
>>  */
>>  int queue_family_comp_index;
>> +    int nb_comp_queues;
>>  /**
>>  * 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
>> -- 
>> 2.26.2
>>
>
> Put the new fields at the end.  It looks like that combined with zero implying the number of queues available in that queue family would be sufficient to maintain compatibility?
>

nb_graphics_queues is required to be set to at least one.
There is absolutely no one to keep compatibility for, as there's no one using this API at all (since it was
not usable as-is).
I'd like to have the header looking nicely for the release, and like the commit message said, this is the
last change we need to make, so I'd really rather not change this.
As for why this wasn't done earlier, its because there were only a few API users to really integrate with,
and those API users had not really thought about making their own API interoperable with everyone
else's, so like us, they exposed the minimal amount of information needed.
It was only very recently that libplacebo gained support for being initialized with an external device and
instance, and even then, like us, it lacked certain aspects that allowed interoperability, so we had to
work to solve those issues. We had at least thought of this before, so there were only a few modifications
we had to make, but considering the size of the Vulkan spec and that there was a single person working
on this (and another person working on libplacebo), and we both made the same mistakes, it really
 showed anyone could make these mistakes while assuming the API would be interoperable with
anyone else's.



> Under what circumstances would a user create an AVVulkanDeviceContext which does not set these to their maximum value?  Will you want to expose some option so that a device created by lavu can do the same thing?
>

An API user would set these to less than the total amount of queues a device has in case they already have something to keep their queues busy.
One example I can think of is uploading frames to be presented in a 3D (game) pipeline. Your graphics
pipeline is busy drawing, you're using the compute queues to simulate physics, and you leave one
compute queue to lavu to handle uploads.
Queues are scheduled like programs on a CPU, so a single queue is not dedicated to a single program,
but unlike with CPUs, you have to choose which queue you run on, which a library shouldn't choose by
itself if its used as a library. There is no way of knowing how many submissions a queue has scheduled
on it either.
I don't get what you mean by exposing an option? If you mean having an AVDictionary option to set
the number of queues to use, I can't think of a situation in which that would be required, perhaps maybe
some scenario where you're doing a screen capture, and uploading frames, and you have a game running
in the background, and you'd like to use a hardware encoder while keeping queue impact down to a
minimum.
But that's a rather contrived scenario, and we don't need to add an API for this, since it would be just a
regular option, so I think we can add that later if there's a need.

On 19/05/2020 22:29, Lynne wrote:
> May 19, 2020, 20:50 by sw@jkqxz.net:
> 
>> On 13/05/2020 16:53, Lynne wrote:
>>
>>> This, along with the next patch, are the last missing pieces to being 
>>> interoperable with libplacebo.
>>> There is no danger of users running into this API break because there are none,
>>> and API was completely backwards-incompatibly changed just 2 days ago.
>>> This is needed so we won't have to break the API anymore in the future.
>>>
>>
>> ?  The previous change wasn't an ABI break after the fields were rearranged properly.
>>
> 
> The number of fences per image was what I was referring to.

Huh, right - I hadn't seen those two breaks at all and they weren't mentioned in APIchanges.  If you are going to apply multiple changes which break API and ABI like this then perhaps it would be better to note in the relevant headers that the Vulkan API is experimental and compatibility may be broken unexpectedly?  That would avoid this discussion being repeated.

>>> From 63a95c6848e44f365256dbe10c0e1fa595a0f679 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev@lynne.ee>
>>> Date: Wed, 13 May 2020 16:20:15 +0100
>>> Subject: [PATCH 1/2] hwcontext_vulkan: expose the amount of queues for each
>>>  queue family
>>>
>>> This, along with the next patch, are the last missing pieces to being
>>> interoperable with libplacebo.
>>> ---
>>>  libavutil/hwcontext_vulkan.c |  3 +++
>>>  libavutil/hwcontext_vulkan.h | 14 ++++++++++++--
>>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>>> index 8f3f3fdd2a..82ceb7013a 100644
>>> --- a/libavutil/hwcontext_vulkan.c
>>> +++ b/libavutil/hwcontext_vulkan.c
>>> @@ -679,16 +679,19 @@ static int search_queue_families(AVHWDeviceContext *ctx, VkDeviceCreateInfo *cd)
>>>  hwctx->queue_family_index      = graph_index;
>>>  hwctx->queue_family_comp_index = graph_index;
>>>  hwctx->queue_family_tx_index   = graph_index;
>>> +    hwctx->nb_graphics_queues      = qs[graph_index].queueCount;
>>>  
>>>  if (comp_index != -1) {
>>>  ADD_QUEUE(comp_index, 0, 1, tx_index < 0)
>>>  hwctx->queue_family_tx_index   = comp_index;
>>>  hwctx->queue_family_comp_index = comp_index;
>>> +        hwctx->nb_comp_queues          = qs[comp_index].queueCount;
>>>  }
>>>  
>>>  if (tx_index != -1) {
>>>  ADD_QUEUE(tx_index, 0, 0, 1)
>>>  hwctx->queue_family_tx_index = tx_index;
>>> +        hwctx->nb_tx_queues          = qs[tx_index].queueCount;
>>>  }
>>>  
>>>  #undef ADD_QUEUE
>>> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
>>> index c3fc14af70..9fbe8b9dcb 100644
>>> --- a/libavutil/hwcontext_vulkan.h
>>> +++ b/libavutil/hwcontext_vulkan.h
>>> @@ -55,16 +55,26 @@ typedef struct AVVulkanDeviceContext {
>>>  * @note av_hwdevice_create() will set all 3 queue indices if unset
>>>  * If there is no dedicated queue for compute or transfer operations,
>>>  * they will be set to the graphics queue index which can handle both.
>>> +     * nb_graphics_queues indicates how many queues were enabled for the
>>> +     * graphics queue (must be at least 1)
>>>  */
>>>  int queue_family_index;
>>> +    int nb_graphics_queues;
>>>  /**
>>> -     * Queue family index for transfer ops only
>>> +     * Queue family index to use for transfer operations, and the amount of queues
>>> +     * enabled. In case there is no dedicated transfer queue, nb_tx_queues
>>> +     * must be 0 and queue_family_tx_index must be the same as either the graphics
>>> +     * queue or the compute queue, if available.
>>>  */
>>>  int queue_family_tx_index;
>>> +    int nb_tx_queues;
>>>  /**
>>> -     * Queue family index for compute ops
>>> +     * Queue family index for compute ops, and the amount of queues enabled.
>>> +     * In case there are no dedicated compute queues, nb_comp_queues must be
>>> +     * 0 and its queue family index must be set to the graphics queue.
>>>  */
>>>  int queue_family_comp_index;
>>> +    int nb_comp_queues;
>>>  /**
>>>  * 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
>>> -- 
>>> 2.26.2
>>>
>>
>> Put the new fields at the end.  It looks like that combined with zero implying the number of queues available in that queue family would be sufficient to maintain compatibility?
>>
> 
> nb_graphics_queues is required to be set to at least one.
> There is absolutely no one to keep compatibility for, as there's no one using this API at all (since it was
> not usable as-is).
> I'd like to have the header looking nicely for the release, and like the commit message said, this is the
> last change we need to make, so I'd really rather not change this.
> As for why this wasn't done earlier, its because there were only a few API users to really integrate with,
> and those API users had not really thought about making their own API interoperable with everyone
> else's, so like us, they exposed the minimal amount of information needed.
> It was only very recently that libplacebo gained support for being initialized with an external device and
> instance, and even then, like us, it lacked certain aspects that allowed interoperability, so we had to
> work to solve those issues. We had at least thought of this before, so there were only a few modifications
> we had to make, but considering the size of the Vulkan spec and that there was a single person working
> on this (and another person working on libplacebo), and we both made the same mistakes, it really
>  showed anyone could make these mistakes while assuming the API would be interoperable with
> anyone else's.
> 
> 
> 
>> Under what circumstances would a user create an AVVulkanDeviceContext which does not set these to their maximum value?  Will you want to expose some option so that a device created by lavu can do the same thing?
>>
> 
> An API user would set these to less than the total amount of queues a device has in case they already have something to keep their queues busy.
> One example I can think of is uploading frames to be presented in a 3D (game) pipeline. Your graphics
> pipeline is busy drawing, you're using the compute queues to simulate physics, and you leave one
> compute queue to lavu to handle uploads.
> Queues are scheduled like programs on a CPU, so a single queue is not dedicated to a single program,
> but unlike with CPUs, you have to choose which queue you run on, which a library shouldn't choose by
> itself if its used as a library. There is no way of knowing how many submissions a queue has scheduled
> on it either.
> I don't get what you mean by exposing an option? If you mean having an AVDictionary option to set
> the number of queues to use, I can't think of a situation in which that would be required, perhaps maybe
> some scenario where you're doing a screen capture, and uploading frames, and you have a game running
> in the background, and you'd like to use a hardware encoder while keeping queue impact down to a
> minimum.
> But that's a rather contrived scenario, and we don't need to add an API for this, since it would be just a
> regular option, so I think we can add that later if there's a need.

I was imagining it could somehow help when using Vulkan in the ffmpeg.c case.  Still, that makes sense, so fair enough.

- Mark

May 20, 2020, 22:43 by sw@jkqxz.net:

> On 19/05/2020 22:29, Lynne wrote:
>
>> May 19, 2020, 20:50 by sw@jkqxz.net:
>>
>>> On 13/05/2020 16:53, Lynne wrote:
>>>
>>>> This, along with the next patch, are the last missing pieces to being 
>>>> interoperable with libplacebo.
>>>> There is no danger of users running into this API break because there are none,
>>>> and API was completely backwards-incompatibly changed just 2 days ago.
>>>> This is needed so we won't have to break the API anymore in the future.
>>>>
>>>
>>> ?  The previous change wasn't an ABI break after the fields were rearranged properly.
>>>
>>
>> The number of fences per image was what I was referring to.
>>
>
> Huh, right - I hadn't seen those two breaks at all and they weren't mentioned in APIchanges.  If you are going to apply multiple changes which break API and ABI like this then perhaps it would be better to note in the relevant headers that the Vulkan API is experimental and compatibility may be broken unexpectedly?  That would avoid this discussion being repeated.
>

We've ironed out all the issues regarding using the API externally, and I've spend a week now thinking
about if there are any other hitches that would require us to break the API, and I couldn't really think of any.
So with those changes, I'll  freeze it ABI/API wise.

The decoding (and encoding?) extensions may require additions to the frame structure, but with it
not being part of the ABI (I think it was a really good decision) we can extend it without breaking anything.



> I was imagining it could somehow help when using Vulkan in the ffmpeg.c case.  Still, that makes sense, so fair enough.
>

Reducing the total amount of queues ffmpeg.c uses would just slow things, but would leave more
resources for other programs, so that's why I hinted we may want to add that as an option in the future,
but as-is, I don't see much point adding that now.

Pushed, thanks for the review.