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

[Anton Khirnov]

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.

-- 
Anton Khirnov