diff mbox series

[FFmpeg-devel,5/5] avcodec/vulkan_decode: Factor creating session params out, fix leak

Message ID GV1P250MB0737BD44A94683CD74809A6C8FF7A@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit c1b6235d4158b3771e2a2ee7d5754477df7edc59
Headers show
Series [FFmpeg-devel,1/5] avutil/hwcontext_vulkan: Remove redundant resetting | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 14, 2023, 11:45 p.m. UTC 
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(-)

Comments

Lynne Sept. 15, 2023, 12:05 a.m. UTC | #1 
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.
diff mbox series

Patch

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;
 }