Mar 20, 2024, 00:33 by s...@jkqxz.net:
> On 18/03/2024 05:33, Lynne wrote:
>
>> 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.
>>
>
> 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.
_______________________________________________
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".
