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

[Mark Thompson]

On 19/05/2020 22:29, Lynne wrote:
> May 19, 2020, 20:50 by sw at 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 at 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