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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Paul B Mahol:
> On Thu, Jun 29, 2023 at 9:20 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
> 
>> Paul B Mahol:
>>> On Wed, Jun 28, 2023 at 7:34 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Wed, Jun 28, 2023 at 11:57 AM Paul B Mahol <onemda@gmail.com>
>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jun 28, 2023 at 11:11 AM Andreas Rheinhardt <
>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>
>>>>>>> Paul B Mahol:
>>>>>>>> On Wed, Jun 28, 2023 at 12:26 AM Andreas Rheinhardt <
>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>
>>>>>>>>> Paul B Mahol:
>>>>>>>>>> On Tue, Jun 27, 2023 at 11:45 PM Andreas Rheinhardt <
>>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>>> On Tue, Jun 27, 2023 at 11:05 PM Andreas Rheinhardt <
>>>>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>>>>> On Tue, Jun 27, 2023 at 9:47 PM Andreas Rheinhardt <
>>>>>>>>>>>>>> andreas.rheinhardt@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.
>>
> 
> But you already said  that using max_depth or 3 is sub-optimal.

It is sub-optimal if you have a better compile-time upper-bound.
Otherwise, it is not sub-optimal.

> Also 3 may not always work, depending on vlc specification.
> 

We only use VLCs with a depth of at most three.

> Your approach to this bug and unwillingness to cooperate is very bad for
> project.
> 

Up until now you have not shown anything, merely asserted the existence
of a bug.

> You can find code on github, it deals with ac4 decoder. And have many
> codebooks
> which are picked by other parameters at runtime. Thus hardcoding max_depth
> is not possible.
> Thus I though about adding generic support for this to benefit all.
> 
> But as your lack of understanding of overall problem I guess both you and
> project do not need
> ac4 decoder and/or this fix.
> 
> Have a nice riddance...
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".