[FFmpeg-devel] [PATCH] Vulkan hwcontext and filters

[Lynne]

Jan 21, 2020, 12:08 by anton at khirnov.net:

> Quoting Lynne (2020-01-10 22:05:21)
>
>>  /**
>>  * Activated physical device
>>  */
>>  VkDevice act_dev;
>>
>
> Where is this 'activated' terminology from? From what I can see, the
> spec calls this a 'logical' device. Would be nice to be consistent with
> it.
>

Its just the current active device. Not really wanting to change it, its pretty clear what it is from the type.



>> /**
>>  * Queue family index for graphics
>>  */
>>  int queue_family_index;
>>
>
> What is this for? I don't see it used anywhere.
>

Its required to do anything graphics-related (like non-compute shaders, which lavfi will support in a future patch). You need to know it at init time and at runtime, and there's nothing you can call to know what its been initialized as.


>> /**
>>  * Queue family index for transfer ops only. By default, the priority order
>>  * is dedicated transfer > dedicated compute > graphics.
>>  */
>>
>
> IIUC the second sentence talks about how the internal device
> creation/derivation code works, but that isn't made very clear. Maybe
> make it a
> @note the av_hwdevice_create() will set those fields like this...
>

Done.



>> int queue_family_tx_index;
>>  /**
>>  * Queue family index for compute ops. Will be equal to the graphics
>>  * one unless a dedicated transfer queue is found.
>>  */
>>  int queue_family_comp_index;
>>
>
> Same comment as above.
>

Fixed.



>> /**
>>  * The UUID of the selected physical device.
>>  */
>>  uint8_t device_uuid[VK_UUID_SIZE];
>>
>
> Do we really need to export this here? It can be queried from phys_dev,
> no?
>

Actually no, so removed it and changed hwcontext_cuda to use vkGetPhysicalDeviceProperties2.



>> } AVVulkanDeviceContext;
>>
>> /**
>>  * Allocated as AVHWFramesContext.hwctx, used to set pool-specific options
>>  */
>> typedef struct AVVulkanFramesContext {
>>  /**
>>  * Controls the tiling of output frames.
>>  */
>>  VkImageTiling tiling;
>>  /**
>>  * Defines extra usage of output frames. This is bitwise OR'd with the
>>  * standard usage flags (SAMPLED, STORAGE, TRANSFER_SRC and TRANSFER_DST).
>>  */
>>  VkImageUsageFlagBits usage;
>>  /**
>>  * Extension data for image creation. By default, if the extension is
>>  * available, this will be chained to a VkImageFormatListCreateInfoKHR.
>>  */
>>  void *create_pnext;
>>  /**
>>  * Extension data for memory allocation. Must have as many entries as
>>  * the number of planes of the sw_format.
>>  * This will be chained to VkExportMemoryAllocateInfo, which is used
>>  * to make all pool images exportable to other APIs.
>>  */
>>  void *alloc_pnext[AV_NUM_DATA_POINTERS];
>> } AVVulkanFramesContext;
>>
>> /*
>>  * Frame structure, the VkFormat of the image will always match
>>  * the pool's sw_format.
>>  * All frames, imported or allocated, will be created with the
>>  * VK_IMAGE_CREATE_ALIAS_BIT flag set, so the memory may be aliased if needed.
>>  */
>> typedef struct AVVkFrame {
>>  /**
>>  * Vulkan images to which the memory is bound to.
>>  */
>>  VkImage img[AV_NUM_DATA_POINTERS];
>>
>>  /**
>>  * Same tiling must be used for all images.
>>  */
>>  VkImageTiling tiling;
>>
>>  /**
>>  * Memory backing the images. Could be less than the amount of images
>>  * if importing from a DRM or VAAPI frame.
>>  */
>>  VkDeviceMemory mem[AV_NUM_DATA_POINTERS];
>>  size_t size[AV_NUM_DATA_POINTERS];
>>
>>  /**
>>  * OR'd flags for all memory allocated
>>  */
>>  VkMemoryPropertyFlagBits flags;
>>
>>  /**
>>  * Updated after every barrier
>>  */
>>  VkAccessFlagBits access[AV_NUM_DATA_POINTERS];
>>  VkImageLayout layout[AV_NUM_DATA_POINTERS];
>>
>>  /**
>>  * Per-image semaphores. Must not be freed manually. Must be waited on
>>  * and signalled at every queue submission.
>>  */
>>  VkSemaphore sem[AV_NUM_DATA_POINTERS];
>>
>>  /**
>>  * Internal data.
>>  */
>>  struct AVVkFrameInternal *internal;
>> } AVVkFrame;
>>
>
> This is a pretty big and complicated struct and its size has to be
> hardcoded in the ABI IIUC. Do we want a constructor for it?
>

Its size too? Didn't know that.
I do think its a good idea to be able to append fields to it, so I've added a av_vk_frame_alloc() function. I've followed what av_frame_alloc is called, though I'm not sure if its too close to that one's name.

Changes are in git.