| Message ID | 20210224093307.28627-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] vp9: fix a race in exporting encoding parameters | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 2/24/2021 6:33 AM, Anton Khirnov wrote: > Modifying shared state after ff_thread_finish_setup() is not allowed, so > set the encoding parameters directly on the output frame. Does this also ensure the side data will be present in show_existing_frame frames? This means the "ret == 0" case when returning from decode_frame_header(), where it simply outputs an already decoded frame again directly from s->s.refs. See vp90-2-10-show-existing-frame* in samples/vp9-test-vectors/ > > Found-by: James Almer <jamrial@gmail.com> By Valgrind :p > --- > libavcodec/vp9.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c > index 4659f94ee8..46062f8a8f 100644 > --- a/libavcodec/vp9.c > +++ b/libavcodec/vp9.c > @@ -1499,7 +1499,8 @@ int loopfilter_proc(AVCodecContext *avctx) > } > #endif > > -static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame) > +static int vp9_export_enc_params(VP9Context *s, AVFrame *frame_dst, > + const VP9Frame *frame) > { > AVVideoEncParams *par; > unsigned int tile, nb_blocks = 0; > @@ -1509,7 +1510,7 @@ 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, > + par = av_video_enc_params_create_side_data(frame_dst, > AV_VIDEO_ENC_PARAMS_VP9, nb_blocks); > if (!par) > return AVERROR(ENOMEM); > @@ -1759,12 +1760,6 @@ FF_ENABLE_DEPRECATION_WARNINGS > 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 > for (i = 0; i < 8; i++) { > @@ -1778,6 +1773,13 @@ finish: > if (!s->s.h.invisible) { > if ((ret = av_frame_ref(frame, s->s.frames[CUR_FRAME].tf.f)) < 0) > return ret; > + > + if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) { > + ret = vp9_export_enc_params(s, frame, &s->s.frames[CUR_FRAME]); > + if (ret < 0) > + return ret; > + } > + > *got_frame = 1; > } > >
diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c index 4659f94ee8..46062f8a8f 100644 --- a/libavcodec/vp9.c +++ b/libavcodec/vp9.c @@ -1499,7 +1499,8 @@ int loopfilter_proc(AVCodecContext *avctx) } #endif -static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame) +static int vp9_export_enc_params(VP9Context *s, AVFrame *frame_dst, + const VP9Frame *frame) { AVVideoEncParams *par; unsigned int tile, nb_blocks = 0; @@ -1509,7 +1510,7 @@ 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, + par = av_video_enc_params_create_side_data(frame_dst, AV_VIDEO_ENC_PARAMS_VP9, nb_blocks); if (!par) return AVERROR(ENOMEM); @@ -1759,12 +1760,6 @@ FF_ENABLE_DEPRECATION_WARNINGS 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 for (i = 0; i < 8; i++) { @@ -1778,6 +1773,13 @@ finish: if (!s->s.h.invisible) { if ((ret = av_frame_ref(frame, s->s.frames[CUR_FRAME].tf.f)) < 0) return ret; + + if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) { + ret = vp9_export_enc_params(s, frame, &s->s.frames[CUR_FRAME]); + if (ret < 0) + return ret; + } + *got_frame = 1; }