| Message ID | 20201210111657.2276739-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6e8fcd9c5624c1a89e49803de9e10562ace61b6a |
| Headers | show |
| Series | Make mpegvideo encoders init-threadsafe | expand |
| 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 |
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()
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
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 --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];
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(-)