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

Tue Nov 10 14:38:42 EET 2020

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.
> 

I forgot to mention that this last statement only applies to finding the
total size of the buffer. But as long as this log statement is there,
this is not enough: One really needs all the sizes (or offsets as one
prefers) which is a bit more work (and leads to some unnecessary code
and unnecessary tables).

>> 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
>>>>>>>>>
>>>>>>>
>>>>>