Paul B Mahol: > On Wed, Jun 28, 2023 at 11:11 AM Andreas Rheinhardt < > [email protected]> wrote: > >> Paul B Mahol: >>> On Wed, Jun 28, 2023 at 12:26 AM Andreas Rheinhardt < >>> [email protected]> wrote: >>> >>>> Paul B Mahol: >>>>> On Tue, Jun 27, 2023 at 11:45 PM Andreas Rheinhardt < >>>>> [email protected]> wrote: >>>>> >>>>>> Paul B Mahol: >>>>>>> On Tue, Jun 27, 2023 at 11:05 PM Andreas Rheinhardt < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Paul B Mahol: >>>>>>>>> On Tue, Jun 27, 2023 at 9:47 PM Andreas Rheinhardt < >>>>>>>>> [email protected]> 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. >
1. If nothing uses this yet, then it can wait until you post the patches that rely on this. 2. A better way to achieve the same aim would be to record the maximum recursion depth in build_table() instead of adding code to the loops that iterate over every element. - Andreas _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
