[FFmpeg-devel] [PATCH 004/114] avcodec/bitstream: Allow static VLC tables to be bigger than needed

[Andreas Rheinhardt]

Paul B Mahol:
> On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
> andreas.rheinhardt at gmail.com> wrote:
> 
>> Paul B Mahol:
>>> Because it is a bad idea.
>>
>> And I still like to hear a reason for this.
>>
>>
> Code is there so correct intended size is always allocated.
> 

No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
flag set and without it.
If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
too) allocates the needed space for the VLC table (actually, it
overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
this case the whole given VLC is treated as uninitialized, i.e.
vlc->table_allocated is ignored.

If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
at least vlc->table_allocated elements and vlc->table_allocated must be
sufficient for the whole VLC: This buffer will never be (re)allocated;
it actually points to static storage and not to anything that is
heap-allocated. Given the error message that this commit intends to
remove there is currently a requirement for table_allocated to be
exactly equal to the actually required size. And this requires tables of
the sizes if VLCs are initialized in a loop (alternatively one can of
course unroll the loop and hardcode the values via one of the INIT_VLC
macros).

> 
>>> No need to change code that worked for ages.
>>>
>>
>> It allows to remove useless tables and it simplifies making more VLCs
>> that should be static static.
>>
> 
> What is static static? How does it allows to remove useless tables?
> 

And this requirement is just a pointless straitjacket. If one drops it,
one can remove the useless tables by using the pattern mentioned in the
commit message. Several commits of this series provide examples for
this; e.g. #12
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
#40
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
#53
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
#57
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html) or
#88 (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).

These are only examples where existing offsets tables were removed;
making VLCs static like in #68
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
making these VLCs static is especially beneficial for a multithreaded
decoder like this one so that the VLCs can be shared between all
threads) would be more complicated if the requirement of always using
exactly the required size were not dropped.

> 
>>
>>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
>>> andreas.rheinhardt at gmail.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> I do not think this is a good direction.
>>>>>
>>>>
>>>> Why?
>>>>
>>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
>>>>> andreas.rheinhardt at gmail.com> wrote:
>>>>>
>>>>>> Right now the allocated size of the VLC table of a static VLC has to
>>>>>> exactly match the size actually used for the VLC: If it is not enough,
>>>>>> abort is called; if it is more than enough, an error message is
>>>>>> emitted. This is no problem when one wants to initialize an individual
>>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>>>>>> size. Yet it is an obstacle when one wants to initialize several VLCs
>>>>>> in a loop as one then needs to add an array for the sizes/offsets of
>>>>>> the VLC tables (unless max_depth of all arrays is one in which case
>>>>>> the sizes are derivable from the number of bits used).
>>>>>>
>>>>>> Yet said size array is not necessary if one removes the warning for
>> too
>>>>>> big buffers. The reason is that the amount of entries needed for the
>>>>>> table is of course generated as a byproduct of initializing the VLC.
>>>>>> So one can proceed as follows:
>>>>>>
>>>>>> static VLC vlcs[NUM];
>>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>>>>>
>>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
>>>>>>     vlcs[i].table           = &vlc_table[offset];
>>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>>>>>     init_vlc();
>>>>>>     offset += vlcs[i].table_size;
>>>>>> }
>>>>>>
>>>>>> Of course, BUF_SIZE should be equal to the number of entries actually
>>>>>> needed.
>>>>>>
>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>>>> ---
>>>>>>  libavcodec/bitstream.c | 3 ---
>>>>>>  1 file changed, 3 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>>>>>> index 03d39ad88c..4ffec7e17a 100644
>>>>>> --- a/libavcodec/bitstream.c
>>>>>> +++ b/libavcodec/bitstream.c
>>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
>> int
>>>>>> nb_codes, VLCcode *codes,
>>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>>>>>
>>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>>>>>> -        if(vlc->table_size != vlc->table_allocated)
>>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>>>>>> vlc->table_size, vlc->table_allocated);
>>>>>> -
>>>>>>          av_assert0(ret >= 0);
>>>>>>          *vlc_arg = *vlc;
>>>>>>      } else {
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>
>>