[FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
Paul B Mahol
onemda at gmail.com
Thu Jun 29 21:56:29 EEST 2023
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.
>
> - 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".
>
More information about the ffmpeg-devel
mailing list