diff mbox series

[FFmpeg-devel,20/39] avcodec/h261dec: Don't initialize unused part of RLTable

Message ID 20201210111657.2276739-21-andreas.rheinhardt@gmail.com
State Accepted
Commit ed913fcb5963eff0e7edf2719d6e4ec025ddab42
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

Commit Message

Andreas Rheinhardt Dec. 10, 2020, 11:16 a.m. UTC
The H.261 decoder only uses an RLTable's VLC table, yet it also
initializes its index_run, max_level and max_run. This commit stops
doing so; it will also simplify making this decoder init-threadsafe,
as the H.261 decoder and encoder now initialize disjoint parts of their
common RLTable.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
The earlier patchset instead guarded ff_h261_common_init() by an AVOnce.

 libavcodec/h261dec.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Anton Khirnov Jan. 18, 2021, 10:44 a.m. UTC | #1
Quoting Andreas Rheinhardt (2020-12-10 12:16:38)
> The H.261 decoder only uses an RLTable's VLC table, yet it also
> initializes its index_run, max_level and max_run. This commit stops
> doing so; it will also simplify making this decoder init-threadsafe,
> as the H.261 decoder and encoder now initialize disjoint parts of their
> common RLTable.

Does it then make sense to keep this RLTable common?
Andreas Rheinhardt Jan. 21, 2021, 8:20 p.m. UTC | #2
Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-12-10 12:16:38)
>> The H.261 decoder only uses an RLTable's VLC table, yet it also
>> initializes its index_run, max_level and max_run. This commit stops
>> doing so; it will also simplify making this decoder init-threadsafe,
>> as the H.261 decoder and encoder now initialize disjoint parts of their
>> common RLTable.
> 
> Does it then make sense to keep this RLTable common?
> 
I presume you want to know whether the RLTable structure should be split
into smaller structures?

To answer this question, let's examine what parts are used for each RLTable.

Initializing an RLTable's VLCs uses all three static tables (table_vlc,
table_run and table_level); initializing the rest only uses table_run
and table_level. get_rl_index (only used in encoders) uses index_run and
max_level, which is initialized by ff_rl_init).

ff_h261_rl_tcoeff:
The H.261 encoder uses table_vlc and get_rl_index (thereby also using
index_run and max_level); the H.261 decoder uses only the first VLC.
max_run is completely unused.

ff_rl_mpeg1/ff_rl_mpeg2:
Only the first VLC table is used by the various decoders using them; the
encoder uses max_level, index_run and table_vlc. max_run is unused.

ff_rl_speedhq:
The decoder uses the first VLC table; the encoder uses table_vlc as well
as max_level and index_run. max_run is unused.

ff_h263_rl_inter:
ituh263dec.c only uses the first VLC table; mpeg4videodec.c uses all the
VLC tables as well as max_run and max_level. ituh263enc.c uses
index_run, max_level as well as table_vlc. mpeg4videoenc.c uses
table_vlc, max_level, max_run as well as index_run.

ff_rl_intra_aic:
ituh263dec.c only uses the first VLC; ituh263enc uses table_vlc,
index_run, max_level. max_run is unused.

ff_rvlc_rl_intra/inter:
Only used in mpeg4videodec.c. For ff_rvlc_rl_intra, only the first VLC
is used; for ff_rvlc_rl_inter, all VLCs are potentially used. max_level,
max_run and index_run are completely unused (notice that I did not
remove the calls to ff_rl_init for these RLTables because I actually do
not have a sample to confirm that these are unused).

ff_rl_table
msmpeg4dec.c uses the VLCs (only the first VLC for 0..2) as well as
max_run and max_level; msmpeg4enc.c uses table_vlc, index_run, max_level
and max_run.

ff_mpeg4_rl_intra
mpeg4videodec.c uses the first VLC as well as max_run and max_level.
mpeg4videoenc.c uses table_vlc as well as index_run, max_level and max_run.

So seven of the ten RLTables (I am counting the ff_rl_tables as one)
don't use max_run at all; and seven of the ten RLTables (but not the
same seven) only use the first VLC. It is possible to remove rl_vlc from
RLTable and make it separate; the code in mpeg4videodec.c and
msmpeg4dec.c (the only places where VLCs other than the first are used)
are already compatible with this. Yet the other things belong together
in the same struct.

- Andreas

PS: It seems you overlooked patches 7-13.
Anton Khirnov Jan. 23, 2021, 6:13 p.m. UTC | #3
Quoting Andreas Rheinhardt (2021-01-21 21:20:52)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2020-12-10 12:16:38)
> >> The H.261 decoder only uses an RLTable's VLC table, yet it also
> >> initializes its index_run, max_level and max_run. This commit stops
> >> doing so; it will also simplify making this decoder init-threadsafe,
> >> as the H.261 decoder and encoder now initialize disjoint parts of their
> >> common RLTable.
> > 
> > Does it then make sense to keep this RLTable common?
> > 
> I presume you want to know whether the RLTable structure should be split
> into smaller structures?

No, what I meant was whether we shouldn't use different RLTable
instances for encoder and decoder, since their use is disjoint. That
would make the code easier to reason about.

> 
> PS: It seems you overlooked patches 7-13.

It's more that I have next to zero experience with/insight into that
code. I don't think I can review on more than the most superficial level
without major effort.
Andreas Rheinhardt Jan. 23, 2021, 6:50 p.m. UTC | #4
Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-01-21 21:20:52)
>> Anton Khirnov:
>>> Quoting Andreas Rheinhardt (2020-12-10 12:16:38)
>>>> The H.261 decoder only uses an RLTable's VLC table, yet it also
>>>> initializes its index_run, max_level and max_run. This commit stops
>>>> doing so; it will also simplify making this decoder init-threadsafe,
>>>> as the H.261 decoder and encoder now initialize disjoint parts of their
>>>> common RLTable.
>>>
>>> Does it then make sense to keep this RLTable common?
>>>
>> I presume you want to know whether the RLTable structure should be split
>> into smaller structures?
> 
> No, what I meant was whether we shouldn't use different RLTable
> instances for encoder and decoder, since their use is disjoint. That
> would make the code easier to reason about.

I actually didn't think that these RLTables were difficult to reason
about: ff_rl_init and ff_rl_init_vlc/ff_init_2d_vlc_rl initialize
different parts of an RLTable and both only use the static parts of an
RLTable, so that these two can be called independently. In particular
there is no clash in the case of H.261 after the unnecessary call to
ff_rl_init by the decoder is gone. And in case the decoder needs
ff_rl_init, too, one just needs to make sure that it is only initialized
once and that is really not onerous.

So, my answer to your original question is that it makes sense to keep
these RLTables common.

>>
>> PS: It seems you overlooked patches 7-13.
> 
> It's more that I have next to zero experience with/insight into that
> code. I don't think I can review on more than the most superficial level
> without major effort.
> 
Ok, then I'll just ping these patches.

- Andreas
Anton Khirnov Jan. 27, 2021, 9:11 a.m. UTC | #5
Quoting Andreas Rheinhardt (2021-01-23 19:50:11)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2021-01-21 21:20:52)
> >> Anton Khirnov:
> >>> Quoting Andreas Rheinhardt (2020-12-10 12:16:38)
> >>>> The H.261 decoder only uses an RLTable's VLC table, yet it also
> >>>> initializes its index_run, max_level and max_run. This commit stops
> >>>> doing so; it will also simplify making this decoder init-threadsafe,
> >>>> as the H.261 decoder and encoder now initialize disjoint parts of their
> >>>> common RLTable.
> >>>
> >>> Does it then make sense to keep this RLTable common?
> >>>
> >> I presume you want to know whether the RLTable structure should be split
> >> into smaller structures?
> > 
> > No, what I meant was whether we shouldn't use different RLTable
> > instances for encoder and decoder, since their use is disjoint. That
> > would make the code easier to reason about.
> 
> I actually didn't think that these RLTables were difficult to reason
> about: ff_rl_init and ff_rl_init_vlc/ff_init_2d_vlc_rl initialize
> different parts of an RLTable and both only use the static parts of an
> RLTable, so that these two can be called independently. In particular
> there is no clash in the case of H.261 after the unnecessary call to
> ff_rl_init by the decoder is gone. And in case the decoder needs
> ff_rl_init, too, one just needs to make sure that it is only initialized
> once and that is really not onerous.
> 
> So, my answer to your original question is that it makes sense to keep
> these RLTables common.

But then you have to remember that this is true. Someone who does not
would expect there to be one lock per object. So if you don't want to
unshare the table, a comment in the code would be useful.
diff mbox series

Patch

diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c
index 2dc2fb30f2..6b680a862d 100644
--- a/libavcodec/h261dec.c
+++ b/libavcodec/h261dec.c
@@ -81,7 +81,6 @@  static av_cold int h261_decode_init(AVCodecContext *avctx)
     s->low_delay   = 1;
     avctx->pix_fmt = AV_PIX_FMT_YUV420P;
 
-    ff_h261_common_init();
     h261_decode_init_vlc(h);
 
     h->gob_start_code_skipped = 0;