[FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
Paul B Mahol
onemda at gmail.com
Fri Jun 30 20:41:51 EEST 2023
On Fri, Jun 30, 2023 at 2:35 PM Andreas Rheinhardt <
andreas.rheinhardt at outlook.com> wrote:
> Paul B Mahol:
> > On Thu, Jun 29, 2023 at 9:20 PM Andreas Rheinhardt <
> > andreas.rheinhardt at outlook.com> wrote:
> >
> >> 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.
> >>
> >
> > 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 looked at code and have not provided alternative solution to the
problem.
>
> > 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 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".
>
More information about the ffmpeg-devel
mailing list