[FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth

[Andreas Rheinhardt]

Paul B Mahol:
> On Wed, Jun 28, 2023 at 11:11 AM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> 
>> Paul B Mahol:
>>> On Wed, Jun 28, 2023 at 12:26 AM Andreas Rheinhardt <
>>> andreas.rheinhardt at outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Tue, Jun 27, 2023 at 11:45 PM Andreas Rheinhardt <
>>>>> andreas.rheinhardt at outlook.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>> On Tue, Jun 27, 2023 at 11:05 PM Andreas Rheinhardt <
>>>>>>> andreas.rheinhardt at outlook.com> wrote:
>>>>>>>
>>>>>>>> Paul B Mahol:
>>>>>>>>> On Tue, Jun 27, 2023 at 9:47 PM Andreas Rheinhardt <
>>>>>>>>> andreas.rheinhardt at outlook.com> wrote:
>>>>>>>>>
>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>> Patch attached.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Where do you intend to use this? What is the point of it?
>>>>>>>>>> After all, using this value in GET_VLC makes no sense; only
>>>>>> compile-time
>>>>>>>>>> constants do.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It works when used in ac-4 as get_vlc2.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Could you please define "works"? Using a non-compile-time constant
>>>> will
>>>>>>>> not benefit at all; it will only lead to more runtime checks.
>>>>>>>>
>>>>>>>
>>>>>>> I do not follow your worries.
>>>>>>> I can not use compile time constant as its very complicated code.
>>>>>>>
>>>>>>
>>>>>> Let's take a look at GET_VLC:
>>>>>> #define GET_VLC(code, name, gb, table, bits, max_depth)         \
>>>>>>     do {                                                        \
>>>>>>         int n, nb_bits;                                         \
>>>>>>         unsigned int index;                                     \
>>>>>>                                                                 \
>>>>>>         index = SHOW_UBITS(name, gb, bits);                     \
>>>>>>         code  = table[index].sym;                               \
>>>>>>         n     = table[index].len;                               \
>>>>>>                                                                 \
>>>>>>         if (max_depth > 1 && n < 0) {                           \
>>>>>>             LAST_SKIP_BITS(name, gb, bits);                     \
>>>>>>             UPDATE_CACHE(name, gb);                             \
>>>>>>                                                                 \
>>>>>>             nb_bits = -n;                                       \
>>>>>>                                                                 \
>>>>>>             index = SHOW_UBITS(name, gb, nb_bits) + code;       \
>>>>>>             code  = table[index].sym;                           \
>>>>>>             n     = table[index].len;                           \
>>>>>>             if (max_depth > 2 && n < 0) {                       \
>>>>>>                 LAST_SKIP_BITS(name, gb, nb_bits);              \
>>>>>>                 UPDATE_CACHE(name, gb);                         \
>>>>>>                                                                 \
>>>>>>                 nb_bits = -n;                                   \
>>>>>>                                                                 \
>>>>>>                 index = SHOW_UBITS(name, gb, nb_bits) + code;   \
>>>>>>                 code  = table[index].sym;                       \
>>>>>>                 n     = table[index].len;                       \
>>>>>>             }                                                   \
>>>>>>         }                                                       \
>>>>>>         SKIP_BITS(name, gb, n);                                 \
>>>>>>     } while (0)
>>>>>>
>>>>>> If max_depth is not a compile-time constant, then the compiler will
>> have
>>>>>> to perform both of the max_depth > 1 && n < 0 checks; yet, this is not
>>>>>> useful: If the depth of a particular VLC is (say) 1, then none of the
>>>>>> possible bits read will lead to reloading at all, because the n < 0
>>>>>> condition will never be true; the only reason this condition exists is
>>>>>> to use a compile-time upper bound, so that one can eliminate the
>> reload
>>>>>> code (and in particular, avoid the runtime checks).
>>>>>>
>>>>>>> Works means that vlc code is extracted correctly.
>>>>>>>
>>>>>>
>>>>>> If you have no upper bound about max_depth and it works, then use 3.
>>>>>>
>>>>>
>>>>> It does not work to use 3 all the time. And that one never worked in
>> any
>>>>> codec so far.
>>>>>
>>>>
>>>> I just ran FATE with the check for max_depth removed from GET_VLC and
>>>> from read_vlc for the cached API (effectively setting max_depth to 3
>>>> everywhere). It passed. So it "works" (but in a suboptimal way). At
>>>> least it does if you have ordinary VLCs (created by the vlc.c
>>>> functions). Are you doing anything special with them or so?
>>>>
>>>
>>> FATE code coverage is very limited.
>>>
>>> Also I do not follow your reasoning about this added field at all.
>>>
>>> What is calculated over and over again in each get_vlc2() call?
>>>
>>
>> Nothing is calculated over and over again in each get_vlc2() call; but
>> if you use a non-compile-time constant, then the check for max_depth is
>> performed in each get_vlc2() call, even though it is unnecessary.
>>
> 
> So what?
> Nothing use this yet. So it does not matter.
> 

1. If nothing uses this yet, then it can wait until you post the patches
that rely on this.
2. A better way to achieve the same aim would be to record the maximum
recursion depth in build_table() instead of adding code to the loops
that iterate over every element.

- Andreas