diff mbox series

[FFmpeg-devel,02/39] avcodec/mpeg12: Don't initialize encoder-only parts of RLTable

Message ID 20201210111657.2276739-3-andreas.rheinhardt@gmail.com
State Accepted
Commit 6e8fcd9c5624c1a89e49803de9e10562ace61b6a
Headers show
Series Make mpegvideo encoders init-threadsafe
Related show

Checks

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

Commit Message

Andreas Rheinhardt Dec. 10, 2020, 11:16 a.m. UTC
ff_mpeg12_init_vlcs() currently initializes index_run, max_level and
max_run of ff_rl_mpeg1/2; yet the only user of these fields is the
MPEG-1/2 encoder which already initializes these tables on its own.
So remove the initializations in ff_mpeg12_init_vlcs(); this also
simplifies making ff_mpeg12_init_vlcs() thread-safe.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/mpeg12.c    | 4 ----
 libavcodec/mpeg12.h    | 2 --
 libavcodec/mpeg12enc.c | 6 ++++--
 3 files changed, 4 insertions(+), 8 deletions(-)

Comments

Anton Khirnov Dec. 30, 2020, 5:36 p.m. UTC | #1
Quoting Andreas Rheinhardt (2020-12-10 12:16:20)
> ff_mpeg12_init_vlcs() currently initializes index_run, max_level and
> max_run of ff_rl_mpeg1/2; yet the only user of these fields is the
> MPEG-1/2 encoder which already initializes these tables on its own.
> So remove the initializations in ff_mpeg12_init_vlcs(); this also
> simplifies making ff_mpeg12_init_vlcs() thread-safe.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/mpeg12.c    | 4 ----
>  libavcodec/mpeg12.h    | 2 --
>  libavcodec/mpeg12enc.c | 6 ++++--
>  3 files changed, 4 insertions(+), 8 deletions(-)

Looks good, thought it's really confusing how rl_vlc is initialized both
by ff_rl_init() and by INIT_2D_VLC_RL()
Andreas Rheinhardt Dec. 30, 2020, 7:01 p.m. UTC | #2
Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-12-10 12:16:20)
>> ff_mpeg12_init_vlcs() currently initializes index_run, max_level and
>> max_run of ff_rl_mpeg1/2; yet the only user of these fields is the
>> MPEG-1/2 encoder which already initializes these tables on its own.
>> So remove the initializations in ff_mpeg12_init_vlcs(); this also
>> simplifies making ff_mpeg12_init_vlcs() thread-safe.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/mpeg12.c    | 4 ----
>>  libavcodec/mpeg12.h    | 2 --
>>  libavcodec/mpeg12enc.c | 6 ++++--
>>  3 files changed, 4 insertions(+), 8 deletions(-)
> 
> Looks good, thought it's really confusing how rl_vlc is initialized both
> by ff_rl_init() and by INIT_2D_VLC_RL()
> 
It is actually easy: ff_rl_init() initializes index_run, max_level and
max_run (without touching rl_vlc in any way); ff_rl_init_vlc() and
ff_mpeg12_init_vlcs() initialize rl_vlc (via the macros INIT_VLC_RL()
and INIT_2D_VLC_RL() respectively) without touching what ff_rl_init()
sets at all. So rl_vlc is not initialized by ff_rl_init() at all.

- Andreas
Anton Khirnov Dec. 30, 2020, 7:55 p.m. UTC | #3
Quoting Andreas Rheinhardt (2020-12-30 20:01:08)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2020-12-10 12:16:20)
> >> ff_mpeg12_init_vlcs() currently initializes index_run, max_level and
> >> max_run of ff_rl_mpeg1/2; yet the only user of these fields is the
> >> MPEG-1/2 encoder which already initializes these tables on its own.
> >> So remove the initializations in ff_mpeg12_init_vlcs(); this also
> >> simplifies making ff_mpeg12_init_vlcs() thread-safe.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >>  libavcodec/mpeg12.c    | 4 ----
> >>  libavcodec/mpeg12.h    | 2 --
> >>  libavcodec/mpeg12enc.c | 6 ++++--
> >>  3 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > Looks good, thought it's really confusing how rl_vlc is initialized both
> > by ff_rl_init() and by INIT_2D_VLC_RL()
> > 
> It is actually easy: ff_rl_init() initializes index_run, max_level and
> max_run (without touching rl_vlc in any way); ff_rl_init_vlc() and
> ff_mpeg12_init_vlcs() initialize rl_vlc (via the macros INIT_VLC_RL()
> and INIT_2D_VLC_RL() respectively) without touching what ff_rl_init()
> sets at all. So rl_vlc is not initialized by ff_rl_init() at all.

Ah right, I confused ff_rl_init_vlc() for ff_rl_init(). Nevermind then.
diff mbox series

Patch

diff --git a/libavcodec/mpeg12.c b/libavcodec/mpeg12.c
index e4f007aec5..b4ef41e12d 100644
--- a/libavcodec/mpeg12.c
+++ b/libavcodec/mpeg12.c
@@ -41,8 +41,6 @@ 
 #include "bytestream.h"
 #include "thread.h"
 
-uint8_t ff_mpeg12_static_rl_table_store[2][2][2*MAX_RUN + MAX_LEVEL + 3];
-
 static const uint8_t table_mb_ptype[7][2] = {
     { 3, 5 }, // 0x01 MB_INTRA
     { 1, 2 }, // 0x02 MB_PAT
@@ -163,8 +161,6 @@  av_cold void ff_mpeg12_init_vlcs(void)
         INIT_VLC_STATIC(&ff_mb_btype_vlc, MB_BTYPE_VLC_BITS, 11,
                         &table_mb_btype[0][1], 2, 1,
                         &table_mb_btype[0][0], 2, 1, 64);
-        ff_rl_init(&ff_rl_mpeg1, ff_mpeg12_static_rl_table_store[0]);
-        ff_rl_init(&ff_rl_mpeg2, ff_mpeg12_static_rl_table_store[1]);
 
         INIT_2D_VLC_RL(ff_rl_mpeg1, 680, 0);
         INIT_2D_VLC_RL(ff_rl_mpeg2, 674, 0);
diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h
index 345d473d3a..76fc0bf955 100644
--- a/libavcodec/mpeg12.h
+++ b/libavcodec/mpeg12.h
@@ -25,8 +25,6 @@ 
 #include "mpeg12vlc.h"
 #include "mpegvideo.h"
 
-extern uint8_t ff_mpeg12_static_rl_table_store[2][2][2*MAX_RUN + MAX_LEVEL + 3];
-
 void ff_mpeg12_common_init(MpegEncContext *s);
 
 #define INIT_2D_VLC_RL(rl, static_size, flags)\
diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
index ac4af19ae7..05fd8c0e00 100644
--- a/libavcodec/mpeg12enc.c
+++ b/libavcodec/mpeg12enc.c
@@ -1037,8 +1037,10 @@  void ff_mpeg1_encode_mb(MpegEncContext *s, int16_t block[8][64],
 
 static av_cold void mpeg12_encode_init_static(void)
 {
-    ff_rl_init(&ff_rl_mpeg1, ff_mpeg12_static_rl_table_store[0]);
-    ff_rl_init(&ff_rl_mpeg2, ff_mpeg12_static_rl_table_store[1]);
+    static uint8_t mpeg12_static_rl_table_store[2][2][2*MAX_RUN + MAX_LEVEL + 3];
+
+    ff_rl_init(&ff_rl_mpeg1, mpeg12_static_rl_table_store[0]);
+    ff_rl_init(&ff_rl_mpeg2, mpeg12_static_rl_table_store[1]);
 
     for (int i = 0; i < 64; i++) {
         mpeg1_max_level[0][i] = ff_rl_mpeg1.max_level[0][i];