diff mbox series

[FFmpeg-devel,11/14] avcodec/ffv1dec: Fix race when updating slice_damaged

Message ID HE1PR0301MB215432FDBFC9CD7434802DE28F449@HE1PR0301MB2154.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,01/14] avcodec/ffv1dec: Remove redundant writes, fix races
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

Andreas Rheinhardt April 24, 2021, 11:14 a.m. UTC
A slice is marked as damaged when its checksums indicate an error or
if it is not a keyframe and the same slice of the preceding frame was
damaged. These checksums are only evaluated after
ff_thread_finish_setup() has been called and therefore setting them
actually constitutes a data race when using frame threading, because
the decoder's update_thread_context copies it. This is undefined
behaviour, but in practice it works: If the src slice is damaged, but
its preceding slice is not, then it is indeed uncertain whether the
dst slice will already be marked as damaged in update_thread_context();
but decoding the slice only begins after the src slice has been completely
decoded in which case the dst slice will be marked as damaged if the
src slice is so marked.

Yet it is still a data race; fixing it is easy: Don't copy slice_damaged
in update_thread_context; instead just reset it there and only set it
when it is known whether the src slice is damaged or not, i.e. after the
src slice has been decoded.

This fixes all ffv1-FATE-tests with TSAN when frame-threading is used.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ffv1dec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index c9583db60a..c16fc81927 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -1060,7 +1060,12 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
         FFV1Context *fssrc = fsrc->slice_context[i];
         FFV1Context *fsdst = fdst->slice_context[i];
         copy_fields(fsdst, fsrc);
-        fsdst->slice_damaged = fssrc->slice_damaged;
+        /* Reset fsdst->slice_damaged here. decode_frame() will set it
+         * if the slice crc indicates an error and decode_slice() will
+         * set it after the same slice from the previous frame has
+         * been decoded if said slice has it set. Copying the field
+         * here would be a race. */
+        fsdst->slice_damaged = 0;
         if (fsrc->version < 3) {
             fsdst->slice_x      = fssrc->slice_x;
             fsdst->slice_y      = fssrc->slice_y;