[FFmpeg-devel] [PATCH] dirac: make initialization of arithmetic coder tables threadsafe.

[wm4]

On Mon, 27 Mar 2017 10:09:05 -0400
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> ---
>  libavcodec/dirac_arith.c | 17 +++++++++++------
>  libavcodec/dirac_arith.h |  1 +
>  libavcodec/diracdec.c    |  9 ++++++++-
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/dirac_arith.c b/libavcodec/dirac_arith.c
> index bf91392..49c0909 100644
> --- a/libavcodec/dirac_arith.c
> +++ b/libavcodec/dirac_arith.c
> @@ -83,9 +83,19 @@ const uint8_t ff_dirac_next_ctx[DIRAC_CTX_COUNT] = {
>  
>  int16_t ff_dirac_prob_branchless[256][2];
>  
> -void ff_dirac_init_arith_decoder(DiracArith *c, GetBitContext *gb, int length)
> +av_cold void ff_dirac_init_arith_tables(void)
>  {
>      int i;
> +
> +    for (i = 0; i < 256; i++) {
> +        ff_dirac_prob_branchless[i][0] =  ff_dirac_prob[255-i];
> +        ff_dirac_prob_branchless[i][1] = -ff_dirac_prob[i];
> +    }
> +}
> +
> +void ff_dirac_init_arith_decoder(DiracArith *c, GetBitContext *gb, int length)
> +{
> +    int i, ret;
>      align_get_bits(gb);
>  
>      length = FFMIN(length, get_bits_left(gb)/8);
> @@ -106,11 +116,6 @@ void ff_dirac_init_arith_decoder(DiracArith *c, GetBitContext *gb, int length)
>      c->counter = -16;
>      c->range   = 0xffff;
>  
> -    for (i = 0; i < 256; i++) {
> -        ff_dirac_prob_branchless[i][0] =  ff_dirac_prob[255-i];
> -        ff_dirac_prob_branchless[i][1] = -ff_dirac_prob[i];
> -    }
> -
>      for (i = 0; i < DIRAC_CTX_COUNT; i++)
>          c->contexts[i] = 0x8000;
>  }
> diff --git a/libavcodec/dirac_arith.h b/libavcodec/dirac_arith.h
> index 003430a..24a7ca3 100644
> --- a/libavcodec/dirac_arith.h
> +++ b/libavcodec/dirac_arith.h
> @@ -190,6 +190,7 @@ static inline int dirac_get_arith_int(DiracArith *c, int follow_ctx, int data_ct
>      return ret;
>  }
>  
> +void ff_dirac_init_arith_tables(void);
>  void ff_dirac_init_arith_decoder(DiracArith *c, GetBitContext *gb, int length);
>  
>  #endif /* AVCODEC_DIRAC_ARITH_H */
> diff --git a/libavcodec/diracdec.c b/libavcodec/diracdec.c
> index e0604af..202ae94 100644
> --- a/libavcodec/diracdec.c
> +++ b/libavcodec/diracdec.c
> @@ -26,6 +26,7 @@
>   * @author Marco Gerards <marco at gnu.org>, David Conrad, Jordi Ortiz <nenjordi at gmail.com>
>   */
>  
> +#include "libavutil/thread.h"
>  #include "avcodec.h"
>  #include "get_bits.h"
>  #include "bytestream.h"
> @@ -379,10 +380,12 @@ static void free_sequence_buffers(DiracContext *s)
>      av_freep(&s->mcscratch);
>  }
>  
> +static AVOnce dirac_arith_init = AV_ONCE_INIT;
> +
>  static av_cold int dirac_decode_init(AVCodecContext *avctx)
>  {
>      DiracContext *s = avctx->priv_data;
> -    int i;
> +    int i, ret;
>  
>      s->avctx = avctx;
>      s->frame_number = -1;
> @@ -404,6 +407,9 @@ static av_cold int dirac_decode_init(AVCodecContext *avctx)
>              return AVERROR(ENOMEM);
>          }
>      }
> +    ret = ff_thread_once(&dirac_arith_init, ff_dirac_init_arith_tables);
> +    if (ret != 0)
> +        return AVERROR_UNKNOWN;

(I don't think the return value needs to be checked - similar to how we
don't normally check mutex_lock/unlock. A reasonable implementation
would spin or do fixed time sleeps on temporary kernel locking
failures, which is the only thing that could go wrong with correct
parameters and no UB involved.

Checking these calls doesn't hurt either, of course.)

>  
>      return 0;
>  }
> @@ -2299,5 +2305,6 @@ AVCodec ff_dirac_decoder = {
>      .close          = dirac_decode_end,
>      .decode         = dirac_decode_frame,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS | AV_CODEC_CAP_DR1,
> +    .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
>      .flush          = dirac_decode_flush,
>  };

Otherwise I see no issues.