[FFmpeg-devel] [PATCH v3] avcodec: fix atomics usage for h264/mpeg error_count

[Aman Gupta]

On Mon, Mar 12, 2018 at 6:49 PM, Aman Gupta <ffmpeg at tmm1.net> wrote:

> From: Aman Gupta <aman at tmm1.net>
>
> ---
>  libavcodec/h264_slice.c    |  5 +++--
>  libavcodec/mpeg12dec.c     | 12 +++++++-----
>  libavcodec/mpegvideo_enc.c |  3 ++-
>  3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 90e05ed8f1..b381397b4d 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -2762,7 +2762,7 @@ int ff_h264_execute_decode_slices(H264Context *h)
>
>              sl                 = &h->slice_ctx[i];
>              if (CONFIG_ERROR_RESILIENCE) {
> -                sl->er.error_count = 0;
> +                sl->er.error_count = ATOMIC_VAR_INIT(0);
>              }
>
>              /* make sure none of those slices overlap */
> @@ -2786,7 +2786,8 @@ int ff_h264_execute_decode_slices(H264Context *h)
>          h->mb_y              = sl->mb_y;
>          if (CONFIG_ERROR_RESILIENCE) {
>              for (i = 1; i < context_count; i++)
> -                h->slice_ctx[0].er.error_count +=
> h->slice_ctx[i].er.error_count;
> +                atomic_fetch_add(&h->slice_ctx[0].er.error_count,
> +                                 atomic_load(&h->slice_ctx[i].
> er.error_count));
>          }
>
>          if (h->postpone_filter) {
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 9e076e89da..5a8570672c 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1992,7 +1992,7 @@ static int slice_decode_thread(AVCodecContext *c,
> void *arg)
>      int mb_y            = s->start_mb_y;
>      const int field_pic = s->picture_structure != PICT_FRAME;
>
> -    s->er.error_count = (3 * (s->end_mb_y - s->start_mb_y) * s->mb_width)
> >> field_pic;
> +    s->er.error_count = ATOMIC_VAR_INIT((3 * (s->end_mb_y -
> s->start_mb_y) * s->mb_width) >> field_pic);
>
>      for (;;) {
>          uint32_t start_code;
> @@ -2002,7 +2002,7 @@ static int slice_decode_thread(AVCodecContext *c,
> void *arg)
>          emms_c();
>          ff_dlog(c, "ret:%d resync:%d/%d mb:%d/%d ts:%d/%d ec:%d\n",
>                  ret, s->resync_mb_x, s->resync_mb_y, s->mb_x, s->mb_y,
> -                s->start_mb_y, s->end_mb_y, s->er.error_count);
> +                s->start_mb_y, s->end_mb_y, atomic_load(&s->er.error_
> count));
>          if (ret < 0) {
>              if (c->err_recognition & AV_EF_EXPLODE)
>                  return ret;
> @@ -2485,7 +2485,8 @@ static int decode_chunks(AVCodecContext *avctx,
> AVFrame *picture,
>                                     &s2->thread_context[0], NULL,
>                                     s->slice_count, sizeof(void *));
>                      for (i = 0; i < s->slice_count; i++)
> -                        s2->er.error_count += s2->thread_context[i]->er.
> error_count;
> +                        atomic_fetch_add(&s2->er.error_count,
> +                                         atomic_load(&s2->thread_
> context[i]->er.error_count));
>                  }
>
>                  ret = slice_end(avctx, picture);
> @@ -2499,7 +2500,7 @@ static int decode_chunks(AVCodecContext *avctx,
> AVFrame *picture,
>              }
>              s2->pict_type = 0;
>
> -            if (avctx->err_recognition & AV_EF_EXPLODE &&
> s2->er.error_count)
> +            if (avctx->err_recognition & AV_EF_EXPLODE &&
> atomic_load(&s2->er.error_count))
>                  return AVERROR_INVALIDDATA;
>
>              return FFMAX(0, buf_ptr - buf - s2->parse_context.last_index);
> @@ -2553,7 +2554,8 @@ static int decode_chunks(AVCodecContext *avctx,
> AVFrame *picture,
>                                 s2->thread_context, NULL,
>                                 s->slice_count, sizeof(void *));
>                  for (i = 0; i < s->slice_count; i++)
> -                    s2->er.error_count += s2->thread_context[i]->er.
> error_count;
> +                    atomic_fetch_add(&s2->er.error_count,
> +                                     atomic_load(&s2->thread_
> context[i]->er.error_count));
>                  s->slice_count = 0;
>              }
>              if (last_code == 0 || last_code == SLICE_MIN_START_CODE) {
> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> index 979e138b88..3447c5e87a 100644
> --- a/libavcodec/mpegvideo_enc.c
> +++ b/libavcodec/mpegvideo_enc.c
> @@ -3595,7 +3595,8 @@ static void merge_context_after_encode(MpegEncContext
> *dst, MpegEncContext *src)
>      MERGE(b_count);
>      MERGE(skip_count);
>      MERGE(misc_bits);
> -    MERGE(er.error_count);
> +    atomic_fetch_add(&dst->er.error_count, atomic_load(&src->er.error_
> count));
> +    atomic_store(&src->er.error_count, 0);
>      MERGE(padding_bug_score);
>      MERGE(current_picture.encoding_error[0]);
>      MERGE(current_picture.encoding_error[1]);
>

There was some uncertainty on IRC about whether the usages of
atomic_store() and ATOMIC_VAR_INIT() in this patch were correct, or needed
to be swapped.

Would appreciate if someone familiar with the error resilience code could
comment. /cc @michaelni

Aman


> --
> 2.14.2
>
>