* [FFmpeg-devel] [PATCH 2/2] lavc/vp9: fix the race in exporting video enc params
2022-03-15 10:49 [FFmpeg-devel] [PATCH 1/2] lavc/threadframe: allow decoders to attach buffers to ThreadFrame Anton Khirnov
@ 2022-03-15 10:49 ` Anton Khirnov
2022-03-15 13:59 ` [FFmpeg-devel] [PATCH 1/2] lavc/threadframe: allow decoders to attach buffers to ThreadFrame Andreas Rheinhardt
1 sibling, 0 replies; 3+ messages in thread
From: Anton Khirnov @ 2022-03-15 10:49 UTC (permalink / raw)
To: ffmpeg-devel
The parameters are currently attached directly to the decoded frame
after it is decoded. This is racy, since the frame is shared with other
threads, as reported e.g. by the fate-vp9-encparams test in TSAN build.
Instead, write the params to a refcounted AVBuffer. Each output frame
then gets its own reference to this buffer, which should be thread-safe.
---
libavcodec/vp9.c | 93 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 83 insertions(+), 10 deletions(-)
diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 7ef10f7a80..f9df898733 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -41,6 +41,10 @@
#define VP9_SYNCCODE 0x498342
+typedef struct VP9TFPriv {
+ AVBufferRef *enc_par;
+} VP9TFPriv;
+
#if HAVE_THREADS
DEFINE_OFFSET_ARRAY(VP9Context, vp9_context, pthread_init_cnt,
(offsetof(VP9Context, progress_mutex)),
@@ -100,6 +104,13 @@ static void vp9_frame_unref(AVCodecContext *avctx, VP9Frame *f)
f->hwaccel_picture_private = NULL;
}
+static void vp9_tf_priv_free(void *opaque, uint8_t *data)
+{
+ VP9TFPriv *p = (VP9TFPriv*)data;
+ av_buffer_unref(&p->enc_par);
+ av_freep(&data);
+}
+
static int vp9_frame_alloc(AVCodecContext *avctx, VP9Frame *f)
{
VP9Context *s = avctx->priv_data;
@@ -139,6 +150,19 @@ static int vp9_frame_alloc(AVCodecContext *avctx, VP9Frame *f)
}
}
+ if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
+ VP9TFPriv *p = av_mallocz(sizeof(*p));
+ if (!p)
+ goto fail;
+
+ f->tf.priv_buf = av_buffer_create((uint8_t*)p, sizeof(*p), vp9_tf_priv_free,
+ NULL, AV_BUFFER_FLAG_READONLY);
+ if (!f->tf.priv_buf) {
+ av_freep(&p);
+ goto fail;
+ }
+ }
+
return 0;
fail:
@@ -1497,7 +1521,9 @@ int loopfilter_proc(AVCodecContext *avctx)
static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
{
+ VP9TFPriv *p = (VP9TFPriv*)frame->tf.priv_buf->data;
AVVideoEncParams *par;
+ size_t par_size;
unsigned int tile, nb_blocks = 0;
if (s->s.h.segmentation.enabled) {
@@ -1505,11 +1531,16 @@ static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
nb_blocks += s->td[tile].nb_block_structure;
}
- par = av_video_enc_params_create_side_data(frame->tf.f,
- AV_VIDEO_ENC_PARAMS_VP9, nb_blocks);
+ par = av_video_enc_params_alloc(AV_VIDEO_ENC_PARAMS_VP9, nb_blocks, &par_size);
if (!par)
return AVERROR(ENOMEM);
+ p->enc_par = av_buffer_create((uint8_t *)par, par_size, NULL, NULL, 0);
+ if (!p->enc_par) {
+ av_freep(&par);
+ return AVERROR(ENOMEM);
+ }
+
par->qp = s->s.h.yac_qi;
par->delta_qp[0][0] = s->s.h.ydc_qdelta;
par->delta_qp[1][0] = s->s.h.uvdc_qdelta;
@@ -1547,6 +1578,38 @@ static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
return 0;
}
+static int vp9_output_frame(AVFrame *dst, const ThreadFrame *src)
+{
+ int ret = 0;
+
+ ret = av_frame_ref(dst, src->f);
+ if (ret < 0)
+ return ret;
+
+ if (src->priv_buf) {
+ VP9TFPriv *p = (VP9TFPriv*)src->priv_buf->data;
+
+ if (p->enc_par) {
+ AVBufferRef *buf = av_buffer_ref(p->enc_par);
+ if (!buf) {
+ ret = AVERROR(ENOMEM);
+ goto finish;
+ }
+
+ if (!av_frame_new_side_data_from_buf(dst, AV_FRAME_DATA_VIDEO_ENC_PARAMS, buf)) {
+ av_buffer_unref(&buf);
+ ret = AVERROR(ENOMEM);
+ goto finish;
+ }
+ }
+ }
+
+finish:
+ if (ret < 0)
+ av_frame_unref(dst);
+ return ret;
+}
+
static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
int *got_frame, AVPacket *pkt)
{
@@ -1561,11 +1624,14 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
if ((ret = decode_frame_header(avctx, data, size, &ref)) < 0) {
return ret;
} else if (ret == 0) {
- if (!s->s.refs[ref].f->buf[0]) {
+ ThreadFrame *tf = &s->s.refs[ref];
+ if (!tf->f->buf[0]) {
av_log(avctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref);
return AVERROR_INVALIDDATA;
}
- if ((ret = av_frame_ref(frame, s->s.refs[ref].f)) < 0)
+ ff_thread_await_progress(tf, INT_MAX, 0);
+ ret = vp9_output_frame(frame, tf);
+ if (ret < 0)
return ret;
((AVFrame *)frame)->pts = pkt->pts;
((AVFrame *)frame)->pkt_dts = pkt->dts;
@@ -1743,6 +1809,17 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
ff_thread_finish_setup(avctx);
}
} while (s->pass++ == 1);
+
+ /* this must be done before report_progress(INT_MAX), since the exported
+ * data may be read in other threads */
+ if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
+ ret = vp9_export_enc_params(s, &s->s.frames[CUR_FRAME]);
+ if (ret < 0) {
+ ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
+ return ret;
+ }
+ }
+
ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
if (s->td->error_info < 0) {
@@ -1750,11 +1827,6 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
s->td->error_info = 0;
return AVERROR_INVALIDDATA;
}
- if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
- ret = vp9_export_enc_params(s, &s->s.frames[CUR_FRAME]);
- if (ret < 0)
- return ret;
- }
finish:
// ref frame setup
@@ -1767,7 +1839,8 @@ finish:
}
if (!s->s.h.invisible) {
- if ((ret = av_frame_ref(frame, s->s.frames[CUR_FRAME].tf.f)) < 0)
+ ret = vp9_output_frame(frame, &s->s.frames[CUR_FRAME].tf);
+ if (ret < 0)
return ret;
*got_frame = 1;
}
--
2.34.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/2] lavc/threadframe: allow decoders to attach buffers to ThreadFrame
2022-03-15 10:49 [FFmpeg-devel] [PATCH 1/2] lavc/threadframe: allow decoders to attach buffers to ThreadFrame Anton Khirnov
2022-03-15 10:49 ` [FFmpeg-devel] [PATCH 2/2] lavc/vp9: fix the race in exporting video enc params Anton Khirnov
@ 2022-03-15 13:59 ` Andreas Rheinhardt
1 sibling, 0 replies; 3+ messages in thread
From: Andreas Rheinhardt @ 2022-03-15 13:59 UTC (permalink / raw)
To: ffmpeg-devel
Anton Khirnov:
> This may be useful for synchronizing side data that only becomes
> available after ff_thread_finish_setup() is called.
> ---
> libavcodec/pthread_frame.c | 1 +
> libavcodec/threadframe.h | 3 +++
> libavcodec/utils.c | 7 +++++++
> 3 files changed, 11 insertions(+)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 33b5a2e628..4da3832942 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -1138,6 +1138,7 @@ fail:
> void ff_thread_release_ext_buffer(AVCodecContext *avctx, ThreadFrame *f)
> {
> av_buffer_unref(&f->progress);
> + av_buffer_unref(&f->priv_buf);
> f->owner[0] = f->owner[1] = NULL;
> ff_thread_release_buffer(avctx, f->f);
> }
> diff --git a/libavcodec/threadframe.h b/libavcodec/threadframe.h
> index dea4dadc6d..c2ddc2969f 100644
> --- a/libavcodec/threadframe.h
> +++ b/libavcodec/threadframe.h
> @@ -30,6 +30,9 @@ typedef struct ThreadFrame {
> // progress->data is an array of 2 ints holding progress for top/bottom
> // fields
> AVBufferRef *progress;
> +
> + /* arbitrary user data propagated along with the frame */
> + AVBufferRef *priv_buf;
> } ThreadFrame;
>
> /**
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 066da76e16..cc2c2715b3 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -881,6 +881,12 @@ int ff_thread_ref_frame(ThreadFrame *dst, const ThreadFrame *src)
> return AVERROR(ENOMEM);
> }
>
> + if (src->priv_buf) {
> + dst->priv_buf = av_buffer_ref(src->priv_buf);
> + if (!dst->priv_buf)
> + return AVERROR(ENOMEM);
> + }
> +
> return 0;
> }
>
> @@ -913,6 +919,7 @@ void ff_thread_release_ext_buffer(AVCodecContext *avctx, ThreadFrame *f)
> f->owner[0] = f->owner[1] = NULL;
> if (f->f)
> av_frame_unref(f->f);
> + av_buffer_unref(&f->priv_buf);
> }
>
> void ff_thread_finish_setup(AVCodecContext *avctx)
This approach has the downside that you have to add the priv_buf before
ff_thread_finish_setup(). So in case it is not apparent initially
whether one needs this one is forced to add it (even if it turns out not
to be needed); it will also necessitate two av_buffer_ref() in. A better
approach would be to replace the progress array (of two atomic ints)
with a struct containing these two atomic ints and whatever data needs
to be shared. The owner should logically also be part of this struct,
yet I could not figure out if this is compatible with current h264dec
last time I looked at this (when I wrote the patchset containing
02220b88fc38ef9dd4f2d519f5d3e4151258b60c); the current way of doing
things allows different threads to have different opinions about the
ownership of the frames.
(My actual aim with this patchset was to move the AVFrame into the
aforementioned structure like so:
struct {
atomic_int progress[2];
AVFrame *f;
};
This would avoid the need for the av_frame_ref() in
ff_thread_ref_frame(); therefore all the frame's properties could be set
directly on the AVFrame by its owner as long as the frame is not
finished. The non-owners could read the data subject to the current
limitations (i.e. they have to wait for progress) and could read
anything after the frame is finished (progress == INT_MAX; codecs could
of course use their own semantics here if they wished).
There were two reasons why I didn't finish this approach: 1. How to
synchronize in case of two owners? (Happens only for h264dec.) 2. This
would add an av_frame_alloc() for every frame, even when not using frame
threading. The latter can be easily avoided, but avoiding this with
frame-threading would require a smarter pool-implementation. And I hate
to use/extend the AVBuffer-API for something for which it is simply the
wrong tool and use something that does not require an allocation for
every ref. (It would nevertheless have been advantageous
allocation-wise, because one saves the allocations implicit in
av_frame_ref().))
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 3+ messages in thread