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

[Paul B Mahol <onemda@gmail.com>]

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.
Also 3 may not always work, depending on vlc specification.

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

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



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