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 918084BAB5 for ; Wed, 26 Nov 2025 02:04:42 +0000 (UTC) Authentication-Results: ffbox; dkim=fail (body hash mismatch (got b'b35VWml6qvQAki/PuS1ZDxzmdMiM0/TpxaNtKETqN9U=', expected b't3xCDuYFn8Lr3vd/r1ieGlU0w5HLcnvKlnv6a103caM=')) 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=1764122635; 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=b35VWml6qvQAki/PuS1ZDxzmdMiM0/TpxaNtKETqN9U=; b=z4ckHUsgm8nLdUsAZHPz125XHsZ2zXZS5fRa5iqZdksO1V1E+Vwd/O6Jvbc/1ACHxqBOp qZ+s5Yco2fCUhs93T6eLYohfwzpMhGIYSgiir0w4gldVNcNu0kRrvdpwemHik7UcDkqVzGK p9yifq5kMAq4Xjsn7WpDyfSLEA567UPNcsKco3r1xjsWVsZpSvis20TqHK7x3doQ/k2WhB3 /trQBF6LM3KTSSw8DWR6ykObLn7tTgJ/42usJMKOKqp7l7zHnzBepeLw16R1cqzNjaP5krX 3wx9xVrWAvRwPFBKs307XrWd6L53LR3+21xxpsE4n9cV+vr8nhQNK2xSsDIA== Received: from [172.19.0.3] (unknown [172.19.0.3]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 8F46269003D; Wed, 26 Nov 2025 04:03:55 +0200 (EET) ARC-Seal: i=1; cv=none; a=rsa-sha256; d=ffmpeg.org; s=arc; t=1764122612; b=qknTILmCE/BIUmJwAgBG6MmPJ22xEnYQ/CtDAS5xMaTMRs+kGoqmanlG83lpkEesX5vbl 2TnVLIC8a9XTqyNFWNabK4i97MgC1c75XxiaTRWwhAPIEyFcgWRNUVDHPCZ3iUhFH9yTAo1 TrvfzMs7DBJdZPy5Wq6l1+O+OUdvIcA3/37ybUXm57R7DtMgkKXEkLZIyr1uxLrVKDs++18 vVYC5PvkxCpVAKftudOoGGX4+4Ss4PZuxrBgrvv7I1zJcVq3TcmsgK8vwA0J7AoU3HS7Sqr t+cA7uVTn1S3Tpq8K1AW4+XBIqfl7aN4l2t7AmA+sVHDyQD7kiTiOGyFoqwQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=ffmpeg.org; s=arc; t=1764122612; 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=QxRH9oc43zjPHXI3B4gKqTxoXqUG+tjYOn1xXwFHkHs=; b=U2tOmal39HPH1QsvbAE7lNEeSCkYQtRnfoU9/5xCGulnwNRo372rHIIkgv90STrXs3DVf avBGVF3TX2qdyf2lPYIcUXyoO7p9NAw/rHBtGAhoTMadqT5PIDXetBApQtMvceCPi6850Al ukgnbOEYAfvUwS/k5PTPy1aze7R+VXDZSRiFGuJ0IX5UDzUEYzUC4Ez2homMOsSDVdFeXmh XI0FFJF9h8q3SOB5QxvtWJQ4JPGdNhWq+B/uiVbbcbs9QidSpqrbiJkjaZWGjxV9IeCuOd6 CJAWCynuXBS3UZEJpDiRiLNYno1PKDe9X39qROLH3lon23ZJHrfJH0iQRzAQ== 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=1764122602; h=content-type : mime-version : content-transfer-encoding : from : to : reply-to : subject : date : from; bh=t3xCDuYFn8Lr3vd/r1ieGlU0w5HLcnvKlnv6a103caM=; b=cwmXuMqXY2DZ7+FbZkdaszSbaWD3WUc+6uxcCsGqe6gkKQyzK1qSlTplL1tsy8A0qvDOr e23CWaNNrQGwSQAR9aH3De3c3Tb3mfWR3hmMuFcSGtancnkrq3e2lKGHJAoNQkDUKgG74YP 9ZlI1esM/tJzQzVOB5PbhWFaGINHGr2FM+u5BqdUn9r5XlyONG4Zzw9q3ee0Jat0TCsA3kH 7qgI0fIWa5iVjjS8C8znBERbLe6bctcxXekmNFzCOMXfltEs1Brc/21vbg+CEMz1ghmyK7g lZu6/uNsvwhhRiqnHuSI+xtO+6/JhT1L1jNUI8wZDIG6n/wGGdLh3av/+iWA== Received: from 55ca25703178 (code.ffmpeg.org [188.245.149.3]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 833B668B0ED for ; Wed, 26 Nov 2025 04:03:22 +0200 (EET) MIME-Version: 1.0 To: ffmpeg-devel@ffmpeg.org Date: Wed, 26 Nov 2025 02:03:21 -0000 Message-ID: <176412260297.39.17343617117605891621@2cb04c0e5124> Message-ID-Hash: XWIXZTYURCVBW4LERW562PFRE3QHXSYC X-Message-ID-Hash: XWIXZTYURCVBW4LERW562PFRE3QHXSYC X-MailFrom: code@ffmpeg.org X-Mailman-Rule-Hits: nonmember-moderation 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 X-Mailman-Version: 3.3.10 Precedence: list Reply-To: FFmpeg development discussions and patches Subject: [FFmpeg-devel] [PATCH] avcodec/vp3: Sync VLCs once during init, fix crash (PR #21023) 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 #21023 opened by mkver URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21023 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21023.patch Fixes issue #20346 and trac ticket #11592. >>From 6d5d680d7b2f0fa2359e418e95f03fe5d26ad4d5 Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Tue, 25 Nov 2025 21:02:11 +0100 Subject: [PATCH 1/4] avcodec/vp3: Sync VLCs once during init, fix crash 6c7a344b65cb7476d1575cb1504e3a53bcbc83e7 made the VLCs shared between threads and did so in a way that was designed to support stream reconfigurations, so that the structure containing the VLCs was synced in update_thread_context. The idea was that the currently active VLCs would just be passed along between threads. Yet this was broken by 5acbdd2264d3b90dc11369f9e031e762f260882e: Before this commit, submit_packet() was a no-op during flushing for VP3, as it is a no-delay decoder, so it won't produce any output during flushing. This meant that prev_thread in pthread_frame.c contained the last dst thread that update_thread_context() was called for (so that these VLCs could be passed along between threads). Yet after said commit, submit_packet was no longer a no-op during flushing and changed prev_thread in such a way that it did not need to contain any VLCs at all*. When flushing, prev_thread is used to pass the current state to the first worker thread which is the one that is used to restart decoding. It could therefore happen that the decoding thread did not contain the VLCs at all any more after decoding restarts after flushing leading to a crash (this scenario was never anticipated and must not happen at all). There is a simple, easily backportable fix given that we do not support stream reconfigurations (yet) when using frame threading: Don't sync the VLCs in update_thread_context(), instead do it once during init. This fixes forgejo issue #20346 and trac issue #11592. (I don't know why 5acbdd2264d3b90dc11369f9e031e762f260882e changed submit_packet() to no longer be a no-op when draining no-delay decoders.) *: The exact condition for the crash is nb_threads > 2*nb_frames. Signed-off-by: Andreas Rheinhardt --- libavcodec/vp3.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 0613254c14..ab60dee098 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -47,7 +47,6 @@ #include "decode.h" #include "get_bits.h" #include "hpeldsp.h" -#include "internal.h" #include "jpegquanttables.h" #include "mathops.h" #include "progressframe.h" @@ -2459,7 +2458,7 @@ static av_cold int vp3_decode_init(AVCodecContext *avctx) } } - if (!avctx->internal->is_copy) { + if (ff_thread_sync_ref(avctx, offsetof(Vp3DecodeContext, coeff_vlc)) != FF_THREAD_IS_COPY) { CoeffVLCs *vlcs = av_refstruct_alloc_ext(sizeof(*s->coeff_vlc), 0, NULL, free_vlc_tables); if (!vlcs) @@ -2528,8 +2527,6 @@ static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext * const Vp3DecodeContext *s1 = src->priv_data; int qps_changed = 0; - av_refstruct_replace(&s->coeff_vlc, s1->coeff_vlc); - // copy previous frame data ref_frames(s, s1); if (!s1->current_frame.f || -- 2.49.1 >>From 93ca899c80ca2112ecd1db383f07ee6609cb0a56 Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Tue, 25 Nov 2025 20:19:42 +0100 Subject: [PATCH 2/4] avcodec/vp3: Move last_qps from context to stack Signed-off-by: Andreas Rheinhardt --- libavcodec/vp3.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index ab60dee098..9a766ecb41 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -217,7 +217,6 @@ typedef struct Vp3DecodeContext { int qps[3]; int nqps; - int last_qps[3]; int superblock_count; int y_superblock_width; @@ -2551,7 +2550,6 @@ static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext * if (qps_changed) { memcpy(s->qps, s1->qps, sizeof(s->qps)); - memcpy(s->last_qps, s1->last_qps, sizeof(s->last_qps)); s->nqps = s1->nqps; } } @@ -2618,8 +2616,10 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, } if (!s->theora) skip_bits(&gb, 1); + + int last_qps[3]; for (int i = 0; i < 3; i++) - s->last_qps[i] = s->qps[i]; + last_qps[i] = s->qps[i]; s->nqps = 0; do { @@ -2636,13 +2636,13 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, avctx->skip_loop_filter >= (s->keyframe ? AVDISCARD_ALL : AVDISCARD_NONKEY); - if (s->qps[0] != s->last_qps[0]) + if (s->qps[0] != last_qps[0]) init_loop_filter(s); for (int i = 0; i < s->nqps; i++) // reinit all dequantizers if the first one changed, because // the DC of the first quantizer must be used for all matrices - if (s->qps[i] != s->last_qps[i] || s->qps[0] != s->last_qps[0]) + if (s->qps[i] != last_qps[i] || s->qps[0] != last_qps[0]) init_dequantizer(s, i); if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe) -- 2.49.1 >>From 3d4c414f69507be3b6569859025f2e2a7a9963c3 Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Tue, 25 Nov 2025 20:31:26 +0100 Subject: [PATCH 3/4] avcodec/vp3: Remove always-false checks The dimensions are only set at two places: theora_decode_header() and vp3_decode_init(). These functions are called during init and during dimension changes, but the latter is only supported (and attempted) when frame threading is not active. This implies that the dimensions of the various worker threads in vp3_update_thread_context() always coincide, so that these checks are dead and can be removed. (These checks would of course need to be removed when support for dimension changes during frame threading is implemented; and in any case, a dimension change is not an error.) Signed-off-by: Andreas Rheinhardt --- libavcodec/vp3.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 9a766ecb41..60908c877f 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -2528,10 +2528,8 @@ static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext * // copy previous frame data ref_frames(s, s1); - if (!s1->current_frame.f || - s->width != s1->width || s->height != s1->height) { + if (!s1->current_frame.f) return -1; - } if (s != s1) { s->keyframe = s1->keyframe; -- 2.49.1 >>From a65973ea02ffcdff4156a2de139cb4e686fe7217 Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Wed, 26 Nov 2025 01:18:10 +0100 Subject: [PATCH 4/4] avcodec/vp3: Redo updating frames VP3's frame managment is actually simple: It has three frame slots: current, last and golden. After having decoded the current frame, the old last frame will be freed and replaced by the current frame. If the current frame is a keyframe, it also takes over the golden slot. The VP3 decoder handled this like this: In single-threaded mode, the above procedure was carried out (on success). Doing so with frame-threading is impossible, as it would lead to data races. Instead vp3_update_thread_context() created new references to these frames and then carried out said procedure. This means that vp3_update_thread_context() is not just a "dumb" function that only copies certain fields from src to dst; instead it actually processes them. E.g. trying to copy the decoding state from A to B and then from B to C (with no decode_frame call in between) will not be equivalent to copying from A to C, as both current and last frames will be blank in the first case. This commit changes this: Because last_frame won't be needed after decoding, no reference to it will be created to it in vp3_update_thread_context(); instead it is now always unreferenced after decoding it (even on error). Replacing last_frame with the new frame is now always performed when the new frame is allocated. Replacing the golden frame is now done earlier, namely in decode_frame() before ff_thread_finish_setup(), so that update_thread_context only has to reference current frame and golden frame. Being dumb means that update_thread_context also no longer checks whether the current frame is valid, so that it can no longer error out. This unifies the single- and multi-threaded codepaths; it can lead to changes in output in single threaded mode: When erroring out, the current frame would be discarded and not be put into one of the reference slots at all in single-threaded mode. The new code meanwhile does everything as the frame-threaded code already did in order to reduce discrepancies between the two. It would be possible to keep the old single-threaded behavior (one would need to postpone replacing the golden frame to the end of vp3_decode_frame and would need to swap the current frame and the last frame on error, unreferencing the former). Signed-off-by: Andreas Rheinhardt --- libavcodec/vp3.c | 37 ++++++++----------------------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 60908c877f..7ce54967e1 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -2499,25 +2499,11 @@ static av_cold int vp3_decode_init(AVCodecContext *avctx) return allocate_tables(avctx); } -/// Release and shuffle frames after decode finishes -static void update_frames(AVCodecContext *avctx) -{ - Vp3DecodeContext *s = avctx->priv_data; - - if (s->keyframe) - ff_progress_frame_replace(&s->golden_frame, &s->current_frame); - - /* shuffle frames */ - ff_progress_frame_unref(&s->last_frame); - FFSWAP(ProgressFrame, s->last_frame, s->current_frame); -} - #if HAVE_THREADS static void ref_frames(Vp3DecodeContext *dst, const Vp3DecodeContext *src) { ff_progress_frame_replace(&dst->current_frame, &src->current_frame); ff_progress_frame_replace(&dst->golden_frame, &src->golden_frame); - ff_progress_frame_replace(&dst->last_frame, &src->last_frame); } static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext *src) @@ -2528,12 +2514,8 @@ static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext * // copy previous frame data ref_frames(s, s1); - if (!s1->current_frame.f) - return -1; if (s != s1) { - s->keyframe = s1->keyframe; - // copy qscale data if necessary for (int i = 0; i < 3; i++) { if (s->qps[i] != s1->qps[1]) { @@ -2551,8 +2533,6 @@ static int vp3_update_thread_context(AVCodecContext *dst, const AVCodecContext * s->nqps = s1->nqps; } } - - update_frames(dst); return 0; } #endif @@ -2646,14 +2626,14 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe) return buf_size; - ff_progress_frame_unref(&s->current_frame); - ret = ff_progress_frame_get_buffer(avctx, &s->current_frame, + ret = ff_progress_frame_get_buffer(avctx, &s->last_frame, AV_GET_BUFFER_FLAG_REF); if (ret < 0) { // Don't goto error here, as one can't report progress on or // unref a non-existent frame. return ret; } + FFSWAP(ProgressFrame, s->last_frame, s->current_frame); s->current_frame.f->pict_type = s->keyframe ? AV_PICTURE_TYPE_I : AV_PICTURE_TYPE_P; if (s->keyframe) @@ -2678,7 +2658,8 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, #if !CONFIG_VP4_DECODER if (version >= 2) { av_log(avctx, AV_LOG_ERROR, "This build does not support decoding VP4.\n"); - return AVERROR_DECODER_NOT_FOUND; + ret = AVERROR_DECODER_NOT_FOUND; + goto error; } #endif s->version = version; @@ -2716,6 +2697,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, } #endif } + ff_progress_frame_replace(&s->golden_frame, &s->current_frame); } else { if (!s->golden_frame.f) { av_log(s->avctx, AV_LOG_WARNING, @@ -2793,6 +2775,8 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, } vp3_draw_horiz_band(s, s->height); + ff_progress_frame_unref(&s->last_frame); + /* output frame, offset as needed */ if ((ret = av_frame_ref(frame, s->current_frame.f)) < 0) return ret; @@ -2804,16 +2788,11 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, *got_frame = 1; - if (!HAVE_THREADS || !(s->avctx->active_thread_type & FF_THREAD_FRAME)) - update_frames(avctx); - return buf_size; error: ff_progress_frame_report(&s->current_frame, INT_MAX); - - if (!HAVE_THREADS || !(s->avctx->active_thread_type & FF_THREAD_FRAME)) - av_frame_unref(s->current_frame.f); + ff_progress_frame_unref(&s->last_frame); return ret; } -- 2.49.1 _______________________________________________ ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org