diff mbox series

[FFmpeg-devel,v2,06/27] avcodec/vp9: Fix race when attaching side-data for show-existing frame

Message ID GV1P250MB073711693E037A6ED4C41C208F002@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit 0ba058579f332b3060d8470a04ddd3fbf305be61
Headers show
Series [FFmpeg-devel,v2,01/27] avcodec/threadprogress: Add new API for frame-threaded progress | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt April 8, 2024, 8:13 p.m. UTC 
When outputting a show-existing frame, the VP9 decoder simply
created a reference to said frame and returned it immediately to
the caller, without waiting for it to have finished decoding.
In case of frame-threading it is possible for the frame to
only be decoded while it was waiting to be output.
This is normally benign.

But there is one case where it is not: If the user wants
video encoding parameters to be exported, said side data
will only be attached to the src AVFrame at the end of
decoding the frame that is actually being shown. Without
synchronisation adding said side data in the decoder thread
and the reads in av_frame_ref() in the output thread
constitute a data race. This happens e.g. when using the
venc_data_dump tool with vp90-2-10-show-existing-frame.webm
from the FATE-suite.

Fix this by actually waiting for the frame to be output.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vp9.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Anton Khirnov April 10, 2024, 7:06 a.m. UTC | #1 
Quoting Andreas Rheinhardt (2024-04-08 22:13:44)
> When outputting a show-existing frame, the VP9 decoder simply
> created a reference to said frame and returned it immediately to
> the caller, without waiting for it to have finished decoding.
> In case of frame-threading it is possible for the frame to
> only be decoded while it was waiting to be output.
> This is normally benign.
> 
> But there is one case where it is not: If the user wants
> video encoding parameters to be exported, said side data
> will only be attached to the src AVFrame at the end of
> decoding the frame that is actually being shown. Without
> synchronisation adding said side data in the decoder thread
> and the reads in av_frame_ref() in the output thread
> constitute a data race. This happens e.g. when using the
> venc_data_dump tool with vp90-2-10-show-existing-frame.webm
> from the FATE-suite.
> 
> Fix this by actually waiting for the frame to be output.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/vp9.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)

LGTM, though it'd be nice of someone more familiar with this decoder
looked at it.
diff mbox series

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index bd52478ce7..e0bc313301 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -1569,6 +1569,8 @@  static int vp9_decode_frame(AVCodecContext *avctx, AVFrame *frame,
             av_log(avctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref);
             return AVERROR_INVALIDDATA;
         }
+        ff_progress_frame_await(&s->s.refs[ref], INT_MAX);
+
         if ((ret = av_frame_ref(frame, s->s.refs[ref].f)) < 0)
             return ret;
         frame->pts     = pkt->pts;
@@ -1715,10 +1717,8 @@  static int vp9_decode_frame(AVCodecContext *avctx, AVFrame *frame,
 #endif
         {
             ret = decode_tiles(avctx, data, size);
-            if (ret < 0) {
-                ff_progress_frame_report(&s->s.frames[CUR_FRAME].tf, INT_MAX);
-                return ret;
-            }
+            if (ret < 0)
+                goto fail;
         }
 
         // Sum all counts fields into td[0].counts for tile threading
@@ -1732,18 +1732,19 @@  static int vp9_decode_frame(AVCodecContext *avctx, AVFrame *frame,
             ff_thread_finish_setup(avctx);
         }
     } while (s->pass++ == 1);
-    ff_progress_frame_report(&s->s.frames[CUR_FRAME].tf, INT_MAX);
 
     if (s->td->error_info < 0) {
         av_log(avctx, AV_LOG_ERROR, "Failed to decode tile data\n");
         s->td->error_info = 0;
-        return AVERROR_INVALIDDATA;
+        ret = AVERROR_INVALIDDATA;
+        goto fail;
     }
     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;
+            goto fail;
     }
+    ff_progress_frame_report(&s->s.frames[CUR_FRAME].tf, INT_MAX);
 
 finish:
     // ref frame setup
@@ -1757,6 +1758,9 @@  finish:
     }
 
     return pkt->size;
+fail:
+    ff_progress_frame_report(&s->s.frames[CUR_FRAME].tf, INT_MAX);
+    return ret;
 }
 
 static void vp9_decode_flush(AVCodecContext *avctx)