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