[FFmpeg-devel] [PATCH v3 2/2] lavc/vulkan_av1: port to the new stable API
Lynne
dev at lynne.ee
Mon Mar 25 09:57:47 EET 2024
Mar 20, 2024, 00:33 by sw at jkqxz.net:
> On 18/03/2024 05:33, Lynne wrote:
>
>> Mar 17, 2024, 16:36 by sw at jkqxz.net:
>>
>>> On 13/03/2024 16:38, Lynne wrote:
>>>
>>>> Tested by multiple users on multiple operating systems,
>>>> driver implementations and test samples to work.
>>>>
>>>> Co-Authored-by: Dave Airlie <airlied at redhat.com>
>>>>
>>>> From e2d31951f46fcc10e1263b8603e486e111e9578c Mon Sep 17 00:00:00 2001
>>>> From: Lynne <dev at lynne.ee>
>>>> Date: Fri, 19 Jan 2024 10:49:02 +1000
>>>> Subject: [PATCH v3 2/2] lavc/vulkan_av1: port to the new stable API
>>>>
>>>> Co-Authored-by: Dave Airlie <airlied at redhat.com>
>>>> ---
>>>> configure | 4 +-
>>>> libavcodec/Makefile | 5 +-
>>>> libavcodec/av1.h | 2 +
>>>> libavcodec/vulkan_av1.c | 502 ++++++++++--------
>>>> libavcodec/vulkan_decode.c | 31 +-
>>>> libavcodec/vulkan_decode.h | 2 +-
>>>> libavcodec/vulkan_video.h | 2 -
>>>> .../vulkan_video_codec_av1std_decode_mesa.h | 36 --
>>>> libavcodec/vulkan_video_codec_av1std_mesa.h | 403 --------------
>>>> libavutil/hwcontext_vulkan.c | 2 +-
>>>> libavutil/vulkan_functions.h | 2 +-
>>>> libavutil/vulkan_loader.h | 2 +-
>>>> 12 files changed, 300 insertions(+), 693 deletions(-)
>>>> delete mode 100644 libavcodec/vulkan_video_codec_av1std_decode_mesa.h
>>>> delete mode 100644 libavcodec/vulkan_video_codec_av1std_mesa.h
>>>>
>>>> diff --git a/configure b/configure
>>>> index 05f8283af9..b07a742a74 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -7229,8 +7229,8 @@ enabled vdpau &&
>>>> check_lib vdpau_x11 "vdpau/vdpau.h vdpau/vdpau_x11.h" vdp_device_create_x11 -lvdpau -lX11
>>>>
>>>> if enabled vulkan; then
>>>> - check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" ||
>>>> - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)"
>>>> + check_pkg_config_header_only vulkan "vulkan >= 1.3.277" "vulkan/vulkan.h" "defined VK_VERSION_1_3" ||
>>>> + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 277)"
>>>>
>>>
>>> This bumping the requirement to the latest version needs to stop at some point. Vulkan is not an experimental thing any more, it should be usable in released distributions.
>>>
>>
>> The headers are a build-time dep that anyone can copy over without
>> installing.
>> Do you insist on making this optional? I'd rather not quite start
>> ifdeffing code, but if you think so, I will.
>>
>
> Ok for now, but we really should make this better soon so that normal people will get Vulkan support without a special effort to install the latest headers.
>
>>>> if disabled vulkan; then
>>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>>>> index 708434ac76..c552f034b7 100644
>>>> ...
>>>> diff --git a/libavcodec/vulkan_av1.c b/libavcodec/vulkan_av1.c
>>>> index 5afd5353cc..dc71ecf1fa 100644
>>>> --- a/libavcodec/vulkan_av1.c
>>>> +++ b/libavcodec/vulkan_av1.c
>>>> @@ -26,7 +26,7 @@
>>>> const FFVulkanDecodeDescriptor ff_vk_dec_av1_desc = {
>>>> .codec_id = AV_CODEC_ID_AV1,
>>>> .decode_extension = FF_VK_EXT_VIDEO_DECODE_AV1,
>>>> - .decode_op = 0x01000000, /* TODO fix this */
>>>> + .decode_op = VK_VIDEO_CODEC_OPERATION_DECODE_AV1_BIT_KHR,
>>>> .ext_props = {
>>>> .extensionName = VK_STD_VULKAN_VIDEO_CODEC_AV1_DECODE_EXTENSION_NAME,
>>>> .specVersion = VK_STD_VULKAN_VIDEO_CODEC_AV1_DECODE_SPEC_VERSION,
>>>> @@ -36,22 +36,34 @@ const FFVulkanDecodeDescriptor ff_vk_dec_av1_desc = {
>>>> typedef struct AV1VulkanDecodePicture {
>>>> FFVulkanDecodePicture vp;
>>>>
>>>> - /* Workaround for a spec issue.
>>>> - *Can be removed once no longer needed, and threading can be enabled. */
>>>> + /* TODO: investigate if this can be removed to make decoding completely
>>>> + * independent. */
>>>> FFVulkanDecodeContext *dec;
>>>>
>>>
>>> Can you explain what the id_alloc_mask thing is doing which needs this? (The 32 size in particular seems suspicious.)
>>>
>>
>> It tracks allocated frames to assign a unique index to them.
>> Indices for each frame are then given to the decoder via referenceNameSlotIndices.
>> The decoder needs to know this, as it keeps state for each frame that
>> may be internal and opaque to us.
>>
>> Dave can explain it better.
>>
>
> Ok, makes sense. Maybe add a bit more of a comment saying this, and have a clearer bound than the 32 - presumably AV1_NUM_REF_FRAMES?
>
>>>> - StdVideoAV1MESATile tiles[MAX_TILES];
>>>> - StdVideoAV1MESATileList tile_list;
>>>> - const uint32_t *tile_offsets;
>>>> + uint32_t tile_sizes[MAX_TILES];
>>>>
>>>> /* Current picture */
>>>> - VkVideoDecodeAV1DpbSlotInfoMESA vkav1_ref;
>>>> - StdVideoAV1MESAFrameHeader av1_frame_header;
>>>> - VkVideoDecodeAV1PictureInfoMESA av1_pic_info;
>>>> + StdVideoDecodeAV1ReferenceInfo std_ref;
>>>> + VkVideoDecodeAV1DpbSlotInfoKHR vkav1_ref;
>>>> + uint16_t width_in_sbs_minus1[64];
>>>> + uint16_t height_in_sbs_minus1[64];
>>>> + uint16_t mi_col_starts[64];
>>>> + uint16_t mi_row_starts[64];
>>>> + StdVideoAV1TileInfo tile_info;
>>>> + StdVideoAV1Quantization quantization;
>>>> + StdVideoAV1Segmentation segmentation;
>>>> + StdVideoAV1LoopFilter loop_filter;
>>>> + StdVideoAV1CDEF cdef;
>>>> + StdVideoAV1LoopRestoration loop_restoration;
>>>> + StdVideoAV1GlobalMotion global_motion;
>>>> + StdVideoAV1FilmGrain film_grain;
>>>> + StdVideoDecodeAV1PictureInfo std_pic_info;
>>>> + VkVideoDecodeAV1PictureInfoKHR av1_pic_info;
>>>>
>>>> /* Picture refs */
>>>> const AV1Frame *ref_src [AV1_NUM_REF_FRAMES];
>>>> - VkVideoDecodeAV1DpbSlotInfoMESA vkav1_refs[AV1_NUM_REF_FRAMES];
>>>> + StdVideoDecodeAV1ReferenceInfo std_ref_info[AV1_NUM_REF_FRAMES];
>>>> + VkVideoDecodeAV1DpbSlotInfoKHR vkav1_refs[AV1_NUM_REF_FRAMES];
>>>>
>>>> uint8_t frame_id_set;
>>>> uint8_t frame_id;
>>>> @@ -60,44 +72,60 @@ typedef struct AV1VulkanDecodePicture {
>>>> static int vk_av1_fill_pict(AVCodecContext *avctx, const AV1Frame **ref_src,
>>>> VkVideoReferenceSlotInfoKHR *ref_slot, /* Main structure */
>>>> VkVideoPictureResourceInfoKHR *ref, /* Goes in ^ */
>>>> - VkVideoDecodeAV1DpbSlotInfoMESA *vkav1_ref, /* Goes in ^ */
>>>> - const AV1Frame *pic, int is_current, int has_grain,
>>>> - int dpb_slot_index)
>>>> + StdVideoDecodeAV1ReferenceInfo *vkav1_std_ref,
>>>> + VkVideoDecodeAV1DpbSlotInfoKHR *vkav1_ref, /* Goes in ^ */
>>>> + const AV1Frame *pic, int is_current, int has_grain)
>>>> {
>>>> FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
>>>> AV1VulkanDecodePicture *hp = pic->hwaccel_picture_private;
>>>> FFVulkanDecodePicture *vkpic = &hp->vp;
>>>> + AV1DecContext *s = avctx->priv_data;
>>>> + CodedBitstreamAV1Context *cbs_ctx;
>>>>
>>>> int err = ff_vk_decode_prepare_frame(dec, pic->f, vkpic, is_current,
>>>> has_grain || dec->dedicated_dpb);
>>>> if (err < 0)
>>>> return err;
>>>>
>>>> - *vkav1_ref = (VkVideoDecodeAV1DpbSlotInfoMESA) {
>>>> - .sType = VK_STRUCTURE_TYPE_VIDEO_DECODE_AV1_DPB_SLOT_INFO_MESA,
>>>> - .frameIdx = hp->frame_id,
>>>> + cbs_ctx = (CodedBitstreamAV1Context *)(s->cbc->priv_data);
>>>> +
>>>> + *vkav1_std_ref = (StdVideoDecodeAV1ReferenceInfo) {
>>>> + .flags = (StdVideoDecodeAV1ReferenceInfoFlags) {
>>>> + .disable_frame_end_update_cdf = pic->raw_frame_header->disable_frame_end_update_cdf,
>>>> + .segmentation_enabled = pic->raw_frame_header->segmentation_enabled,
>>>> + },
>>>> + .frame_type = pic->raw_frame_header->frame_type,
>>>> + .OrderHint = pic->raw_frame_header->order_hint,
>>>> + .RefFrameSignBias = 0x0,
>>>> };
>>>>
>>>> - for (unsigned i = 0; i < 7; i++) {
>>>> - const int idx = pic->raw_frame_header->ref_frame_idx[i];
>>>> - vkav1_ref->ref_order_hint[i] = pic->raw_frame_header->ref_order_hint[idx];
>>>> + for (int i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
>>>> + vkav1_std_ref->SavedOrderHints[i] = cbs_ctx->order_hints[i];
>>>>
>>>
>>> This isn't right. The values you're writing here aren't SavedOrderHints, rather they are the order hints relative to the current picture (which will then be written identically on every picture in the DPB).
>>>
>>
>> Fixed in my git repo earlier. I did ping you about this earlier.
>> The version there is correct according to Nvidia.
>>
>>
>>>> + vkav1_std_ref->RefFrameSignBias |= cbs_ctx->ref_frame_sign_bias[i] << i;
>>>>
>>>
>>> Again, this looks like it asking for the value relative to the reference picture rather than copying the current-frame value identically for every reference?
>>>
>>
>> "RefFrameSignBias is a bitmask where bit index i correspondsto RefFrameSignBias[i] as defined in section 6.8.2 of the AV1 Specification <https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#aomedia-av1>;"
>>
>
> With that reading it's not a property of the reference picture, so why would it be in this structure?
>
> My reading of this is that this structure contains the values as in 6.8.2 when the reference frame itself was decoded, since that's what most of the other fields (like frame_type) are.
>
> SavedOrderHints does muddy this a bit, but since it contains all of that information at the moment of the current picture it seems fair that this works in the same way.
>
Pushed after approval on IRC, thanks for the reviews and your CBS patch
Still not perfect, but will be worked on as more and more vendors adopt it.
More information about the ffmpeg-devel
mailing list