From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.ffmpeg.org (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id 3C6004B303 for ; Tue, 23 Sep 2025 14:29:26 +0000 (UTC) Authentication-Results: ffbox; dkim=fail (body hash mismatch (got b'qZQUlMmgYPD43/2SUSEDyKaJqFH8eXGZJPnefFmpC7I=', expected b'+BQUDXdsCoB+6cd10X4kxmNrIuC38bnqN+D1b3WamIA=')) header.d=ffmpeg.org header.i=@ffmpeg.org header.a=rsa-sha256 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ffmpeg.org; i=@ffmpeg.org; q=dns/txt; s=mail; t=1758637759; h=mime-version : to : date : message-id : reply-to : subject : list-id : list-archive : list-archive : list-help : list-owner : list-post : list-subscribe : list-unsubscribe : from : cc : content-type : content-transfer-encoding : from; bh=qZQUlMmgYPD43/2SUSEDyKaJqFH8eXGZJPnefFmpC7I=; b=p7s5K7qO1uFGYyRFnRknzNsXGgF2TFXRiVqLqn3Vxjc1nsp9oiILoY6fG3Xr08e4w4JUO bkEulY6ld32USPNJjDXCnDvYkLktKDnc0CpoHHq4RQi/kTDUMEPc3Qsss+BqZwTvML4Y1QT /hFGx9EjbSZE9DNUt1+92zMgxse75Wxdn//v/16TA85PjkPgO34UvOdcht06tZf6ojbrhBb cEzZFD7ucRn9SUG7hUOBkXyttSGsOkb0V9GIG7Fxvgu7zBa3xCZLWbH0aDJRXvBzPEs2FMq ceHHVAvmRZVg5+HHIVzLV47Kkx0LehO5G1eeegPIE0ARIT1cOELnxNJ0tkiw== Received: from [172.19.0.4] (unknown [172.19.0.4]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 21E7E68EBB4; Tue, 23 Sep 2025 17:29:19 +0300 (EEST) ARC-Seal: i=1; cv=none; a=rsa-sha256; d=ffmpeg.org; s=arc; t=1758637757; b=l8fdX6aHeNGK/YHXco8pUkZQgraKG5poe2f8Qd1cYx6pVyqS+OeeN5rhax8PZR7EyhzcT Ep+j18Rcl/K8ykTzt1qwZo7wrc5F3dNEOOZBc3+AF8uiYnvEY4hJEqm+bnnekYGZh15dfJY 7l+c8rktR/BGmh+oEODoQeXZbhBs/8HBrqszFKBt5FoA5tlnVMn7s+XHBaRwgVT2SDbS9BN rX8IO5WNviLerl/icCwn3rHBkEtplZsjdGOuEpWjVGK1DIXLm1Uo6e4Soarb2p/T5cHeXTY Uo3h4HbtzVEtKRmAnKRXdQ6yFrjaCywDzs3c4crLOlY2j0ydifol327XZNIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=ffmpeg.org; s=arc; t=1758637757; h=from : sender : reply-to : subject : date : message-id : to : cc : mime-version : content-type : content-transfer-encoding : content-id : content-description : resent-date : resent-from : resent-sender : resent-to : resent-cc : resent-message-id : in-reply-to : references : list-id : list-help : list-unsubscribe : list-subscribe : list-post : list-owner : list-archive; bh=6WQZp+x7rlerLkt8NxkQbGM2sSJDcecQ+GJmK4+IzoA=; b=Q35RK/M8BUzRLlboaZvfpfdpwzj22u4cknpWyimpp7ERCT1GdfkzuYhas1Wy3r8I4ogOe 2EkmZCwFRdP8fpETQL5vq7gQVEARjHwrpm3HUb2iNFa9mdrwKze/mfKd1iT84w/Z2AqzvI0 Z+5cCR/Qb3tm75gDy3ijS8BxV4eFGCeBYqBTBTCftBsF1WkntqGDFq1RyimVilc4TceOV0P kakLXCiDxP+ygZ4iWjhIBOQK5XVLv5SxxM6B8sRRjsJS7knEsVvSWWlEFzkregKXx9mSvA8 negP1xDXbN92H7xHP1Dy+sBEYOcW5qpJLRPHIgl7Vjee7/tsUPQWKTwaNW6w== ARC-Authentication-Results: i=1; ffmpeg.org; dkim=pass header.d=ffmpeg.org header.i=@ffmpeg.org; arc=none; dmarc=pass header.from=ffmpeg.org policy.dmarc=quarantine Authentication-Results: ffmpeg.org; dkim=pass header.d=ffmpeg.org header.i=@ffmpeg.org; arc=none (Message is not ARC signed); dmarc=pass (Used From Domain Record) header.from=ffmpeg.org policy.dmarc=quarantine DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ffmpeg.org; i=@ffmpeg.org; q=dns/txt; s=mail; t=1758637749; h=content-type : mime-version : content-transfer-encoding : from : to : reply-to : subject : date : from; bh=+BQUDXdsCoB+6cd10X4kxmNrIuC38bnqN+D1b3WamIA=; b=Z73sFRqrzMra8biVaBG1vHIi+GCQSrTSK/Jxrp+J0vcPYSR1jN7ao6qV4mruFcbCMTq/0 jhZrOCIna+uytY0eKnxKx29SBfhWu7iA41EOtbTfK2YmWcm04gAN7b2SXvDiU5AqR/NGoQD eDGbXlJI0sYcb0/HlEY55ycGwqwhGgMHQbzLPnQMGdzT6AWs8UJ1PhUbWjeWFseNaPR9LLC ryShLEZ8Yl2F/p4A9u3Th55ndPFw1GHJxYwnW3K7by14VLg+iK5GxY3fhStP/cwuan0Kp+o 7HsxlvxzBe3FRxwk+vuG36ysrH/xrh0bi3y0goAH4AEb37iEHrDQGP1VMVpA== Received: from ed19c606a818 (code.ffmpeg.org [188.245.149.3]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 7169C68EA8C for ; Tue, 23 Sep 2025 17:29:09 +0300 (EEST) MIME-Version: 1.0 To: ffmpeg-devel@ffmpeg.org Date: Tue, 23 Sep 2025 14:29:09 -0000 Message-ID: <175863774958.25.12766302684032426517@463a07221176> Message-ID-Hash: 7OVIT4BGDIPVK7AQDLVJB4OIMTZVNFTT X-Message-ID-Hash: 7OVIT4BGDIPVK7AQDLVJB4OIMTZVNFTT X-MailFrom: code@ffmpeg.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-ffmpeg-devel.ffmpeg.org-0; header-match-ffmpeg-devel.ffmpeg.org-1; header-match-ffmpeg-devel.ffmpeg.org-2; header-match-ffmpeg-devel.ffmpeg.org-3; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list Reply-To: FFmpeg development discussions and patches Subject: [FFmpeg-devel] [PATCH] ohdec error handling (PR #20587) List-Id: FFmpeg development discussions and patches Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: mkver via ffmpeg-devel Cc: mkver Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Archived-At: List-Archive: List-Post: 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 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 --- 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 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 --- 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 Date: Tue, 23 Sep 2025 16:05:30 +0200 Subject: [PATCH 3/3] avcodec/ohdec: Check mutex/conditions initialization Signed-off-by: Andreas Rheinhardt --- 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