The branch, master has been updated
via 0ea961c0700441b68eff73296755a3245145588e (commit)
via 2ca072e168fd4b4a4edd3dfa2eddf344d86c57e4 (commit)
via d52bca36ef28a928fc42f81214d516f35bd0aa33 (commit)
via 90551b7d80e39c2fcde67fc65e3623bbef12590c (commit)
from 61b034a47ca2490239b08158a43989627c3c3874 (commit)
- Log -----------------------------------------------------------------
commit 0ea961c0700441b68eff73296755a3245145588e
Author: Andreas Rheinhardt <[email protected]>
AuthorDate: Wed Nov 26 01:18:10 2025 +0100
Commit: Andreas Rheinhardt <[email protected]>
CommitDate: Thu Nov 27 11:34:25 2025 +0100
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).
Reviewed-by: Peter Ross <[email protected]>
Signed-off-by: Andreas Rheinhardt <[email protected]>
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;
}
commit 2ca072e168fd4b4a4edd3dfa2eddf344d86c57e4
Author: Andreas Rheinhardt <[email protected]>
AuthorDate: Tue Nov 25 20:31:26 2025 +0100
Commit: Andreas Rheinhardt <[email protected]>
CommitDate: Thu Nov 27 11:33:00 2025 +0100
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.)
Reviewed-by: Peter Ross <[email protected]>
Signed-off-by: Andreas Rheinhardt <[email protected]>
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;
commit d52bca36ef28a928fc42f81214d516f35bd0aa33
Author: Andreas Rheinhardt <[email protected]>
AuthorDate: Tue Nov 25 20:19:42 2025 +0100
Commit: Andreas Rheinhardt <[email protected]>
CommitDate: Thu Nov 27 11:32:54 2025 +0100
avcodec/vp3: Move last_qps from context to stack
Reviewed-by: Peter Ross <[email protected]>
Signed-off-by: Andreas Rheinhardt <[email protected]>
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)
commit 90551b7d80e39c2fcde67fc65e3623bbef12590c
Author: Andreas Rheinhardt <[email protected]>
AuthorDate: Tue Nov 25 21:02:11 2025 +0100
Commit: Andreas Rheinhardt <[email protected]>
CommitDate: Thu Nov 27 11:30:55 2025 +0100
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.
Reviewed-by: Peter Ross <[email protected]>
Signed-off-by: Andreas Rheinhardt <[email protected]>
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 ||
-----------------------------------------------------------------------
Summary of changes:
libavcodec/vp3.c | 54 ++++++++++++++----------------------------------------
1 file changed, 14 insertions(+), 40 deletions(-)
hooks/post-receive
--
_______________________________________________
ffmpeg-cvslog mailing list -- [email protected]
To unsubscribe send an email to [email protected]
