diff mbox

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

Message ID 1490623745-42356-1-git-send-email-rsbultje@gmail.com
State Accepted
Headers show

Commit Message

Ronald S. Bultje March 27, 2017, 2:09 p.m. UTC
---
 libavcodec/dirac_arith.c | 17 +++++++++++------
 libavcodec/dirac_arith.h |  1 +
 libavcodec/diracdec.c    |  9 ++++++++-
 3 files changed, 20 insertions(+), 7 deletions(-)

Comments

Rostislav Pehlivanov March 27, 2017, 2:24 p.m. UTC | #1
On 27 March 2017 at 15:09, Ronald S. Bultje <rsbultje@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@gnu.org>, David Conrad, Jordi Ortiz <
> nenjordi@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;
>
>      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,
>  };
> --
> 2.8.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

LGTM, feel free to apply
wm4 March 27, 2017, 2:54 p.m. UTC | #2
On Mon, 27 Mar 2017 10:09:05 -0400
"Ronald S. Bultje" <rsbultje@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@gnu.org>, David Conrad, Jordi Ortiz <nenjordi@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.
Michael Niedermayer March 27, 2017, 3:10 p.m. UTC | #3
On Mon, Mar 27, 2017 at 10:09:05AM -0400, Ronald S. Bultje 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;

ret seems unused

no other comments from me

[...]
Ronald S. Bultje March 27, 2017, 3:24 p.m. UTC | #4
Hi,

On Mon, Mar 27, 2017 at 11:10 AM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> On Mon, Mar 27, 2017 at 10:09:05AM -0400, Ronald S. Bultje 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;
>
> ret seems unused


Will remove locally.

Ronald
Ronald S. Bultje March 28, 2017, 3:24 p.m. UTC | #5
Hi,

On Mon, Mar 27, 2017 at 11:24 AM, Ronald S. Bultje <rsbultje@gmail.com>
wrote:

> Hi,
>
> On Mon, Mar 27, 2017 at 11:10 AM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
>
>> On Mon, Mar 27, 2017 at 10:09:05AM -0400, Ronald S. Bultje 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;
>>
>> ret seems unused
>
>
> Will remove locally.
>

Removed, and pushed.

Ronald
diff mbox

Patch

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@gnu.org>, David Conrad, Jordi Ortiz <nenjordi@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;
 
     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,
 };