[FFmpeg-devel] [PATCH] ohdec error handling (PR #20587)

[FFmpeg-devel] [PATCH] ohdec error handling (PR #20587)

[mkver via ffmpeg-devel]

PR #20587 opened by mkver
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20587
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20587.patch

While looking at ohdec.c for other stuff, I noticed that the error handling seems suspicious in several error paths:
1. When the dec_ref buffer can't be created, oh_decode_release() would never be called. (Fixed in the first patch.)
2. oh_decode_close() always calls OH_VideoDecoder_Stop() if s->dec is set, but what happens if an error happens in between its creation and OH_VideoDecoder_Start()? Is it really safe to call?
3. What actually happens if the OH_VideoDecoder_Start() call in oh_decode_flush() fails?
4. If oh_decode_output_frame() fails, what happens with output->buffer? It seems to leak.
5. It also does not check mutex/condition variables initialization.

This patchset is entirely untested; I don't even know whether it compiles.


>From 556ce80ff21c0a459d163721e066990583af6e31 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Tue, 23 Sep 2025 15:03:40 +0200
Subject: [PATCH 1/3] avcodec/ohdec: Release decoder on allocation failure

Normally, the OH_AVCodec is wrapped inside an AVBuffer
to be freed in its free callback; yet when creating
the AVBuffer fails, the decoder is never destroyed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ohdec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavcodec/ohdec.c b/libavcodec/ohdec.c
index cf1177afcd..08b03a7e7b 100644
--- a/libavcodec/ohdec.c
+++ b/libavcodec/ohdec.c
@@ -124,8 +124,11 @@ static int oh_decode_create(OHCodecDecContext *s, AVCodecContext *avctx)
 
     s->dec_ref = av_buffer_create((uint8_t *)s->dec, 0, oh_decode_release,
                                   NULL, 0);
-    if (!s->dec_ref)
+    if (!s->dec_ref) {
+        oh_decode_release(NULL, (uint8_t*)s->dec);
+        s->dec = NULL;
         return AVERROR(ENOMEM);
+    }
 
     return 0;
 }
-- 
2.49.1


>From bd3f5c4e7f0afbb81c68f3d79423e0d41c8581d2 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Tue, 23 Sep 2025 15:52:09 +0200
Subject: [PATCH 2/3] avcodec/ohdec: Switch to RefStruct API for internal
 refcounting

It avoids allocations and corresponding error conditions.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ohdec.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/libavcodec/ohdec.c b/libavcodec/ohdec.c
index 08b03a7e7b..86f3c84279 100644
--- a/libavcodec/ohdec.c
+++ b/libavcodec/ohdec.c
@@ -29,6 +29,7 @@
 #include "libavutil/imgutils.h"
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
+#include "libavutil/refstruct.h"
 #include "libavutil/thread.h"
 
 #include "avcodec.h"
@@ -40,11 +41,11 @@
 typedef struct OHCodecDecContext {
     AVClass *avclass;
     OH_AVCodec *dec;
-    /* A reference count to dec. Each hardware frame has a reference count to
+    /* A RefStruct reference backing dec. Each hardware frame has a reference count to
      * dec. dec will be destroyed only after oh_decode_close and all hardware
      * frames have been released.
      */
-    AVBufferRef *dec_ref;
+    OH_AVCodec **dec_ref;
 
     AVMutex input_mutex;
     AVCond input_cond;
@@ -74,13 +75,13 @@ typedef struct OHCodecDecContext {
 typedef struct OHCodecBuffer {
     uint32_t index;
     OH_AVBuffer *buffer;
-    AVBufferRef *dec_ref;
+    OH_AVCodec **dec_ref;  ///< RefStruct reference
 } OHCodecBuffer;
 
-static void oh_decode_release(void *opaque, uint8_t *data)
+static void oh_decode_release(AVRefStructOpaque unused, void *obj)
 {
-    OH_AVCodec *dec = (OH_AVCodec *)data;
-    OH_AVErrCode err = OH_VideoDecoder_Destroy(dec);
+    OH_AVCodec **decp = obj;
+    OH_AVErrCode err = OH_VideoDecoder_Destroy(*decp);
     if (err == AV_ERR_OK)
         av_log(NULL, AV_LOG_DEBUG, "Destroy decoder success\n");
     else
@@ -122,13 +123,13 @@ static int oh_decode_create(OHCodecDecContext *s, AVCodecContext *avctx)
     }
     av_log(avctx, AV_LOG_DEBUG, "Create decoder %s success\n", name);
 
-    s->dec_ref = av_buffer_create((uint8_t *)s->dec, 0, oh_decode_release,
-                                  NULL, 0);
+    s->dec_ref = av_refstruct_alloc_ext(sizeof(*s->dec_ref), 0, NULL, oh_decode_release);
     if (!s->dec_ref) {
-        oh_decode_release(NULL, (uint8_t*)s->dec);
+        oh_decode_release((AVRefStructOpaque){.nc = NULL }, &s->dec);
         s->dec = NULL;
         return AVERROR(ENOMEM);
     }
+    *s->dec_ref = s->dec;
 
     return 0;
 }
@@ -408,7 +409,7 @@ static av_cold int oh_decode_close(AVCodecContext *avctx)
             av_log(avctx, AV_LOG_ERROR, "Stop decoder failed, %d, %s\n",
                    err, av_err2str(ff_oh_err_to_ff_err(err)));
         s->dec = NULL;
-        av_buffer_unref(&s->dec_ref);
+        av_refstruct_unref(&s->dec_ref);
     }
 
     av_packet_unref(&s->pkt);
@@ -431,13 +432,8 @@ static void oh_buffer_release(void *opaque, uint8_t *data)
 
     OHCodecBuffer *buffer = opaque;
 
-    if (!buffer->dec_ref) {
-        av_free(buffer);
-        return;
-    }
-
     if (buffer->buffer) {
-        OH_AVCodec *dec = (OH_AVCodec *)buffer->dec_ref->data;
+        OH_AVCodec *dec = *buffer->dec_ref;
         OH_AVCodecBufferAttr attr;
         OH_AVErrCode err = OH_AVBuffer_GetBufferAttr(buffer->buffer, &attr);
         if (err == AV_ERR_OK && !(attr.flags & AVCODEC_BUFFER_FLAGS_DISCARD))
@@ -446,7 +442,7 @@ static void oh_buffer_release(void *opaque, uint8_t *data)
             OH_VideoDecoder_FreeOutputBuffer(dec, buffer->index);
     }
 
-    av_buffer_unref(&buffer->dec_ref);
+    av_refstruct_unref(&buffer->dec_ref);
     av_free(buffer);
 }
 
@@ -467,11 +463,7 @@ static int oh_decode_wrap_hw_buffer(AVCodecContext *avctx, AVFrame *frame,
     if (!buffer)
         return AVERROR(ENOMEM);
 
-    buffer->dec_ref = av_buffer_ref(s->dec_ref);
-    if (!buffer->dec_ref) {
-        oh_buffer_release(buffer, NULL);
-        return AVERROR(ENOMEM);
-    }
+    buffer->dec_ref = av_refstruct_ref(s->dec_ref);
 
     buffer->index = output->index;
     buffer->buffer = output->buffer;
-- 
2.49.1


>From 753569a014613b69a0ecb257210b6cd02da739ee Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Tue, 23 Sep 2025 16:05:30 +0200
Subject: [PATCH 3/3] avcodec/ohdec: Check mutex/conditions initialization

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ohdec.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/libavcodec/ohdec.c b/libavcodec/ohdec.c
index 86f3c84279..3a1887ade5 100644
--- a/libavcodec/ohdec.c
+++ b/libavcodec/ohdec.c
@@ -37,6 +37,7 @@
 #include "decode.h"
 #include "hwconfig.h"
 #include "ohcodec.h"
+#include "pthread_internal.h"
 
 typedef struct OHCodecDecContext {
     AVClass *avclass;
@@ -70,8 +71,14 @@ typedef struct OHCodecDecContext {
 
     char *name;
     int allow_sw;
+    unsigned mutex_cond_cnt;
 } OHCodecDecContext;
 
+#define OFFSET(x) offsetof(OHCodecDecContext, x)
+DEFINE_OFFSET_ARRAY(OHCodecDecContext, mutex_cond, mutex_cond_cnt,
+                    (OFFSET(input_mutex), OFFSET(output_mutex)),
+                    (OFFSET(input_cond), OFFSET(output_cond)));
+
 typedef struct OHCodecBuffer {
     uint32_t index;
     OH_AVBuffer *buffer;
@@ -367,12 +374,11 @@ static av_cold int oh_decode_init(AVCodecContext *avctx)
     OHCodecDecContext *s = avctx->priv_data;
 
     // Initialize these fields first, so oh_decode_close can destroy them safely
-    ff_mutex_init(&s->input_mutex, NULL);
-    ff_cond_init(&s->input_cond, NULL);
-    ff_mutex_init(&s->output_mutex, NULL);
-    ff_cond_init(&s->output_cond, NULL);
+    int ret = ff_pthread_init(s, mutex_cond_offsets);
+    if (ret < 0)
+        return ret;
 
-    int ret = oh_decode_create(s, avctx);
+    ret = oh_decode_create(s, avctx);
     if (ret < 0)
         return ret;
     ret = oh_decode_set_format(s, avctx);
@@ -414,14 +420,12 @@ static av_cold int oh_decode_close(AVCodecContext *avctx)
 
     av_packet_unref(&s->pkt);
 
-    ff_mutex_destroy(&s->input_mutex);
-    ff_cond_destroy(&s->input_cond);
     av_fifo_freep2(&s->input_queue);
 
-    ff_mutex_destroy(&s->output_mutex);
-    ff_cond_destroy(&s->output_cond);
     av_fifo_freep2(&s->output_queue);
 
+    ff_thread_free(s, mutex_cond_offsets);
+
     return 0;
 }
 
@@ -718,7 +722,6 @@ static const AVCodecHWConfigInternal *const oh_hw_configs[] = {
     NULL
 };
 
-#define OFFSET(x) offsetof(OHCodecDecContext, x)
 #define VD (AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM)
 static const AVOption ohcodec_vdec_options[] = {
     {"codec_name", "Select codec by name",
-- 
2.49.1

_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org