diff mbox series

[FFmpeg-devel] vp9: fix a race in exporting encoding parameters

Message ID 20210224093307.28627-1-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel] vp9: fix a race in exporting encoding parameters
Related show

Checks

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

Commit Message

Anton Khirnov Feb. 24, 2021, 9:33 a.m. UTC
Modifying shared state after ff_thread_finish_setup() is not allowed, so
set the encoding parameters directly on the output frame.

Found-by: James Almer <jamrial@gmail.com>
---
 libavcodec/vp9.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

James Almer Feb. 24, 2021, 1:41 p.m. UTC | #1
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 mbox series

Patch

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;
     }