diff mbox series

[FFmpeg-devel,28/39] lavc/ffv1dec: fix races in accessing FFV1SliceContext.slice_damaged

Message ID 20240716171155.31838-28-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,01/39] tests/fate/vcodec: add vsynth tests for FFV1 version 2 | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Anton Khirnov July 16, 2024, 5:11 p.m. UTC 
That variable is shared between frame threads in the same defective way
described in the previous commit. Fix it by adding a RefStruct-managed
arrays of flags that is propagated across frame threads in the standard
manner.

Remove now-unused FFV1Context.fsrc
---
 libavcodec/ffv1.c    |  2 ++
 libavcodec/ffv1.h    |  3 ++-
 libavcodec/ffv1dec.c | 24 ++++++++++--------------
 3 files changed, 14 insertions(+), 15 deletions(-)

Comments

Michael Niedermayer July 17, 2024, 8:51 p.m. UTC | #1 
On Tue, Jul 16, 2024 at 07:11:43PM +0200, Anton Khirnov wrote:
> That variable is shared between frame threads in the same defective way
> described in the previous commit. Fix it by adding a RefStruct-managed
> arrays of flags that is propagated across frame threads in the standard
> manner.
> 
> Remove now-unused FFV1Context.fsrc
> ---
>  libavcodec/ffv1.c    |  2 ++
>  libavcodec/ffv1.h    |  3 ++-
>  libavcodec/ffv1dec.c | 24 ++++++++++--------------
>  3 files changed, 14 insertions(+), 15 deletions(-)

breaks error handling
and introduces a race

try any of the files from:
https://samples.ffmpeg.org/avi/ffv1/

./ffmpeg  -i ffv1.3-01ec.avi -f crc -
the results are vissually worse, and differ from run to run

also
./ffmpeg -thread_type slice -i ffv1.3-01ec.avi -f crc -
has issues, its not just the default

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c
index 9c219b5ddb..333fb3d79b 100644
--- a/libavcodec/ffv1.c
+++ b/libavcodec/ffv1.c
@@ -214,6 +214,8 @@  av_cold int ff_ffv1_close(AVCodecContext *avctx)
         ff_refstruct_unref(&sc->plane);
     }
 
+    ff_refstruct_unref(&s->slice_damaged);
+
     av_freep(&avctx->stats_out);
     for (j = 0; j < s->quant_table_count; j++) {
         av_freep(&s->initial_states[j]);
diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
index edc3f6aef0..ae62732650 100644
--- a/libavcodec/ffv1.h
+++ b/libavcodec/ffv1.h
@@ -118,7 +118,6 @@  typedef struct FFV1Context {
     int64_t picture_number;
     int key_frame;
     ProgressFrame picture, last_picture;
-    struct FFV1Context *fsrc;
 
     const AVFrame *cur_enc_frame;
     int plane_count;
@@ -148,6 +147,8 @@  typedef struct FFV1Context {
     int num_h_slices;
 
     FFV1SliceContext *slices;
+    // RefStruct object, per-slice damage flags
+    uint8_t          *slice_damaged;
 } FFV1Context;
 
 int ff_ffv1_common_init(AVCodecContext *avctx);
diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 7dc4a537a9..c9ac850d98 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -263,16 +263,9 @@  static int decode_slice(AVCodecContext *c, void *arg)
     const int      si = sc - f->slices;
     GetBitContext gb;
 
-    if (f->fsrc && !(p->flags & AV_FRAME_FLAG_KEY) && f->last_picture.f)
+    if (!(p->flags & AV_FRAME_FLAG_KEY) && f->last_picture.f)
         ff_progress_frame_await(&f->last_picture, si);
 
-    if (f->fsrc) {
-        const FFV1SliceContext *scsrc = &f->fsrc->slices[si];
-
-        if (!(p->flags & AV_FRAME_FLAG_KEY))
-            sc->slice_damaged |= scsrc->slice_damaged;
-    }
-
     sc->slice_rct_by_coef = 1;
     sc->slice_rct_ry_coef = 1;
 
@@ -347,6 +340,8 @@  static int decode_slice(AVCodecContext *c, void *arg)
         }
     }
 
+    f->slice_damaged[si] = sc->slice_damaged;
+
     ff_progress_frame_report(&f->picture, si);
 
     return 0;
@@ -767,11 +762,14 @@  static int read_header(FFV1Context *f)
         return AVERROR_INVALIDDATA;
     }
 
+    ff_refstruct_unref(&f->slice_damaged);
+    f->slice_damaged = ff_refstruct_allocz(f->slice_count * sizeof(*f->slice_damaged));
+    if (!f->slice_damaged)
+        return AVERROR(ENOMEM);
+
     for (int j = 0; j < f->slice_count; j++) {
         FFV1SliceContext *sc = &f->slices[j];
 
-        sc->slice_damaged = 0;
-
         if (f->version == 2) {
             int sx = get_symbol(c, state, 0);
             int sy = get_symbol(c, state, 0);
@@ -1041,8 +1039,6 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
         FFV1SliceContext       *sc  = &fdst->slices[i];
         const FFV1SliceContext *sc0 = &fsrc->slices[i];
 
-        sc->slice_damaged = sc0->slice_damaged;
-
         ff_refstruct_replace(&sc->plane, sc0->plane);
 
         if (fsrc->version < 3) {
@@ -1053,12 +1049,12 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
         }
     }
 
+    ff_refstruct_replace(&fdst->slice_damaged, fsrc->slice_damaged);
+
     av_assert1(fdst->max_slice_count == fsrc->max_slice_count);
 
     ff_progress_frame_replace(&fdst->picture, &fsrc->picture);
 
-    fdst->fsrc = fsrc;
-
     return 0;
 }
 #endif