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

[Anton Khirnov]

Quoting Lynne (2020-01-21 18:51:22)
> 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.

Can't really force you, but I'd prefer you did change it. At least drop
'physical' from the doxy, since the spec makes a distinction between
'physical' and 'logical' devices and this is not a physical device.

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

At init time of what? The filter or the hwdevice? I'm wondering if you
can have a situation where you want to use different queue families for
different filters (on a single device).

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

My original intent with this API was that calles are allowed to provide
their own pools. Not sure if that's still allowed or works though. But
if it is, the caller needs to be able to allocate/free AVkFrame.

-- 
Anton Khirnov