diff mbox series

[FFmpeg-devel,02/14] avcodec/ffv1dec: Don't set ThreadFrame properties, fix race

Message ID HE1PR0301MB215454691198418840CC624B8F449@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
Each FFV1 slice has its own SAR and picture structure encoded;
when a slice header was parsed, the relevant fields of a ThreadFrame's
AVFrame were directly set based upon the parsed values. This is
a data race in case slice threading is in use because of the concurrent
writes. In case of frame threading, it is also a data race because
the writes happen after ff_thread_finish_setup(), so that the reads
performed by ff_thread_ref_frame() are unsynchronized with the writes
performed when parsing the header.

This commit fixes these issues by not writing to the ThreadFrame at all;
instead the raw data is read into the each SliceContext first; after
decoding the current frame and creating the actual output frame these
values are compared to each other. If they are valid and coincide, the
derived value is written directly to the output frame, not to the
ThreadFrame, thereby avoiding data races.

This fixes most FFV1 FATE-tests completely when using slice threading;
the exceptions are fate-vsynth3-ffv1, vsynth3-ffv1-v3-yuv420p and
vsynth3-ffv1-v3-yuv422p10. (In these tests the partitioning into slices
does not honour chroma subsampling; as a result, chroma pixels at slice
borders get set by more than one thread without any synchronization.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
The spec [1] does not rule out that these values differ on a per-slice
basis; I don't know whether this is even intended (is there a use-case
for this?).
That the slice dimensions needn't be compatible with subsampling is very
weird. I think it is possible for two adjacent slices to set different
values for the same chroma pixel.

 libavcodec/ffv1.h    |  2 ++
 libavcodec/ffv1dec.c | 62 ++++++++++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 22 deletions(-)

Comments

Michael Niedermayer April 25, 2021, 12:17 p.m. UTC | #1
On Sat, Apr 24, 2021 at 01:14:34PM +0200, Andreas Rheinhardt wrote:
> Each FFV1 slice has its own SAR and picture structure encoded;
> when a slice header was parsed, the relevant fields of a ThreadFrame's
> AVFrame were directly set based upon the parsed values. This is
> a data race in case slice threading is in use because of the concurrent
> writes. In case of frame threading, it is also a data race because
> the writes happen after ff_thread_finish_setup(), so that the reads
> performed by ff_thread_ref_frame() are unsynchronized with the writes
> performed when parsing the header.
> 
> This commit fixes these issues by not writing to the ThreadFrame at all;
> instead the raw data is read into the each SliceContext first; after
> decoding the current frame and creating the actual output frame these
> values are compared to each other. If they are valid and coincide, the
> derived value is written directly to the output frame, not to the
> ThreadFrame, thereby avoiding data races.
> 
> This fixes most FFV1 FATE-tests completely when using slice threading;
> the exceptions are fate-vsynth3-ffv1, vsynth3-ffv1-v3-yuv420p and
> vsynth3-ffv1-v3-yuv422p10. (In these tests the partitioning into slices
> does not honour chroma subsampling; as a result, chroma pixels at slice
> borders get set by more than one thread without any synchronization.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> The spec [1] does not rule out that these values differ on a per-slice
> basis; I don't know whether this is even intended (is there a use-case
> for this?).
> That the slice dimensions needn't be compatible with subsampling is very
> weird. I think it is possible for two adjacent slices to set different
> values for the same chroma pixel.
> 
>  libavcodec/ffv1.h    |  2 ++
>  libavcodec/ffv1dec.c | 62 ++++++++++++++++++++++++++++----------------
>  2 files changed, 42 insertions(+), 22 deletions(-)
> 
> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
> index 147fe7ae16..81cbe8757d 100644
> --- a/libavcodec/ffv1.h
> +++ b/libavcodec/ffv1.h
> @@ -96,6 +96,8 @@ typedef struct FFV1Context {
>      struct FFV1Context *fsrc;
>  
>      AVFrame *cur;
> +    int picture_structure;
> +    AVRational sample_aspect_ratio;
>      int plane_count;
>      int ac;                              ///< 1=range coder <-> 0=golomb rice
>      int ac_byte_count;                   ///< number of bytes used for AC coding
> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
> index 24dfc68ca4..b3481df922 100644
> --- a/libavcodec/ffv1dec.c
> +++ b/libavcodec/ffv1dec.c
> @@ -165,7 +165,7 @@ static int decode_slice_header(FFV1Context *f, FFV1Context *fs)
>  {
>      RangeCoder *c = &fs->c;
>      uint8_t state[CONTEXT_SIZE];
> -    unsigned ps, i, context_count;
> +    unsigned i, context_count;
>      memset(state, 128, sizeof(state));
>  
>      av_assert0(f->version > 2);
> @@ -203,26 +203,9 @@ static int decode_slice_header(FFV1Context *f, FFV1Context *fs)
>          p->context_count = context_count;
>      }
>  
> -    ps = get_symbol(c, state, 0);
> -    if (ps == 1) {
> -        f->cur->interlaced_frame = 1;
> -        f->cur->top_field_first  = 1;
> -    } else if (ps == 2) {
> -        f->cur->interlaced_frame = 1;
> -        f->cur->top_field_first  = 0;
> -    } else if (ps == 3) {
> -        f->cur->interlaced_frame = 0;
> -    }
> -    f->cur->sample_aspect_ratio.num = get_symbol(c, state, 0);
> -    f->cur->sample_aspect_ratio.den = get_symbol(c, state, 0);
> -
> -    if (av_image_check_sar(f->width, f->height,
> -                           f->cur->sample_aspect_ratio) < 0) {
> -        av_log(f->avctx, AV_LOG_WARNING, "ignoring invalid SAR: %u/%u\n",
> -               f->cur->sample_aspect_ratio.num,
> -               f->cur->sample_aspect_ratio.den);
> -        f->cur->sample_aspect_ratio = (AVRational){ 0, 1 };
> -    }
> +    fs->picture_structure       = get_symbol(c, state, 0);
> +    fs->sample_aspect_ratio.num = get_symbol(c, state, 0);
> +    fs->sample_aspect_ratio.den = get_symbol(c, state, 0);
>  
>      if (fs->version > 3) {
>          fs->slice_reset_contexts = get_rac(c, state);

> @@ -967,9 +950,44 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
>  
>      if (f->last_picture.f)
>          ff_thread_release_buffer(avctx, &f->last_picture);
> -    if ((ret = av_frame_ref(data, f->picture.f)) < 0)
> +    p = data;
> +    if ((ret = av_frame_ref(p, f->picture.f)) < 0)
>          return ret;
>  
> +    if (f->version > 2) {
> +        AVRational sar = f->slice_context[0]->sample_aspect_ratio;
> +        int picture_structure;
> +
> +        for (i = f->slice_count - 1; i > 0; i--)
> +            if (f->slice_context[i]->sample_aspect_ratio.num != sar.num ||
> +                f->slice_context[i]->sample_aspect_ratio.den != sar.den)
> +                break;
> +        if (i > 0) {
> +            av_log(avctx, AV_LOG_WARNING, "ignoring inconsistent SAR\n");
> +            p->sample_aspect_ratio = (AVRational){ 0, 1 };
> +        } else if (av_image_check_sar(f->width, f->height, sar) < 0) {
> +            av_log(avctx, AV_LOG_WARNING, "ignoring invalid SAR: %u/%u\n",
> +                   sar.num, sar.den);
> +            p->sample_aspect_ratio = (AVRational){ 0, 1 };
> +        } else
> +            p->sample_aspect_ratio = sar;
> +
> +        picture_structure = f->slice_context[0]->picture_structure;
> +        for (i = f->slice_count - 1; i > 0; i--)
> +            if (f->slice_context[i]->picture_structure != picture_structure)
> +                break;
> +        if (!i) {
> +            if (picture_structure == 1) {
> +                p->interlaced_frame = 1;
> +                p->top_field_first  = 1;
> +            } else if (picture_structure == 2) {
> +                p->interlaced_frame = 1;
> +                p->top_field_first  = 0;
> +            } else if (picture_structure == 3)
> +                p->interlaced_frame = 0;
> +        }

If you already go to the troubble of collecting the slice header data from
each slice, it would make sense to pick the most often occuring value when
there is an inconsistency probably or to favor slices which decoded without
error

Completely ignoring the values of all slices if one is different defeats
the purpose of replicating the information in the bitstream

thanks

[...]
diff mbox series

Patch

diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
index 147fe7ae16..81cbe8757d 100644
--- a/libavcodec/ffv1.h
+++ b/libavcodec/ffv1.h
@@ -96,6 +96,8 @@  typedef struct FFV1Context {
     struct FFV1Context *fsrc;
 
     AVFrame *cur;
+    int picture_structure;
+    AVRational sample_aspect_ratio;
     int plane_count;
     int ac;                              ///< 1=range coder <-> 0=golomb rice
     int ac_byte_count;                   ///< number of bytes used for AC coding
diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 24dfc68ca4..b3481df922 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -165,7 +165,7 @@  static int decode_slice_header(FFV1Context *f, FFV1Context *fs)
 {
     RangeCoder *c = &fs->c;
     uint8_t state[CONTEXT_SIZE];
-    unsigned ps, i, context_count;
+    unsigned i, context_count;
     memset(state, 128, sizeof(state));
 
     av_assert0(f->version > 2);
@@ -203,26 +203,9 @@  static int decode_slice_header(FFV1Context *f, FFV1Context *fs)
         p->context_count = context_count;
     }
 
-    ps = get_symbol(c, state, 0);
-    if (ps == 1) {
-        f->cur->interlaced_frame = 1;
-        f->cur->top_field_first  = 1;
-    } else if (ps == 2) {
-        f->cur->interlaced_frame = 1;
-        f->cur->top_field_first  = 0;
-    } else if (ps == 3) {
-        f->cur->interlaced_frame = 0;
-    }
-    f->cur->sample_aspect_ratio.num = get_symbol(c, state, 0);
-    f->cur->sample_aspect_ratio.den = get_symbol(c, state, 0);
-
-    if (av_image_check_sar(f->width, f->height,
-                           f->cur->sample_aspect_ratio) < 0) {
-        av_log(f->avctx, AV_LOG_WARNING, "ignoring invalid SAR: %u/%u\n",
-               f->cur->sample_aspect_ratio.num,
-               f->cur->sample_aspect_ratio.den);
-        f->cur->sample_aspect_ratio = (AVRational){ 0, 1 };
-    }
+    fs->picture_structure       = get_symbol(c, state, 0);
+    fs->sample_aspect_ratio.num = get_symbol(c, state, 0);
+    fs->sample_aspect_ratio.den = get_symbol(c, state, 0);
 
     if (fs->version > 3) {
         fs->slice_reset_contexts = get_rac(c, state);
@@ -967,9 +950,44 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
 
     if (f->last_picture.f)
         ff_thread_release_buffer(avctx, &f->last_picture);
-    if ((ret = av_frame_ref(data, f->picture.f)) < 0)
+    p = data;
+    if ((ret = av_frame_ref(p, f->picture.f)) < 0)
         return ret;
 
+    if (f->version > 2) {
+        AVRational sar = f->slice_context[0]->sample_aspect_ratio;
+        int picture_structure;
+
+        for (i = f->slice_count - 1; i > 0; i--)
+            if (f->slice_context[i]->sample_aspect_ratio.num != sar.num ||
+                f->slice_context[i]->sample_aspect_ratio.den != sar.den)
+                break;
+        if (i > 0) {
+            av_log(avctx, AV_LOG_WARNING, "ignoring inconsistent SAR\n");
+            p->sample_aspect_ratio = (AVRational){ 0, 1 };
+        } else if (av_image_check_sar(f->width, f->height, sar) < 0) {
+            av_log(avctx, AV_LOG_WARNING, "ignoring invalid SAR: %u/%u\n",
+                   sar.num, sar.den);
+            p->sample_aspect_ratio = (AVRational){ 0, 1 };
+        } else
+            p->sample_aspect_ratio = sar;
+
+        picture_structure = f->slice_context[0]->picture_structure;
+        for (i = f->slice_count - 1; i > 0; i--)
+            if (f->slice_context[i]->picture_structure != picture_structure)
+                break;
+        if (!i) {
+            if (picture_structure == 1) {
+                p->interlaced_frame = 1;
+                p->top_field_first  = 1;
+            } else if (picture_structure == 2) {
+                p->interlaced_frame = 1;
+                p->top_field_first  = 0;
+            } else if (picture_structure == 3)
+                p->interlaced_frame = 0;
+        }
+    }
+
     *got_frame = 1;
 
     return buf_size;