diff mbox series

[FFmpeg-devel,2/5] vulkan_decode: use the new AVHWFramesContext.opaque field

Message ID NXmyk7P--3-9@lynne.ee
State New
Headers show
Series [FFmpeg-devel,RFC,1/5] hwcontext: add a new AVHWFramesContext.opaque field | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

Lynne June 13, 2023, 4:19 a.m. UTC 
This depends on the previous patch, and allows moving the codec
profile to the new AVHWFramesContext.opaque field.

Patch attached.

Comments

Anton Khirnov June 13, 2023, 12:28 p.m. UTC | #1 
Quoting Lynne (2023-06-13 06:19:34)
> This depends on the previous patch, and allows moving the codec
> profile to the new AVHWFramesContext.opaque field.
> 
> Patch attached.
> 
> 
> From f992905250062711fab7522906a573ff8ab5f716 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Tue, 13 Jun 2023 06:10:20 +0200
> Subject: [PATCH 2/5] vulkan_decode: use the new AVHWFramesContext.opaque field
> 
> ---
>  libavcodec/vulkan_decode.c | 56 +++++++++++++++++++++++---------------
>  libavcodec/vulkan_decode.h |  6 ++--
>  2 files changed, 37 insertions(+), 25 deletions(-)

This will not work, because the callers are not required to call
avcodec_get_hw_frames_parameters() and can create the frames context
themselves.

The decoder is then not allowed to make any assumptions about the opaque
field.
Lynne June 13, 2023, 12:53 p.m. UTC | #2 
Jun 13, 2023, 14:29 by anton@khirnov.net:

> Quoting Lynne (2023-06-13 06:19:34)
>
>> This depends on the previous patch, and allows moving the codec
>> profile to the new AVHWFramesContext.opaque field.
>>
>> Patch attached.
>>
>>
>> From f992905250062711fab7522906a573ff8ab5f716 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Tue, 13 Jun 2023 06:10:20 +0200
>> Subject: [PATCH 2/5] vulkan_decode: use the new AVHWFramesContext.opaque field
>>
>> ---
>>  libavcodec/vulkan_decode.c | 56 +++++++++++++++++++++++---------------
>>  libavcodec/vulkan_decode.h |  6 ++--
>>  2 files changed, 37 insertions(+), 25 deletions(-)
>>
>
> This will not work, because the callers are not required to call
> avcodec_get_hw_frames_parameters() and can create the frames context
> themselves.
>

Indeed they are. This commit doesn't break this.


> The decoder is then not allowed to make any assumptions about the opaque
> field.
>

Exactly. This is just for the case that the user calls avcodec_get_hw_frames_parameters.
Then, we need somewhere to put the profile.
The opaque field is only set, it is never read in this particular case.
Since it's libavcodec creating the frames context in this case, and since the user
explicitly asked for frames parameters to be set, I don't think it's a problem
to set a public field, much the same way the width, height and sw_format
values are set.

If the user doesn't call avcodec_get_hw_frames_parameters, the rest
of the code wouldn't notice. The rest of the decoder code gets the profile
via the create_pnext chain (where the pointers to the profile structs must be).
So users can attach their own profile, and store it wherever, including
the opaque field.

This commit also fixes the situation where a users calls
avcodec_get_hw_frames_parameters, gets a frame parameters.
destroys the decode context, and uses the same frames context
which was created for another decoder.
Anton Khirnov June 18, 2023, 11:10 a.m. UTC | #3 
Quoting Lynne (2023-06-13 14:53:35)
> Jun 13, 2023, 14:29 by anton@khirnov.net:
> 
> > Quoting Lynne (2023-06-13 06:19:34)
> >
> >> This depends on the previous patch, and allows moving the codec
> >> profile to the new AVHWFramesContext.opaque field.
> >>
> >> Patch attached.
> >>
> >>
> >> From f992905250062711fab7522906a573ff8ab5f716 Mon Sep 17 00:00:00 2001
> >> From: Lynne <dev@lynne.ee>
> >> Date: Tue, 13 Jun 2023 06:10:20 +0200
> >> Subject: [PATCH 2/5] vulkan_decode: use the new AVHWFramesContext.opaque field
> >>
> >> ---
> >>  libavcodec/vulkan_decode.c | 56 +++++++++++++++++++++++---------------
> >>  libavcodec/vulkan_decode.h |  6 ++--
> >>  2 files changed, 37 insertions(+), 25 deletions(-)
> >>
> >
> > This will not work, because the callers are not required to call
> > avcodec_get_hw_frames_parameters() and can create the frames context
> > themselves.
> >
> 
> Indeed they are. This commit doesn't break this.
> 
> 
> > The decoder is then not allowed to make any assumptions about the opaque
> > field.
> >
> 
> Exactly. This is just for the case that the user calls avcodec_get_hw_frames_parameters.
> Then, we need somewhere to put the profile.
> The opaque field is only set, it is never read in this particular case.
> Since it's libavcodec creating the frames context in this case, and since the user
> explicitly asked for frames parameters to be set, I don't think it's a problem
> to set a public field, much the same way the width, height and sw_format
> values are set.
> 
> If the user doesn't call avcodec_get_hw_frames_parameters, the rest
> of the code wouldn't notice. The rest of the decoder code gets the profile
> via the create_pnext chain (where the pointers to the profile structs must be).
> So users can attach their own profile, and store it wherever, including
> the opaque field.
> 
> This commit also fixes the situation where a users calls
> avcodec_get_hw_frames_parameters, gets a frame parameters.
> destroys the decode context, and uses the same frames context
> which was created for another decoder.

Why can't you use the existing user_opaque field?
Lynne June 18, 2023, 12:20 p.m. UTC | #4 
Jun 18, 2023, 13:10 by anton@khirnov.net:

> Quoting Lynne (2023-06-13 14:53:35)
>
>> Jun 13, 2023, 14:29 by anton@khirnov.net:
>>
>> > Quoting Lynne (2023-06-13 06:19:34)
>> >
>> >> This depends on the previous patch, and allows moving the codec
>> >> profile to the new AVHWFramesContext.opaque field.
>> >>
>> >> Patch attached.
>> >>
>> >>
>> >> From f992905250062711fab7522906a573ff8ab5f716 Mon Sep 17 00:00:00 2001
>> >> From: Lynne <dev@lynne.ee>
>> >> Date: Tue, 13 Jun 2023 06:10:20 +0200
>> >> Subject: [PATCH 2/5] vulkan_decode: use the new AVHWFramesContext.opaque field
>> >>
>> >> ---
>> >>  libavcodec/vulkan_decode.c | 56 +++++++++++++++++++++++---------------
>> >>  libavcodec/vulkan_decode.h |  6 ++--
>> >>  2 files changed, 37 insertions(+), 25 deletions(-)
>> >>
>> >
>> > This will not work, because the callers are not required to call
>> > avcodec_get_hw_frames_parameters() and can create the frames context
>> > themselves.
>> >
>>
>> Indeed they are. This commit doesn't break this.
>>
>>
>> > The decoder is then not allowed to make any assumptions about the opaque
>> > field.
>> >
>>
>> Exactly. This is just for the case that the user calls avcodec_get_hw_frames_parameters.
>> Then, we need somewhere to put the profile.
>> The opaque field is only set, it is never read in this particular case.
>> Since it's libavcodec creating the frames context in this case, and since the user
>> explicitly asked for frames parameters to be set, I don't think it's a problem
>> to set a public field, much the same way the width, height and sw_format
>> values are set.
>>
>> If the user doesn't call avcodec_get_hw_frames_parameters, the rest
>> of the code wouldn't notice. The rest of the decoder code gets the profile
>> via the create_pnext chain (where the pointers to the profile structs must be).
>> So users can attach their own profile, and store it wherever, including
>> the opaque field.
>>
>> This commit also fixes the situation where a users calls
>> avcodec_get_hw_frames_parameters, gets a frame parameters.
>> destroys the decode context, and uses the same frames context
>> which was created for another decoder.
>>
>
> Why can't you use the existing user_opaque field?
>

Didn't notice there was one.
diff mbox series

Patch

From f992905250062711fab7522906a573ff8ab5f716 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 06:10:20 +0200
Subject: [PATCH 2/5] vulkan_decode: use the new AVHWFramesContext.opaque field

---
 libavcodec/vulkan_decode.c | 56 +++++++++++++++++++++++---------------
 libavcodec/vulkan_decode.h |  6 ++--
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/libavcodec/vulkan_decode.c b/libavcodec/vulkan_decode.c
index 35e265a5b1..81085273d8 100644
--- a/libavcodec/vulkan_decode.c
+++ b/libavcodec/vulkan_decode.c
@@ -196,7 +196,6 @@  int ff_vk_decode_add_slice(AVCodecContext *avctx, FFVulkanDecodePicture *vp,
 {
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    FFVulkanDecodeProfileData *prof = &ctx->profile_data;
 
     static const uint8_t startcode_prefix[3] = { 0x0, 0x0, 0x1 };
     const size_t startcode_len = add_startcode ? sizeof(startcode_prefix) : 0;
@@ -206,8 +205,8 @@  int ff_vk_decode_add_slice(AVCodecContext *avctx, FFVulkanDecodePicture *vp,
     FFVkVideoBuffer *vkbuf;
 
     size_t new_size = vp->slices_size + startcode_len + size +
-                      prof->caps.minBitstreamBufferSizeAlignment;
-    new_size = FFALIGN(new_size, prof->caps.minBitstreamBufferSizeAlignment);
+                      ctx->caps.minBitstreamBufferSizeAlignment;
+    new_size = FFALIGN(new_size, ctx->caps.minBitstreamBufferSizeAlignment);
 
     slice_off = av_fast_realloc(vp->slice_off, &vp->slice_off_max,
                                 (nb + 1)*sizeof(slice_off));
@@ -295,7 +294,6 @@  int ff_vk_decode_frame(AVCodecContext *avctx,
 
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    FFVulkanDecodeProfileData *prof = &ctx->profile_data;
     FFVulkanFunctions *vk = &ctx->s.vkfn;
 
     /* Output */
@@ -319,7 +317,7 @@  int ff_vk_decode_frame(AVCodecContext *avctx,
     VkImageMemoryBarrier2 img_bar[37];
     int nb_img_bar = 0;
     size_t data_size = FFALIGN(vp->slices_size,
-                               prof->caps.minBitstreamBufferSizeAlignment);
+                               ctx->caps.minBitstreamBufferSizeAlignment);
 
     FFVkExecContext *exec = ff_vk_exec_get(&ctx->exec_pool);
 
@@ -640,10 +638,10 @@  static VkResult vulkan_setup_profile(AVCodecContext *avctx,
                                      VkVideoDecodeH264CapabilitiesKHR *h264_caps,
                                      VkVideoDecodeH265CapabilitiesKHR *h265_caps,
                                      VkVideoDecodeAV1CapabilitiesMESA *av1_caps,
+                                     VkVideoCapabilitiesKHR *caps,
+                                     VkVideoDecodeCapabilitiesKHR *dec_caps,
                                      int cur_profile)
 {
-    VkVideoCapabilitiesKHR *caps = &prof->caps;
-    VkVideoDecodeCapabilitiesKHR *dec_caps = &prof->dec_caps;
     VkVideoDecodeUsageInfoKHR *usage = &prof->usage;
     VkVideoProfileInfoKHR *profile = &prof->profile;
     VkVideoProfileListInfoKHR *profile_list = &prof->profile_list;
@@ -703,6 +701,7 @@  static VkResult vulkan_setup_profile(AVCodecContext *avctx,
 
 static int vulkan_decode_get_profile(AVCodecContext *avctx, AVBufferRef *frames_ref,
                                      enum AVPixelFormat *pix_fmt, VkFormat *vk_fmt,
+                                     FFVulkanDecodeProfileData *prof,
                                      int *dpb_dedicate)
 {
     VkResult ret;
@@ -719,9 +718,8 @@  static int vulkan_decode_get_profile(AVCodecContext *avctx, AVBufferRef *frames_
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
     FFVulkanFunctions *vk = &ctx->s.vkfn;
 
-    FFVulkanDecodeProfileData *prof = &ctx->profile_data;
-    VkVideoCapabilitiesKHR *caps = &prof->caps;
-    VkVideoDecodeCapabilitiesKHR *dec_caps = &prof->dec_caps;
+    VkVideoCapabilitiesKHR *caps = &ctx->caps;
+    VkVideoDecodeCapabilitiesKHR *dec_caps = &ctx->dec_caps;
 
     VkVideoDecodeH264CapabilitiesKHR h264_caps = {
         .sType = VK_STRUCTURE_TYPE_VIDEO_DECODE_H264_CAPABILITIES_KHR,
@@ -760,6 +758,8 @@  static int vulkan_decode_get_profile(AVCodecContext *avctx, AVBufferRef *frames_
                                &h264_caps,
                                &h265_caps,
                                &av1_caps,
+                               caps,
+                               dec_caps,
                                cur_profile);
     if (ret == VK_ERROR_VIDEO_PROFILE_OPERATION_NOT_SUPPORTED_KHR &&
         avctx->flags & AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH &&
@@ -774,6 +774,8 @@  static int vulkan_decode_get_profile(AVCodecContext *avctx, AVBufferRef *frames_
                                    &h264_caps,
                                    &h265_caps,
                                    &av1_caps,
+                                   caps,
+                                   dec_caps,
                                    cur_profile);
     }
 
@@ -967,8 +969,8 @@  int ff_vk_frame_params(AVCodecContext *avctx, AVBufferRef *hw_frames_ctx)
     AVHWFramesContext *frames_ctx = (AVHWFramesContext*)hw_frames_ctx->data;
     AVVulkanFramesContext *hwfc = frames_ctx->hwctx;
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
-    FFVulkanDecodeShared *ctx;
     FFVulkanDecodeProfileData *prof;
+    AVBufferRef *prof_ref;
 
     frames_ctx->sw_format = AV_PIX_FMT_NONE;
 
@@ -976,15 +978,19 @@  int ff_vk_frame_params(AVCodecContext *avctx, AVBufferRef *hw_frames_ctx)
     if (err < 0)
         return err;
 
-    ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    prof = &ctx->profile_data;
+    prof_ref = av_buffer_allocz(sizeof(*prof));
+    if (!prof_ref)
+        return AVERROR(ENOMEM);
+
+    prof = (FFVulkanDecodeProfileData *)prof_ref->data;
 
     err = vulkan_decode_get_profile(avctx, hw_frames_ctx,
                                     &frames_ctx->sw_format, &vkfmt,
-                                    &dedicated_dpb);
+                                    prof, &dedicated_dpb);
     if (err < 0)
         return err;
 
+    frames_ctx->opaque = prof_ref;
     frames_ctx->width  = avctx->width;
     frames_ctx->height = avctx->height;
     frames_ctx->format = AV_PIX_FMT_VULKAN;
@@ -1027,10 +1033,10 @@  int ff_vk_decode_init(AVCodecContext *avctx)
     VkResult ret;
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx;
-    FFVulkanDecodeProfileData *prof;
     FFVulkanContext *s;
     FFVulkanFunctions *vk;
     FFVkQueueFamilyCtx qf_dec;
+    const VkVideoProfileListInfoKHR *profile_list;
 
     VkVideoDecodeH264SessionParametersCreateInfoKHR h264_params = {
         .sType = VK_STRUCTURE_TYPE_VIDEO_DECODE_H264_SESSION_PARAMETERS_CREATE_INFO_KHR,
@@ -1064,7 +1070,6 @@  int ff_vk_decode_init(AVCodecContext *avctx)
 
     /* Initialize contexts */
     ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    prof = &ctx->profile_data;
     s = &ctx->s;
     vk = &ctx->s.vkfn;
 
@@ -1075,6 +1080,13 @@  int ff_vk_decode_init(AVCodecContext *avctx)
     s->device = (AVHWDeviceContext *)s->frames->device_ref->data;
     s->hwctx = s->device->hwctx;
 
+    profile_list = ff_vk_find_struct(s->hwfc->create_pnext,
+                                     VK_STRUCTURE_TYPE_VIDEO_PROFILE_LIST_INFO_KHR);
+    if (!profile_list) {
+        av_log(avctx, AV_LOG_ERROR, "Profile list missing from frames context!");
+        return AVERROR(EINVAL);
+    }
+
     err = ff_vk_load_props(s);
     if (err < 0)
         goto fail;
@@ -1096,13 +1108,13 @@  int ff_vk_decode_init(AVCodecContext *avctx)
 
     session_create.flags = 0x0;
     session_create.queueFamilyIndex = s->hwctx->queue_family_decode_index;
-    session_create.maxCodedExtent = prof->caps.maxCodedExtent;
-    session_create.maxDpbSlots = prof->caps.maxDpbSlots;
-    session_create.maxActiveReferencePictures = prof->caps.maxActiveReferencePictures;
+    session_create.maxCodedExtent = ctx->caps.maxCodedExtent;
+    session_create.maxDpbSlots = ctx->caps.maxDpbSlots;
+    session_create.maxActiveReferencePictures = ctx->caps.maxActiveReferencePictures;
     session_create.pictureFormat = s->hwfc->format[0];
     session_create.referencePictureFormat = session_create.pictureFormat;
     session_create.pStdHeaderVersion = dec_ext[avctx->codec_id];
-    session_create.pVideoProfile = &prof->profile_list.pProfiles[0];
+    session_create.pVideoProfile = &profile_list->pProfiles[0];
 
     /* Create decode exec context.
      * 2 async contexts per thread was experimentally determined to be optimal
@@ -1147,14 +1159,14 @@  int ff_vk_decode_init(AVCodecContext *avctx)
         dpb_frames->height    = s->frames->height;
 
         dpb_hwfc = dpb_frames->hwctx;
-        dpb_hwfc->create_pnext = (void *)&prof->profile_list;
+        dpb_hwfc->create_pnext = (void *)profile_list;
         dpb_hwfc->format[0]    = s->hwfc->format[0];
         dpb_hwfc->tiling       = VK_IMAGE_TILING_OPTIMAL;
         dpb_hwfc->usage        = VK_IMAGE_USAGE_VIDEO_DECODE_DPB_BIT_KHR |
                                  VK_IMAGE_USAGE_SAMPLED_BIT; /* Shuts validator up. */
 
         if (dec->layered_dpb)
-            dpb_hwfc->nb_layers = prof->caps.maxDpbSlots;
+            dpb_hwfc->nb_layers = ctx->caps.maxDpbSlots;
 
         err = av_hwframe_ctx_init(ctx->dpb_hwfc_ref);
         if (err < 0)
diff --git a/libavcodec/vulkan_decode.h b/libavcodec/vulkan_decode.h
index 681d2476cd..3ac103f6b4 100644
--- a/libavcodec/vulkan_decode.h
+++ b/libavcodec/vulkan_decode.h
@@ -26,8 +26,6 @@ 
 #include "vulkan_video.h"
 
 typedef struct FFVulkanDecodeProfileData {
-    VkVideoCapabilitiesKHR caps;
-    VkVideoDecodeCapabilitiesKHR dec_caps;
     VkVideoDecodeH264ProfileInfoKHR h264_profile;
     VkVideoDecodeH264ProfileInfoKHR h265_profile;
     VkVideoDecodeAV1ProfileInfoMESA av1_profile;
@@ -40,7 +38,9 @@  typedef struct FFVulkanDecodeShared {
     FFVulkanContext s;
     FFVkVideoCommon common;
     FFVkExecPool exec_pool;
-    FFVulkanDecodeProfileData profile_data;
+
+    VkVideoCapabilitiesKHR caps;
+    VkVideoDecodeCapabilitiesKHR dec_caps;
 
     AVBufferRef *dpb_hwfc_ref;  /* Only used for dedicated_dpb */
 
-- 
2.40.1