diff mbox series

[FFmpeg-devel,v2,143/162] avcodec/aac*: Make initializing ff_aac_pow*sf_tab thread-safe

Message ID 20201120073327.820745-44-andreas.rheinhardt@gmail.com
State Accepted
Commit 195d8ce85eb73ff283f85dcee63383ec4081e3e7
Headers show
Series VLC, esp. init_vlc patches
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 20, 2020, 7:33 a.m. UTC
This table is currently initialized up to three times: Once by the
encoder and twice by the decoders (once by the fixed and once by the
floating-point decoder); each of these initializations is guarded by an
AVOnce, yet the fact that there are three of them implies that there
might be data races (the fact that each entry is only written to once
(to its final value) when initializing means that this is safe in
practice, yet it is still undefined behaviour). Fix this by only
initializing the table from one place that is guarded by one AVOnce.
This also avoids unnecessary duplications of the init code.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/aacenc.c | 12 +----------
 libavcodec/aactab.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/aactab.h | 46 +--------------------------------------
 3 files changed, 54 insertions(+), 56 deletions(-)

Comments

Michael Niedermayer Nov. 20, 2020, 6:59 p.m. UTC | #1
On Fri, Nov 20, 2020 at 08:33:08AM +0100, Andreas Rheinhardt wrote:
> This table is currently initialized up to three times: Once by the
> encoder and twice by the decoders (once by the fixed and once by the
> floating-point decoder); each of these initializations is guarded by an
> AVOnce, yet the fact that there are three of them implies that there
> might be data races (the fact that each entry is only written to once
> (to its final value) when initializing means that this is safe in
> practice, yet it is still undefined behaviour). Fix this by only
> initializing the table from one place that is guarded by one AVOnce.
> This also avoids unnecessary duplications of the init code.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/aacenc.c | 12 +----------
>  libavcodec/aactab.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/aactab.h | 46 +--------------------------------------
>  3 files changed, 54 insertions(+), 56 deletions(-)

LGTM

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
index bb203981b2..274e5ca294 100644
--- a/libavcodec/aacenc.c
+++ b/libavcodec/aacenc.c
@@ -30,7 +30,6 @@ 
  ***********************************/
 
 #include "libavutil/libm.h"
-#include "libavutil/thread.h"
 #include "libavutil/float_dsp.h"
 #include "libavutil/opt.h"
 #include "avcodec.h"
@@ -49,8 +48,6 @@ 
 
 #include "psymodel.h"
 
-static AVOnce aac_table_init = AV_ONCE_INIT;
-
 static void put_pce(PutBitContext *pb, AVCodecContext *avctx)
 {
     int i, j;
@@ -951,11 +948,6 @@  static av_cold int alloc_buffers(AVCodecContext *avctx, AACEncContext *s)
     return 0;
 }
 
-static av_cold void aac_encode_init_tables(void)
-{
-    ff_aac_tableinit();
-}
-
 static av_cold int aac_encode_init(AVCodecContext *avctx)
 {
     AACEncContext *s = avctx->priv_data;
@@ -1107,10 +1099,8 @@  static av_cold int aac_encode_init(AVCodecContext *avctx)
     if (HAVE_MIPSDSP)
         ff_aac_coder_init_mips(s);
 
-    if ((ret = ff_thread_once(&aac_table_init, &aac_encode_init_tables)) != 0)
-        return AVERROR_UNKNOWN;
-
     ff_af_queue_init(avctx, &s->afq);
+    ff_aac_tableinit();
 
     return 0;
 }
diff --git a/libavcodec/aactab.c b/libavcodec/aactab.c
index df551b058f..79dd13d9cb 100644
--- a/libavcodec/aactab.c
+++ b/libavcodec/aactab.c
@@ -28,7 +28,9 @@ 
  */
 
 #include "libavutil/mem.h"
+#include "libavutil/thread.h"
 #include "aac.h"
+#include "aactab.h"
 
 #include <stdint.h>
 
@@ -3280,3 +3282,53 @@  const DECLARE_ALIGNED(32, int, ff_aac_eld_window_480_fixed)[1800] = {
     0xffecff1c, 0xffed391e, 0xffed740c, 0xffedafb1,
     0xffedebe1, 0xffee287d, 0xffee654e, 0xffeea23f,
 };
+
+static void aac_tableinit(void)
+{
+    /* 2^(i/16) for 0 <= i <= 15 */
+    static const float exp2_lut[] = {
+        1.00000000000000000000,
+        1.04427378242741384032,
+        1.09050773266525765921,
+        1.13878863475669165370,
+        1.18920711500272106672,
+        1.24185781207348404859,
+        1.29683955465100966593,
+        1.35425554693689272830,
+        1.41421356237309504880,
+        1.47682614593949931139,
+        1.54221082540794082361,
+        1.61049033194925430818,
+        1.68179283050742908606,
+        1.75625216037329948311,
+        1.83400808640934246349,
+        1.91520656139714729387,
+    };
+    float t1 = 8.8817841970012523233890533447265625e-16; // 2^(-50)
+    float t2 = 3.63797880709171295166015625e-12; // 2^(-38)
+    int t1_inc_cur, t2_inc_cur;
+    int t1_inc_prev = 0;
+    int t2_inc_prev = 8;
+
+    for (int i = 0; i < 428; i++) {
+        t1_inc_cur = 4 * (i % 4);
+        t2_inc_cur = (8 + 3*i) % 16;
+        if (t1_inc_cur < t1_inc_prev)
+            t1 *= 2;
+        if (t2_inc_cur < t2_inc_prev)
+            t2 *= 2;
+        // A much more efficient and accurate way of doing:
+        // ff_aac_pow2sf_tab[i]  = pow(2, (i - POW_SF2_ZERO) / 4.0);
+        // ff_aac_pow34sf_tab[i] = pow(ff_aac_pow2sf_tab[i], 3.0/4.0);
+        ff_aac_pow2sf_tab[i]  = t1 * exp2_lut[t1_inc_cur];
+        ff_aac_pow34sf_tab[i] = t2 * exp2_lut[t2_inc_cur];
+        t1_inc_prev = t1_inc_cur;
+        t2_inc_prev = t2_inc_cur;
+    }
+}
+
+void ff_aac_tableinit(void)
+{
+    static AVOnce init_static_once = AV_ONCE_INIT;
+    ff_thread_once(&init_static_once, aac_tableinit);
+}
diff --git a/libavcodec/aactab.h b/libavcodec/aactab.h
index 29df6a43f0..ce6a7ba1d2 100644
--- a/libavcodec/aactab.h
+++ b/libavcodec/aactab.h
@@ -42,51 +42,7 @@ 
 extern float ff_aac_pow2sf_tab[428];
 extern float ff_aac_pow34sf_tab[428];
 
-static inline void ff_aac_tableinit(void)
-{
-    int i;
-
-    /* 2^(i/16) for 0 <= i <= 15 */
-    static const float exp2_lut[] = {
-        1.00000000000000000000,
-        1.04427378242741384032,
-        1.09050773266525765921,
-        1.13878863475669165370,
-        1.18920711500272106672,
-        1.24185781207348404859,
-        1.29683955465100966593,
-        1.35425554693689272830,
-        1.41421356237309504880,
-        1.47682614593949931139,
-        1.54221082540794082361,
-        1.61049033194925430818,
-        1.68179283050742908606,
-        1.75625216037329948311,
-        1.83400808640934246349,
-        1.91520656139714729387,
-    };
-    float t1 = 8.8817841970012523233890533447265625e-16; // 2^(-50)
-    float t2 = 3.63797880709171295166015625e-12; // 2^(-38)
-    int t1_inc_cur, t2_inc_cur;
-    int t1_inc_prev = 0;
-    int t2_inc_prev = 8;
-
-    for (i = 0; i < 428; i++) {
-        t1_inc_cur = 4 * (i % 4);
-        t2_inc_cur = (8 + 3*i) % 16;
-        if (t1_inc_cur < t1_inc_prev)
-            t1 *= 2;
-        if (t2_inc_cur < t2_inc_prev)
-            t2 *= 2;
-        // A much more efficient and accurate way of doing:
-        // ff_aac_pow2sf_tab[i] = pow(2, (i - POW_SF2_ZERO) / 4.0);
-        // ff_aac_pow34sf_tab[i] = pow(ff_aac_pow2sf_tab[i], 3.0/4.0);
-        ff_aac_pow2sf_tab[i] = t1 * exp2_lut[t1_inc_cur];
-        ff_aac_pow34sf_tab[i] = t2 * exp2_lut[t2_inc_cur];
-        t1_inc_prev = t1_inc_cur;
-        t2_inc_prev = t2_inc_cur;
-    }
-}
+void ff_aac_tableinit(void);
 
 /* @name ltp_coef
  * Table of the LTP coefficients