[FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

This is a public field, settable before frames context initialization,
and propagated to any derived contexts.

API users can use it to store state and determine which context
is one of theirs.

This also allows decoders which create their own contexts to store
some state which would be freed only at context destruction.

Request for comments.

Patch attached.


[-- Attachment #2: 0001-hwcontext-add-a-new-AVHWFramesContext.opaque-field.patch --]
[-- Type: text/x-diff, Size: 2157 bytes --]

From a9bdbbb64acfcb0540727895b7be4027ab9955f9 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 06:10:50 +0200
Subject: [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

This is a public field, settable before frames context initialization,
and propagated to any derived contexts.

API users can use it to store state and determine which context
is one of theirs.

This also allows decoders which create their own contexts to store
some state which would be freed only at context destruction.
---
 libavutil/hwcontext.c | 6 ++++++
 libavutil/hwcontext.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index 3396598269..92a8cc907a 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -238,6 +238,7 @@ static void hwframe_ctx_free(void *opaque, uint8_t *data)
     av_buffer_unref(&ctx->internal->source_frames);
 
     av_buffer_unref(&ctx->device_ref);
+    av_buffer_unref(&ctx->opaque);
 
     av_freep(&ctx->hwctx);
     av_freep(&ctx->internal->priv);
@@ -913,6 +914,11 @@ int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx,
     dst->sw_format = src->sw_format;
     dst->width     = src->width;
     dst->height    = src->height;
+    dst->opaque    = av_buffer_ref(src->opaque);
+    if (!dst->internal->source_frames) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
 
     dst->internal->source_frames = av_buffer_ref(source_frame_ctx);
     if (!dst->internal->source_frames) {
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index 7ff08c8608..7655bee33f 100644
--- a/libavutil/hwcontext.h
+++ b/libavutil/hwcontext.h
@@ -227,6 +227,15 @@ typedef struct AVHWFramesContext {
      * Must be set by the user before calling av_hwframe_ctx_init().
      */
     int width, height;
+
+    /**
+     * Opaque data. Can be set before calling av_hwframe_ctx_init().
+     * MUST NOT be set afterwards. Will be unref'd along with the
+     * main context at closure.
+     *
+     * Will be propagated to any derived contexts.
+     */
+    AVBufferRef *opaque;
 } AVHWFramesContext;
 
 /**
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 133 bytes --]

This depends on the previous patch, and allows moving the codec
profile to the new AVHWFramesContext.opaque field.

Patch attached.


[-- Attachment #2: 0002-vulkan_decode-use-the-new-AVHWFramesContext.opaque-f.patch --]
[-- Type: text/x-diff, Size: 10358 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/5] hwcontext_vulkan: call ff_vk_uninit() on device uninit

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 111 bytes --]

This fixes three memory leaks from ff_vk_load_props().
Does not depend on any other patches.

Patch attached.


[-- Attachment #2: 0003-hwcontext_vulkan-call-ff_vk_uninit-on-device-uninit.patch --]
[-- Type: text/x-diff, Size: 888 bytes --]

From 75945869e8341058edd1fca72b9ccf46d0086ddc Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 04:35:29 +0200
Subject: [PATCH 3/5] hwcontext_vulkan: call ff_vk_uninit() on device uninit

This fixes three memory leaks from ff_vk_load_props().
---
 libavutil/hwcontext_vulkan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index 1132a61390..c86229ba65 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1176,6 +1176,8 @@ static void vulkan_device_free(AVHWDeviceContext *ctx)
 
     RELEASE_PROPS(hwctx->enabled_inst_extensions, hwctx->nb_enabled_inst_extensions);
     RELEASE_PROPS(hwctx->enabled_dev_extensions, hwctx->nb_enabled_dev_extensions);
+
+    ff_vk_uninit(&p->vkctx);
 }
 
 static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/5] hwcontext_vulkan: free temporary array once unneeded

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 138 bytes --]

Fixes a small memory leak.
This also prevents leaks on malloc/mutex init errors.

Does not depend on any other patches.

Patch attached.


[-- Attachment #2: 0004-hwcontext_vulkan-free-temporary-array-once-unneeded.patch --]
[-- Type: text/x-diff, Size: 1746 bytes --]

From 52cdd121f986d36d18b95077f9c748bc9d7d918d Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 04:36:54 +0200
Subject: [PATCH 4/5] hwcontext_vulkan: free temporary array once unneeded

Fixes a small memory leak.
This also prevents leaks on malloc/mutex init errors.
---
 libavutil/hwcontext_vulkan.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index c86229ba65..ca802cc86e 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1427,24 +1427,31 @@ static int vulkan_device_init(AVHWDeviceContext *ctx)
     vk->GetPhysicalDeviceQueueFamilyProperties(hwctx->phys_dev, &qf_num, qf);
 
     p->qf_mutex = av_calloc(qf_num, sizeof(*p->qf_mutex));
-    if (!p->qf_mutex)
+    if (!p->qf_mutex) {
+        av_free(qf);
         return AVERROR(ENOMEM);
+    }
     p->nb_tot_qfs = qf_num;
 
     for (uint32_t i = 0; i < qf_num; i++) {
         p->qf_mutex[i] = av_calloc(qf[i].queueCount, sizeof(**p->qf_mutex));
-        if (!p->qf_mutex[i])
+        if (!p->qf_mutex[i]) {
+            av_free(qf);
             return AVERROR(ENOMEM);
+        }
         for (uint32_t j = 0; j < qf[i].queueCount; j++) {
             err = pthread_mutex_init(&p->qf_mutex[i][j], NULL);
             if (err != 0) {
                 av_log(ctx, AV_LOG_ERROR, "pthread_mutex_init failed : %s\n",
                        av_err2str(err));
+                av_free(qf);
                 return AVERROR(err);
             }
         }
     }
 
+    av_free(qf);
+
     graph_index = hwctx->queue_family_index;
     comp_index  = hwctx->queue_family_comp_index;
     tx_index    = hwctx->queue_family_tx_index;
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/5] vulkan_decode: fix small memory leak

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

This small memory leak was essentially responsible for keeping
decoding working at all.

The issue is that the codec profile must be attached to the frames
context. We created the codec profile at decoder creation time.
Which meant that if the decoder was destroyed, the context
became essentially useless, and segfaulted as the profile
was a hanging pointer.
This was an issue that happened when seeking through H264.

This requires using the new AVHWFramesContext.opaque field, as
otherwise, the profile attached to the decoder will be freed
before the frames context, rendering the frames context useless.

Patch attached.


[-- Attachment #2: 0005-vulkan_decode-fix-small-memory-leak.patch --]
[-- Type: text/x-diff, Size: 908 bytes --]

From 7d8f1c73308dd29dbf2d46dc09d85808aa1cc6be Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 06:13:10 +0200
Subject: [PATCH 5/5] vulkan_decode: fix small memory leak

This requires using the new AVHWFramesContext.opaque field, as
otherwise, the profile attached to the decoder will be freed
before the frames context, rendering the frames context useless.
---
 libavcodec/vulkan_decode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/vulkan_decode.c b/libavcodec/vulkan_decode.c
index 81085273d8..5aad8b213b 100644
--- a/libavcodec/vulkan_decode.c
+++ b/libavcodec/vulkan_decode.c
@@ -584,6 +584,8 @@ static void free_common(void *opaque, uint8_t *data)
                                           s->hwctx->alloc);
 
     ff_vk_uninit(s);
+
+    av_free(ctx);
 }
 
 static int vulkan_decode_bootstrap(AVCodecContext *avctx, AVBufferRef *frames_ref)
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Andreas Rheinhardt]

Lynne:
> +    dst->opaque    = av_buffer_ref(src->opaque);
> +    if (!dst->internal->source_frames) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail;
> +    }

Don't you want to check for !dst->opaque?

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

Jun 13, 2023, 06:43 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> +    dst->opaque    = av_buffer_ref(src->opaque);
>> +    if (!dst->internal->source_frames) {
>> +        ret = AVERROR(ENOMEM);
>> +        goto fail;
>> +    }
>>
>
> Don't you want to check for !dst->opaque?
>
> - Andreas
>

Yes, copy and paste error.
Fixed.


[-- Attachment #2: 0001-hwcontext-add-a-new-AVHWFramesContext.opaque-field.patch --]
[-- Type: text/x-diff, Size: 2140 bytes --]

From f1d59fc0e6377c0c3ce2a4182a8c187b46382d4e Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 06:10:50 +0200
Subject: [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

This is a public field, settable before frames context initialization,
and propagated to any derived contexts.

API users can use it to store state and determine which context
is one of theirs.

This also allows decoders which create their own contexts to store
some state which would be freed only at context destruction.
---
 libavutil/hwcontext.c | 6 ++++++
 libavutil/hwcontext.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index 3396598269..dc4be45d4b 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -238,6 +238,7 @@ static void hwframe_ctx_free(void *opaque, uint8_t *data)
     av_buffer_unref(&ctx->internal->source_frames);
 
     av_buffer_unref(&ctx->device_ref);
+    av_buffer_unref(&ctx->opaque);
 
     av_freep(&ctx->hwctx);
     av_freep(&ctx->internal->priv);
@@ -913,6 +914,11 @@ int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx,
     dst->sw_format = src->sw_format;
     dst->width     = src->width;
     dst->height    = src->height;
+    dst->opaque    = av_buffer_ref(src->opaque);
+    if (!dst->opaque) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
 
     dst->internal->source_frames = av_buffer_ref(source_frame_ctx);
     if (!dst->internal->source_frames) {
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index 7ff08c8608..7655bee33f 100644
--- a/libavutil/hwcontext.h
+++ b/libavutil/hwcontext.h
@@ -227,6 +227,15 @@ typedef struct AVHWFramesContext {
      * Must be set by the user before calling av_hwframe_ctx_init().
      */
     int width, height;
+
+    /**
+     * Opaque data. Can be set before calling av_hwframe_ctx_init().
+     * MUST NOT be set afterwards. Will be unref'd along with the
+     * main context at closure.
+     *
+     * Will be propagated to any derived contexts.
+     */
+    AVBufferRef *opaque;
 } AVHWFramesContext;
 
 /**
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Lynne]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Andreas Rheinhardt]

Lynne:
> +    dst->opaque    = av_buffer_ref(src->opaque);
> +    if (!dst->opaque) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail;
> +    }

According to the doxy, opaque can be left unset (and will be given that
it is a new field), yet av_buffer_ref(NULL) will crash. We have
av_buffer_replace() for something like that.

How has this even been tested?

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

Jun 13, 2023, 18:56 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> +    dst->opaque    = av_buffer_ref(src->opaque);
>> +    if (!dst->opaque) {
>> +        ret = AVERROR(ENOMEM);
>> +        goto fail;
>> +    }
>>
>
> According to the doxy, opaque can be left unset (and will be given that
> it is a new field), yet av_buffer_ref(NULL) will crash. We have
> av_buffer_replace() for something like that.
>

Fixed.


[-- Attachment #2: 0001-hwcontext-add-a-new-AVHWFramesContext.opaque-field.patch --]
[-- Type: text/x-diff, Size: 2089 bytes --]

From 02cd540d4cd10ad009e949f097bd3f0250a31fde Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Tue, 13 Jun 2023 06:10:50 +0200
Subject: [PATCH 1/6] hwcontext: add a new AVHWFramesContext.opaque field

This is a public field, settable before frames context initialization,
and propagated to any derived contexts.

API users can use it to store state and determine which context
is one of theirs.

This also allows decoders which create their own contexts to store
some state which would be freed only at context destruction.
---
 libavutil/hwcontext.c | 3 +++
 libavutil/hwcontext.h | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index 3396598269..bbb824abfc 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -238,6 +238,7 @@ static void hwframe_ctx_free(void *opaque, uint8_t *data)
     av_buffer_unref(&ctx->internal->source_frames);
 
     av_buffer_unref(&ctx->device_ref);
+    av_buffer_unref(&ctx->opaque);
 
     av_freep(&ctx->hwctx);
     av_freep(&ctx->internal->priv);
@@ -913,6 +914,8 @@ int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx,
     dst->sw_format = src->sw_format;
     dst->width     = src->width;
     dst->height    = src->height;
+    if ((ret = av_buffer_replace(&dst->opaque, src->opaque)) < 0)
+        goto fail;
 
     dst->internal->source_frames = av_buffer_ref(source_frame_ctx);
     if (!dst->internal->source_frames) {
diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
index 7ff08c8608..7655bee33f 100644
--- a/libavutil/hwcontext.h
+++ b/libavutil/hwcontext.h
@@ -227,6 +227,15 @@ typedef struct AVHWFramesContext {
      * Must be set by the user before calling av_hwframe_ctx_init().
      */
     int width, height;
+
+    /**
+     * Opaque data. Can be set before calling av_hwframe_ctx_init().
+     * MUST NOT be set afterwards. Will be unref'd along with the
+     * main context at closure.
+     *
+     * Will be propagated to any derived contexts.
+     */
+    AVBufferRef *opaque;
 } AVHWFramesContext;
 
 /**
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/5] hwcontext_vulkan: call ff_vk_uninit() on device uninit

[Lynne]

Jun 13, 2023, 06:20 by dev@lynne.ee:

> This fixes three memory leaks from ff_vk_load_props().
> Does not depend on any other patches.
>
> Patch attached.
>

Pushed.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] hwcontext_vulkan: free temporary array once unneeded

[Lynne]

Jun 13, 2023, 06:21 by dev@lynne.ee:

> Fixes a small memory leak.
> This also prevents leaks on malloc/mutex init errors.
>
> Does not depend on any other patches.
>
> Patch attached.
>

Pushed.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Lynne]

Jun 13, 2023, 19:18 by dev@lynne.ee:

> Jun 13, 2023, 18:56 by andreas.rheinhardt@outlook.com:
>
>> Lynne:
>>
>>> +    dst->opaque    = av_buffer_ref(src->opaque);
>>> +    if (!dst->opaque) {
>>> +        ret = AVERROR(ENOMEM);
>>> +        goto fail;
>>> +    }
>>>
>>
>> According to the doxy, opaque can be left unset (and will be given that
>> it is a new field), yet av_buffer_ref(NULL) will crash. We have
>> av_buffer_replace() for something like that.
>>
>
> Fixed.
>

Ping.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] [PATCH 1/5] hwcontext: add a new AVHWFramesContext.opaque field

[Lynne]

Jun 16, 2023, 13:02 by dev@lynne.ee:

> Jun 13, 2023, 19:18 by dev@lynne.ee:
>
>> Jun 13, 2023, 18:56 by andreas.rheinhardt@outlook.com:
>>
>>> Lynne:
>>>
>>>> +    dst->opaque    = av_buffer_ref(src->opaque);
>>>> +    if (!dst->opaque) {
>>>> +        ret = AVERROR(ENOMEM);
>>>> +        goto fail;
>>>> +    }
>>>>
>>>
>>> According to the doxy, opaque can be left unset (and will be given that
>>> it is a new field), yet av_buffer_ref(NULL) will crash. We have
>>> av_buffer_replace() for something like that.
>>>
>>
>> Fixed.
>>
>
> Ping.
>

Consider this out of RFC state.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Anton Khirnov]

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?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Lynne]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".