[FFmpeg-devel] [PATCH 1/5] avutil/hwcontext_vulkan: Remove redundant resetting

[FFmpeg-devel] [PATCH 1/5] avutil/hwcontext_vulkan: Remove redundant resetting

[Andreas Rheinhardt]

vulkan_free_internal() already resets the AVVkFrame.internal
pointer.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
1. I find it odd that the frame's internal is freed without
the frame itself being freed; vulkan_frame_free() expects
the internal to be present and seems to NPD if not.

(Actually, I was about to allocate AVVkFrames together
with their internal when I found out that their lifetime
was different.)

2. I have only tested that these patches compile, nothing more.

 libavutil/hwcontext_vulkan.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index 711a32a0ac..c783080567 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -3065,7 +3065,6 @@ static int vulkan_transfer_data_from_cuda(AVHWFramesContext *hwfc,
 fail:
     CHECK_CU(cu->cuCtxPopCurrent(&dummy));
     vulkan_free_internal(dst_f);
-    dst_f->internal = NULL;
     av_buffer_unref(&dst->buf[0]);
     return err;
 }
@@ -3642,7 +3641,6 @@ static int vulkan_transfer_data_to_cuda(AVHWFramesContext *hwfc, AVFrame *dst,
 fail:
     CHECK_CU(cu->cuCtxPopCurrent(&dummy));
     vulkan_free_internal(dst_f);
-    dst_f->internal = NULL;
     av_buffer_unref(&dst->buf[0]);
     return err;
 }
-- 
2.34.1

_______________________________________________
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] avutil/hwcontext_vulkan: Improve type-safety

[Andreas Rheinhardt]

The AVBuffer API uses uint8_t as base type for buffers
and therefore its free callbacks need to abide by this.
Therefore vulkan_frame_free() used an inappropriate signature
which caused casts whenever this function has been called
manually.

This commit changes this by making vulkan_frame_free()
use the proper type and a vulkan_frame_free_cb() that
is used as free callback for the AVBuffer API.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/hwcontext_vulkan.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index c783080567..d1c2d69b7b 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1778,10 +1778,8 @@ static void vulkan_free_internal(AVVkFrame *f)
     av_freep(&f->internal);
 }
 
-static void vulkan_frame_free(void *opaque, uint8_t *data)
+static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
 {
-    AVVkFrame *f = (AVVkFrame *)data;
-    AVHWFramesContext *hwfc = opaque;
     AVVulkanDeviceContext *hwctx = hwfc->device_ctx->hwctx;
     VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
     FFVulkanFunctions *vk = &p->vkctx.vkfn;
@@ -1807,6 +1805,11 @@ static void vulkan_frame_free(void *opaque, uint8_t *data)
     av_free(f);
 }
 
+static void vulkan_frame_free_cb(void *opaque, uint8_t *data)
+{
+    vulkan_frame_free(opaque, (AVVkFrame*)data);
+}
+
 static int alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
                           void *alloc_pnext, size_t alloc_pnext_stride)
 {
@@ -2087,7 +2090,7 @@ static int create_frame(AVHWFramesContext *hwfc, AVVkFrame **frame,
     return 0;
 
 fail:
-    vulkan_frame_free(hwfc, (uint8_t *)f);
+    vulkan_frame_free(hwfc, f);
     return err;
 }
 
@@ -2209,14 +2212,14 @@ static AVBufferRef *vulkan_pool_alloc(void *opaque, size_t size)
         goto fail;
 
     avbuf = av_buffer_create((uint8_t *)f, sizeof(AVVkFrame),
-                             vulkan_frame_free, hwfc, 0);
+                             vulkan_frame_free_cb, hwfc, 0);
     if (!avbuf)
         goto fail;
 
     return avbuf;
 
 fail:
-    vulkan_frame_free(hwfc, (uint8_t *)f);
+    vulkan_frame_free(hwfc, f);
     return NULL;
 }
 
@@ -2357,7 +2360,7 @@ static int vulkan_frames_init(AVHWFramesContext *hwfc)
     if (err)
         return err;
 
-    vulkan_frame_free(hwfc, (uint8_t *)f);
+    vulkan_frame_free(hwfc, f);
 
     /* If user did not specify a pool, hwfc->pool will be set to the internal one
      * in hwcontext.c just after this gets called */
@@ -2766,7 +2769,7 @@ static int vulkan_map_from_drm(AVHWFramesContext *hwfc, AVFrame *dst,
     return 0;
 
 fail:
-    vulkan_frame_free(hwfc->device_ctx->hwctx, (uint8_t *)f);
+    vulkan_frame_free(hwfc->device_ctx->hwctx, f);
     dst->data[0] = NULL;
     return err;
 }
-- 
2.34.1

_______________________________________________
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] avutil/hwcontext_vulkan: Deduplicate code

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/hwcontext_vulkan.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index d1c2d69b7b..0c846fde38 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -2407,31 +2407,7 @@ static int vulkan_transfer_get_formats(AVHWFramesContext *hwfc,
 #if CONFIG_LIBDRM
 static void vulkan_unmap_from_drm(AVHWFramesContext *hwfc, HWMapDescriptor *hwmap)
 {
-    AVVkFrame *f = hwmap->priv;
-    AVVulkanDeviceContext *hwctx = hwfc->device_ctx->hwctx;
-    VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
-    FFVulkanFunctions *vk = &p->vkctx.vkfn;
-    const int nb_images = ff_vk_count_images(f);
-
-    VkSemaphoreWaitInfo wait_info = {
-        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
-        .flags          = 0x0,
-        .pSemaphores    = f->sem,
-        .pValues        = f->sem_value,
-        .semaphoreCount = nb_images,
-    };
-
-    vk->WaitSemaphores(hwctx->act_dev, &wait_info, UINT64_MAX);
-
-    vulkan_free_internal(f);
-
-    for (int i = 0; i < nb_images; i++) {
-        vk->DestroyImage(hwctx->act_dev,     f->img[i], hwctx->alloc);
-        vk->FreeMemory(hwctx->act_dev,       f->mem[i], hwctx->alloc);
-        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
-    }
-
-    av_free(f);
+    vulkan_frame_free(hwfc, hwmap->priv);
 }
 
 static const struct {
-- 
2.34.1

_______________________________________________
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] avutil/hwcontext_vulkan: Cosmetics

[Andreas Rheinhardt]

The alignment in vulkan_unmap_from_drm() (formerly the clone
of vulkan_frame_free()) is nicer than the in vulkan_frame_free(),
let's preserve it.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/hwcontext_vulkan.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index 0c846fde38..c676f4fc57 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1786,9 +1786,10 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
     int nb_images = ff_vk_count_images(f);
 
     VkSemaphoreWaitInfo sem_wait = {
-        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
-        .pSemaphores = f->sem,
-        .pValues = f->sem_value,
+        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
+        .flags          = 0x0,
+        .pSemaphores    = f->sem,
+        .pValues        = f->sem_value,
         .semaphoreCount = nb_images,
     };
 
-- 
2.34.1

_______________________________________________
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] avcodec/vulkan_decode: Factor creating session params out, fix leak

[Andreas Rheinhardt]

All Vulkan HWAccels share the same boilerplate code for creating
session params and this includes a common bug: In case actually
creating the video session parameters fails, the buffer destined
to hold them leaks; in case of HEVC this is also true if
get_data_set_buf() fails.

This commit factors this code out and fixes the leak.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vulkan_av1.c    | 28 ++++------------------------
 libavcodec/vulkan_decode.c | 31 ++++++++++++++++++++++++++++++-
 libavcodec/vulkan_decode.h |  5 +++--
 libavcodec/vulkan_h264.c   | 28 ++++------------------------
 libavcodec/vulkan_hevc.c   | 27 +++------------------------
 5 files changed, 44 insertions(+), 75 deletions(-)

diff --git a/libavcodec/vulkan_av1.c b/libavcodec/vulkan_av1.c
index adaebb2818..b1373722ef 100644
--- a/libavcodec/vulkan_av1.c
+++ b/libavcodec/vulkan_av1.c
@@ -104,12 +104,9 @@ static int vk_av1_fill_pict(AVCodecContext *avctx, const AV1Frame **ref_src,
 
 static int vk_av1_create_params(AVCodecContext *avctx, AVBufferRef **buf)
 {
-    VkResult ret;
-
     const AV1DecContext *s = avctx->priv_data;
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    FFVulkanFunctions *vk = &ctx->s.vkfn;
 
     const AV1RawSequenceHeader *seq = s->raw_seq;
 
@@ -118,10 +115,7 @@ static int vk_av1_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     VkVideoDecodeAV1SessionParametersCreateInfoMESA av1_params;
     VkVideoSessionParametersCreateInfoKHR session_params_create;
 
-    AVBufferRef *tmp;
-    VkVideoSessionParametersKHR *par = av_malloc(sizeof(*par));
-    if (!par)
-        return AVERROR(ENOMEM);
+    int err;
 
     av1_sequence_header = (StdVideoAV1MESASequenceHeader) {
         .flags = (StdVideoAV1MESASequenceHeaderFlags) {
@@ -189,26 +183,12 @@ static int vk_av1_create_params(AVCodecContext *avctx, AVBufferRef **buf)
         .videoSessionParametersTemplate = NULL,
     };
 
-    /* Create session parameters */
-    ret = vk->CreateVideoSessionParametersKHR(ctx->s.hwctx->act_dev, &session_params_create,
-                                              ctx->s.hwctx->alloc, par);
-    if (ret != VK_SUCCESS) {
-        av_log(avctx, AV_LOG_ERROR, "Unable to create Vulkan video session parameters: %s!\n",
-               ff_vk_ret2str(ret));
-        return AVERROR_EXTERNAL;
-    }
-
-    tmp = av_buffer_create((uint8_t *)par, sizeof(*par), ff_vk_decode_free_params,
-                           ctx, 0);
-    if (!tmp) {
-        ff_vk_decode_free_params(ctx, (uint8_t *)par);
-        return AVERROR(ENOMEM);
-    }
+    err = ff_vk_decode_create_params(buf, avctx, ctx, &session_params_create);
+    if (err < 0)
+        return err;
 
     av_log(avctx, AV_LOG_DEBUG, "Created frame parameters\n");
 
-    *buf = tmp;
-
     return 0;
 }
 
diff --git a/libavcodec/vulkan_decode.c b/libavcodec/vulkan_decode.c
index 3986330c81..534a76edda 100644
--- a/libavcodec/vulkan_decode.c
+++ b/libavcodec/vulkan_decode.c
@@ -1057,7 +1057,7 @@ int ff_vk_frame_params(AVCodecContext *avctx, AVBufferRef *hw_frames_ctx)
     return err;
 }
 
-void ff_vk_decode_free_params(void *opaque, uint8_t *data)
+static void vk_decode_free_params(void *opaque, uint8_t *data)
 {
     FFVulkanDecodeShared *ctx = opaque;
     FFVulkanFunctions *vk = &ctx->s.vkfn;
@@ -1067,6 +1067,35 @@ void ff_vk_decode_free_params(void *opaque, uint8_t *data)
     av_free(par);
 }
 
+int ff_vk_decode_create_params(AVBufferRef **par_ref, void *logctx, FFVulkanDecodeShared *ctx,
+                               const VkVideoSessionParametersCreateInfoKHR *session_params_create)
+{
+    VkVideoSessionParametersKHR *par = av_malloc(sizeof(*par));
+    const FFVulkanFunctions *vk = &ctx->s.vkfn;
+    VkResult ret;
+
+    if (!par)
+        return AVERROR(ENOMEM);
+
+    /* Create session parameters */
+    ret = vk->CreateVideoSessionParametersKHR(ctx->s.hwctx->act_dev, session_params_create,
+                                              ctx->s.hwctx->alloc, par);
+    if (ret != VK_SUCCESS) {
+        av_log(logctx, AV_LOG_ERROR, "Unable to create Vulkan video session parameters: %s!\n",
+               ff_vk_ret2str(ret));
+        av_free(par);
+        return AVERROR_EXTERNAL;
+    }
+    *par_ref = av_buffer_create((uint8_t *)par, sizeof(*par),
+                                vk_decode_free_params, ctx, 0);
+    if (!*par_ref) {
+        vk_decode_free_params(ctx, (uint8_t *)par);
+        return AVERROR(ENOMEM);
+    }
+
+    return 0;
+}
+
 int ff_vk_decode_uninit(AVCodecContext *avctx)
 {
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
diff --git a/libavcodec/vulkan_decode.h b/libavcodec/vulkan_decode.h
index c50527e5f8..abf08a98bf 100644
--- a/libavcodec/vulkan_decode.h
+++ b/libavcodec/vulkan_decode.h
@@ -156,9 +156,10 @@ int ff_vk_get_decode_buffer(FFVulkanDecodeContext *ctx, AVBufferRef **buf,
                             void *create_pNext, size_t size);
 
 /**
- * Free VkVideoSessionParametersKHR.
+ * Create VkVideoSessionParametersKHR wrapped in an AVBufferRef.
  */
-void ff_vk_decode_free_params(void *opaque, uint8_t *data);
+int ff_vk_decode_create_params(AVBufferRef **par_ref, void *logctx, FFVulkanDecodeShared *ctx,
+                               const VkVideoSessionParametersCreateInfoKHR *session_params_create);
 
 /**
  * Flush decoder.
diff --git a/libavcodec/vulkan_h264.c b/libavcodec/vulkan_h264.c
index 9fe0d194f5..32ef32d640 100644
--- a/libavcodec/vulkan_h264.c
+++ b/libavcodec/vulkan_h264.c
@@ -285,10 +285,9 @@ static void set_pps(const PPS *pps, const SPS *sps,
 
 static int vk_h264_create_params(AVCodecContext *avctx, AVBufferRef **buf)
 {
-    VkResult ret;
+    int err;
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    FFVulkanFunctions *vk = &ctx->s.vkfn;
     const H264Context *h = avctx->priv_data;
 
     /* SPS */
@@ -319,11 +318,6 @@ static int vk_h264_create_params(AVCodecContext *avctx, AVBufferRef **buf)
         .videoSessionParametersTemplate = NULL,
     };
 
-    AVBufferRef *tmp;
-    VkVideoSessionParametersKHR *par = av_malloc(sizeof(*par));
-    if (!par)
-        return AVERROR(ENOMEM);
-
     /* SPS list */
     for (int i = 0; i < FF_ARRAY_ELEMS(h->ps.sps_list); i++) {
         if (h->ps.sps_list[i]) {
@@ -347,27 +341,13 @@ static int vk_h264_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     h264_params.maxStdSPSCount = h264_params_info.stdSPSCount;
     h264_params.maxStdPPSCount = h264_params_info.stdPPSCount;
 
-    /* Create session parameters */
-    ret = vk->CreateVideoSessionParametersKHR(ctx->s.hwctx->act_dev, &session_params_create,
-                                              ctx->s.hwctx->alloc, par);
-    if (ret != VK_SUCCESS) {
-        av_log(avctx, AV_LOG_ERROR, "Unable to create Vulkan video session parameters: %s!\n",
-               ff_vk_ret2str(ret));
-        return AVERROR_EXTERNAL;
-    }
-
-    tmp = av_buffer_create((uint8_t *)par, sizeof(*par), ff_vk_decode_free_params,
-                           ctx, 0);
-    if (!tmp) {
-        ff_vk_decode_free_params(ctx, (uint8_t *)par);
-        return AVERROR(ENOMEM);
-    }
+    err = ff_vk_decode_create_params(buf, avctx, ctx, &session_params_create);
+    if (err < 0)
+        return err;
 
     av_log(avctx, AV_LOG_DEBUG, "Created frame parameters: %i SPS %i PPS\n",
            h264_params_info.stdSPSCount, h264_params_info.stdPPSCount);
 
-    *buf = tmp;
-
     return 0;
 }
 
diff --git a/libavcodec/vulkan_hevc.c b/libavcodec/vulkan_hevc.c
index 672694a19d..ef371bda67 100644
--- a/libavcodec/vulkan_hevc.c
+++ b/libavcodec/vulkan_hevc.c
@@ -650,11 +650,9 @@ static void set_vps(const HEVCVPS *vps,
 static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
 {
     int err;
-    VkResult ret;
     const HEVCContext *h = avctx->priv_data;
     FFVulkanDecodeContext *dec = avctx->internal->hwaccel_priv_data;
     FFVulkanDecodeShared *ctx = (FFVulkanDecodeShared *)dec->shared_ref->data;
-    FFVulkanFunctions *vk = &ctx->s.vkfn;
 
     VkVideoDecodeH265SessionParametersAddInfoKHR h265_params_info = {
         .sType = VK_STRUCTURE_TYPE_VIDEO_DECODE_H265_SESSION_PARAMETERS_ADD_INFO_KHR,
@@ -677,11 +675,6 @@ static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     AVBufferRef *data_set;
     HEVCHeaderSet *hdr;
 
-    AVBufferRef *tmp;
-    VkVideoSessionParametersKHR *par = av_malloc(sizeof(*par));
-    if (!par)
-        return AVERROR(ENOMEM);
-
     for (int i = 0; h->ps.vps_list[i]; i++)
         nb_vps++;
 
@@ -725,29 +718,15 @@ static int vk_hevc_create_params(AVCodecContext *avctx, AVBufferRef **buf)
     h265_params.maxStdPPSCount = h265_params_info.stdPPSCount;
     h265_params.maxStdVPSCount = h265_params_info.stdVPSCount;
 
-    /* Create session parameters */
-    ret = vk->CreateVideoSessionParametersKHR(ctx->s.hwctx->act_dev, &session_params_create,
-                                              ctx->s.hwctx->alloc, par);
+    err = ff_vk_decode_create_params(buf, avctx, ctx, &session_params_create);
     av_buffer_unref(&data_set);
-    if (ret != VK_SUCCESS) {
-        av_log(avctx, AV_LOG_ERROR, "Unable to create Vulkan video session parameters: %s!\n",
-               ff_vk_ret2str(ret));
-        return AVERROR_EXTERNAL;
-    }
-
-    tmp = av_buffer_create((uint8_t *)par, sizeof(*par), ff_vk_decode_free_params,
-                           ctx, 0);
-    if (!tmp) {
-        ff_vk_decode_free_params(ctx, (uint8_t *)par);
-        return AVERROR(ENOMEM);
-    }
+    if (err < 0)
+        return err;
 
     av_log(avctx, AV_LOG_DEBUG, "Created frame parameters: %i SPS %i PPS %i VPS\n",
            h265_params_info.stdSPSCount, h265_params_info.stdPPSCount,
            h265_params_info.stdVPSCount);
 
-    *buf = tmp;
-
     return 0;
 }
 
-- 
2.34.1

_______________________________________________
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 5/5] avcodec/vulkan_decode: Factor creating session params out, fix leak

[Lynne]

Sep 15, 2023, 01:45 by andreas.rheinhardt@outlook.com:

> All Vulkan HWAccels share the same boilerplate code for creating
> session params and this includes a common bug: In case actually
> creating the video session parameters fails, the buffer destined
> to hold them leaks; in case of HEVC this is also true if
> get_data_set_buf() fails.
>
> This commit factors this code out and fixes the leak.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/vulkan_av1.c    | 28 ++++------------------------
>  libavcodec/vulkan_decode.c | 31 ++++++++++++++++++++++++++++++-
>  libavcodec/vulkan_decode.h |  5 +++--
>  libavcodec/vulkan_h264.c   | 28 ++++------------------------
>  libavcodec/vulkan_hevc.c   | 27 +++------------------------
>  5 files changed, 44 insertions(+), 75 deletions(-)
>

Patchset tested and reviewed, LGTM.
Thanks.
_______________________________________________
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".