diff mbox series

[FFmpeg-devel,01/14] avcodec/ffv1dec: Remove redundant writes, fix races

Message ID HE1PR0301MB2154382CF0217C65ABEBEABF8F449@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 17605d1a4afa5761271b27b2aea756a772ca0efc
Headers show
Series [FFmpeg-devel,01/14] avcodec/ffv1dec: Remove redundant writes, fix races | expand

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, 10:53 a.m. UTC 
Every modification of the data that is copied in update_thread_context()
is a data race if it happens after ff_thread_finish_setup. ffv1dec's
update_thread_context() simply uses memcpy for updating the new context,
so that every modification of the src's context is a race.
Some of these modifications are unnecessary: picture_number is write-only
for the decoder and cur will be reset when decoding the next frame anyway.
So remove them. And while just at it, also don't set cur for the slice
contexts as this variable is write-only.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Weirdly ubitux's TSAN fate-box (which uses frame threading by default)
does not show any failing FFV1 tests; although (Clang-)TSAN does it
for me and it is totally obvious that these are data races.

 libavcodec/ffv1dec.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Anton Khirnov April 25, 2021, 10:58 a.m. UTC | #1 
Quoting Andreas Rheinhardt (2021-04-24 12:53:24)
> Every modification of the data that is copied in update_thread_context()
> is a data race if it happens after ff_thread_finish_setup. ffv1dec's
> update_thread_context() simply uses memcpy for updating the new context,
> so that every modification of the src's context is a race.
> Some of these modifications are unnecessary: picture_number is write-only
> for the decoder and cur will be reset when decoding the next frame anyway.
> So remove them. And while just at it, also don't set cur for the slice
> contexts as this variable is write-only.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Weirdly ubitux's TSAN fate-box (which uses frame threading by default)
> does not show any failing FFV1 tests; although (Clang-)TSAN does it
> for me and it is totally obvious that these are data races.

Ok
diff mbox series

Patch

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 8516fef5d7..24dfc68ca4 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -924,7 +924,6 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
             fs->c.bytestream_end = buf_p + v;
 
         fs->avctx = avctx;
-        fs->cur = p;
     }
 
     avctx->execute(avctx,
@@ -966,11 +965,8 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
     }
     ff_thread_report_progress(&f->picture, INT_MAX, 0);
 
-    f->picture_number++;
-
     if (f->last_picture.f)
         ff_thread_release_buffer(avctx, &f->last_picture);
-    f->cur = NULL;
     if ((ret = av_frame_ref(data, f->picture.f)) < 0)
         return ret;