[FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

[Michael Niedermayer]

On Sun, Nov 19, 2017 at 03:23:47PM -0800, Nick Lewycky wrote:
[...]
>  error_resilience.c |   22 ++++++++++++----------
>  error_resilience.h |    3 ++-
>  h264_slice.c       |    6 ++++--
>  mpeg12dec.c        |   11 ++++++-----
>  mpegvideo_enc.c    |    5 ++++-
>  5 files changed, 28 insertions(+), 19 deletions(-)
> 1978e414145ffde47b44859fe6c8e05b1459741d  0001-libavcodec-error_resilience.h-Use-C11-atomics-for-ER.patch
> From 6a469111d07fb7ddc01f6a19d4bbfee3e20d738e Mon Sep 17 00:00:00 2001
> From: Nick Lewycky <nlewycky at google.com>
> Date: Thu, 16 Nov 2017 11:50:38 -0800
> Subject: [PATCH] libavcodec/error_resilience.h: Use C11 atomics for ERContext
>  error_count.
> 
> ---
>  libavcodec/error_resilience.c | 22 ++++++++++++----------
>  libavcodec/error_resilience.h |  3 ++-
>  libavcodec/h264_slice.c       |  6 ++++--
>  libavcodec/mpeg12dec.c        | 11 ++++++-----
>  libavcodec/mpegvideo_enc.c    |  5 ++++-
>  5 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/libavcodec/error_resilience.c b/libavcodec/error_resilience.c
> index 0c7f29d171..0da5777b52 100644
> --- a/libavcodec/error_resilience.c
> +++ b/libavcodec/error_resilience.c
> @@ -25,6 +25,7 @@
>   * Error resilience / concealment.
>   */
>  
> +#include <stdatomic.h>
>  #include <limits.h>
>  
>  #include "libavutil/atomic.h"
> @@ -807,7 +808,7 @@ void ff_er_frame_start(ERContext *s)
>  
>      memset(s->error_status_table, ER_MB_ERROR | VP_START | ER_MB_END,
>             s->mb_stride * s->mb_height * sizeof(uint8_t));
> -    s->error_count    = 3 * s->mb_num;
> +    atomic_store(&s->error_count, 3 * s->mb_num);

This cannot execute with anything in paralel so shouldnt need any
fancy way of storing i think


>      s->error_occurred = 0;
>  }
>  
> @@ -852,20 +853,20 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
>      mask &= ~VP_START;
>      if (status & (ER_AC_ERROR | ER_AC_END)) {
>          mask           &= ~(ER_AC_ERROR | ER_AC_END);
> -        avpriv_atomic_int_add_and_fetch(&s->error_count, start_i - end_i - 1);
> +        atomic_fetch_add(&s->error_count, start_i - end_i - 1);
>      }
>      if (status & (ER_DC_ERROR | ER_DC_END)) {
>          mask           &= ~(ER_DC_ERROR | ER_DC_END);
> -        avpriv_atomic_int_add_and_fetch(&s->error_count, start_i - end_i - 1);
> +        atomic_fetch_add(&s->error_count, start_i - end_i - 1);
>      }
>      if (status & (ER_MV_ERROR | ER_MV_END)) {
>          mask           &= ~(ER_MV_ERROR | ER_MV_END);
> -        avpriv_atomic_int_add_and_fetch(&s->error_count, start_i - end_i - 1);
> +        atomic_fetch_add(&s->error_count, start_i - end_i - 1);
>      }
>  
>      if (status & ER_MB_ERROR) {
>          s->error_occurred = 1;
> -        avpriv_atomic_int_set(&s->error_count, INT_MAX);
> +        atomic_store(&s->error_count, INT_MAX);
>      }
>  
>      if (mask == ~0x7F) {
> @@ -878,7 +879,7 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
>      }
>  
>      if (end_i == s->mb_num)
> -        avpriv_atomic_int_set(&s->error_count, INT_MAX);
> +        atomic_store(&s->error_count, INT_MAX);
>      else {
>          s->error_status_table[end_xy] &= mask;
>          s->error_status_table[end_xy] |= status;
> @@ -893,7 +894,7 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
>          prev_status &= ~ VP_START;
>          if (prev_status != (ER_MV_END | ER_DC_END | ER_AC_END)) {
>              s->error_occurred = 1;
> -            avpriv_atomic_int_set(&s->error_count, INT_MAX);
> +            atomic_store(&s->error_count, INT_MAX);
>          }
>      }
>  }

> @@ -910,10 +911,11 @@ void ff_er_frame_end(ERContext *s)
>  
>      /* We do not support ER of field pictures yet,
>       * though it should not crash if enabled. */
> -    if (!s->avctx->error_concealment || s->error_count == 0            ||
> +    if (!s->avctx->error_concealment                                   ||
> +        atomic_load(&s->error_count) == 0                              ||
>          s->avctx->lowres                                               ||
>          !er_supported(s)                                               ||
> -        s->error_count == 3 * s->mb_width *
> +        atomic_load(&s->error_count) == 3 * s->mb_width *
>                            (s->avctx->skip_top + s->avctx->skip_bottom)) {
>          return;
>      }
> @@ -927,7 +929,7 @@ void ff_er_frame_end(ERContext *s)
>      if (   mb_x == s->mb_width
>          && s->avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO
>          && (FFALIGN(s->avctx->height, 16)&16)
> -        && s->error_count == 3 * s->mb_width * (s->avctx->skip_top + s->avctx->skip_bottom + 1)
> +        && atomic_load(&s->error_count) == 3 * s->mb_width * (s->avctx->skip_top + s->avctx->skip_bottom + 1)
>      ) {
>          av_log(s->avctx, AV_LOG_DEBUG, "ignoring last missing slice\n");
>          return;

like frame start, also frame end should not execute in paralel with
anything, so i think it doesnt need any fancy access functions

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171120/6aa2b214/attachment.sig>