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

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


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