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

[Andreas Rheinhardt]

Paul B Mahol:
> On Wed, Jun 28, 2023 at 7:34 PM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> 
>> Paul B Mahol:
>>> On Wed, Jun 28, 2023 at 11:57 AM Paul B Mahol <onemda at gmail.com> wrote:
>>>
>>>>
>>>>
>>>> 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.
>>>>
>>>
>>> As already written, cant use compile time constant at all, as there are
>>> many codebooks with different size and max depth.
>>> And codebooks are picked by other parameters, and max depth differs even
>>> between same codebooks set.
>>
>> max_depth only needs to be a upper bound of the actual max_depth. You do
>> not need the real max_depth.
>>
>>> And using 3 always is not efficient and also may not work reliably all
>> the
>>> time.
>>>
>>
>> I have already told you why I believe the opposite to be true for the
>> first statement and why I don't understand your second statement at all.
>> You have not given any counterarguments to my points. Please show your
>> code.
>>
> 
> Please show your version of auto max depth calculation that can make you
> happy.
> 

As long as I see no need to calculate max_depth, no way to calculate it
will make me happy.

- Andreas