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

[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.
And using 3 always is not efficient and also may not work reliably all the
time.


>
>
>>
>> - Andreas
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>