Mar 17, 2024, 16:36 by s...@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 <airl...@redhat.com>
>>
>> From e2d31951f46fcc10e1263b8603e486e111e9578c Mon Sep 17 00:00:00 2001
>> From: Lynne <d...@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 <airl...@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.
>> if disabled vulkan; then
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 708434ac76..c552f034b7 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -1258,8 +1258,7 @@ SKIPHEADERS += %_tablegen.h
>> \
>> aacenc_quantization.h \
>> aacenc_quantization_misc.h \
>> bitstream_template.h \
>> - vulkan_video_codec_av1std_mesa.h \
>> - $(ARCH)/vpx_arith.h \
>> + $(ARCH)/vpx_arith.h \
>>
>> SKIPHEADERS-$(CONFIG_AMF) += amfenc.h
>> SKIPHEADERS-$(CONFIG_D3D11VA) += d3d11va.h dxva2_internal.h
>> @@ -1280,7 +1279,7 @@ SKIPHEADERS-$(CONFIG_QSVENC) += qsvenc.h
>> SKIPHEADERS-$(CONFIG_VAAPI) += vaapi_decode.h vaapi_hevc.h
>> vaapi_encode.h
>> SKIPHEADERS-$(CONFIG_VDPAU) += vdpau.h vdpau_internal.h
>> SKIPHEADERS-$(CONFIG_VIDEOTOOLBOX) += videotoolbox.h vt_internal.h
>> -SKIPHEADERS-$(CONFIG_VULKAN) += vulkan.h vulkan_video.h
>> vulkan_decode.h vulkan_video_codec_av1std_decode_mesa.h
>> +SKIPHEADERS-$(CONFIG_VULKAN) += vulkan.h vulkan_video.h
>> vulkan_decode.h
>> SKIPHEADERS-$(CONFIG_V4L2_M2M) += v4l2_buffers.h v4l2_context.h
>> v4l2_m2m.h
>> SKIPHEADERS-$(CONFIG_ZLIB) += zlib_wrapper.h
>>
>> diff --git a/libavcodec/av1.h b/libavcodec/av1.h
>> index 8704bc41c1..18d5fa9e7f 100644
>> --- a/libavcodec/av1.h
>> +++ b/libavcodec/av1.h
>> @@ -121,6 +121,8 @@ enum {
>> AV1_DIV_LUT_NUM = 257,
>>
>> AV1_MAX_LOOP_FILTER = 63,
>> +
>> + AV1_RESTORATION_TILESIZE_MAX = 256,
>>
>
> ? This isn't used anywhere.
>
Removed.
>> };
>>
>>
>> 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.
>> - 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>;"
>
> (Probably need to store that in AV1ReferenceFrameState.)
>
I store them in AV1VulkanDecodePicture as they're not needed anywhere.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
