diff mbox series

[FFmpeg-devel,2/2] avcdec: move setting decode_error_flags from h264 decoder to error concealment code

Message ID 20210524204355.6312-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/error_resilience: cleanup mpeg2 handling
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

Michael Niedermayer May 24, 2021, 8:43 p.m. UTC
This should fix some minor race condition
Untested (no testcase, the flag is not tested by fate)

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/error_resilience.c | 3 +++
 libavcodec/h264dec.c          | 5 -----
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Anton Khirnov May 31, 2021, 9:02 a.m. UTC | #1
Quoting Michael Niedermayer (2021-05-24 22:43:55)
> This should fix some minor race condition
> Untested (no testcase, the flag is not tested by fate)

How will this fix the race? It is still modifying a shared struct.
James Almer May 31, 2021, 1:45 p.m. UTC | #2
On 5/31/2021 6:02 AM, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-05-24 22:43:55)
>> This should fix some minor race condition
>> Untested (no testcase, the flag is not tested by fate)
> 
> How will this fix the race? It is still modifying a shared struct.

s->cur_pic.f->decode_error_flags is already being set in 
ff_er_frame_end(), and valgrind hasn't complained about it, so it may be 
safe to touch it here.
James Almer May 31, 2021, 1:52 p.m. UTC | #3
On 5/24/2021 5:43 PM, Michael Niedermayer wrote:
> This should fix some minor race condition
> Untested (no testcase, the flag is not tested by fate)
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavcodec/error_resilience.c | 3 +++
>   libavcodec/h264dec.c          | 5 -----
>   2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/error_resilience.c b/libavcodec/error_resilience.c
> index 68bc10ac31..e880efcc51 100644
> --- a/libavcodec/error_resilience.c
> +++ b/libavcodec/error_resilience.c
> @@ -907,6 +907,9 @@ void ff_er_frame_end(ERContext *s)
>       int is_intra_likely;
>       int size = s->b8_stride * 2 * s->mb_height;
>   
> +    if (s->error_occurred && s->cur_pic.f)

s->cur_pic.f is accessed unconditionally right below this chunk, so no 
need for that check.

> +        s->cur_pic.f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;

You may want to merge or group setting this flag with the 
FF_DECODE_ERROR_CONCEALMENT_ACTIVE one a couple hundred lines into the 
function.

> +
>       /* We do not support ER of field pictures yet,
>        * though it should not crash if enabled. */
>       if (!s->avctx->error_concealment || !atomic_load(&s->error_count)  ||
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 38f8967265..05e46cb456 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -716,11 +716,6 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
>       if (ret < 0 && (h->avctx->err_recognition & AV_EF_EXPLODE))
>           goto end;
>   
> -    // set decode_error_flags to allow users to detect concealed decoding errors
> -    if ((ret < 0 || h->slice_ctx->er.error_occurred) && h->cur_pic_ptr) {
> -        h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
> -    }
> -
>       ret = 0;
>   end:
>   
>
diff mbox series

Patch

diff --git a/libavcodec/error_resilience.c b/libavcodec/error_resilience.c
index 68bc10ac31..e880efcc51 100644
--- a/libavcodec/error_resilience.c
+++ b/libavcodec/error_resilience.c
@@ -907,6 +907,9 @@  void ff_er_frame_end(ERContext *s)
     int is_intra_likely;
     int size = s->b8_stride * 2 * s->mb_height;
 
+    if (s->error_occurred && s->cur_pic.f)
+        s->cur_pic.f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
+
     /* We do not support ER of field pictures yet,
      * though it should not crash if enabled. */
     if (!s->avctx->error_concealment || !atomic_load(&s->error_count)  ||
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 38f8967265..05e46cb456 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -716,11 +716,6 @@  static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
     if (ret < 0 && (h->avctx->err_recognition & AV_EF_EXPLODE))
         goto end;
 
-    // set decode_error_flags to allow users to detect concealed decoding errors
-    if ((ret < 0 || h->slice_ctx->er.error_occurred) && h->cur_pic_ptr) {
-        h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
-    }
-
     ret = 0;
 end: