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

[Andreas Rheinhardt]

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 at outlook.com>
---
 libavcodec/ffv1dec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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