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

[Andreas Rheinhardt]

Paul B Mahol:
> Than how one is supposed to guess correct size to put when this log is gone?
> 
The developer who wants to make VLCs static just has to use a huge array
(in order not to run into the abort) and get the value of the final
offset parameter (either via av_log or via a debugger); or you just sum
the VLC.table_size values before you make the VLCs static. It is no big
deal and it is not much different from what it is now.

> On Tue, Nov 10, 2020 at 1:15 PM Andreas Rheinhardt <
> andreas.rheinhardt at gmail.com> wrote:
> 
>> 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
>>>>>>>>
>>>>>>
>>>>