| Message ID | CAPYw7P63YNTi=4Ck9_Ef58m_evBr=UWjnxz4kRuJN8AYba6trQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vlc: auto calculate depth | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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. - Andreas
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. > > - 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". >
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. - Andreas
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. Works means that vlc code is extracted correctly. > > - 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". >
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. - Andreas
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. > > - 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". >
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? - Andreas
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? > > - 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". >
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. - Andreas
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. > > - 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". >
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. And using 3 always is not efficient and also may not work reliably all the time. > > >> >> - 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". >> >
Paul B Mahol: > 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. > 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
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. - Andreas
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. > > - 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". >
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. - Andreas
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". >
Paul B Mahol: > 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. 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 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... >
On Fri, Jun 30, 2023 at 2:35 PM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Paul B Mahol: > > 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. > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Will apply this soon as there are no more comments.
Paul B Mahol:
> Will apply this soon as there are no more comments.
My objection still stands.
- Andreas
On Mon, Jul 3, 2023 at 1:56 AM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Paul B Mahol: > > Will apply this soon as there are no more comments. > > My objection still stands. > Your objections are invalid, also you tend to object just to object. > > - 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". >
From e1ba15ab470752ebe937bc3865fb0dce99da1921 Mon Sep 17 00:00:00 2001 From: Paul B Mahol <onemda@gmail.com> Date: Sat, 24 Jun 2023 10:02:35 +0200 Subject: [PATCH] libavcodec/vlc: auto calculate depth Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/vlc.c | 9 +++++++-- libavcodec/vlc.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c index 96f2b28c7e..cfbf4717eb 100644 --- a/libavcodec/vlc.c +++ b/libavcodec/vlc.c @@ -276,7 +276,7 @@ int ff_init_vlc_sparse(VLC *vlc, int nb_bits, int nb_codes, int flags) { VLCcode localbuf[LOCALBUF_ELEMS], *buf = localbuf; - int j, ret; + int j, ret, max_len = 0; ret = vlc_common_init(vlc, nb_bits, nb_codes, &buf, flags); if (ret < 0) @@ -296,6 +296,7 @@ int ff_init_vlc_sparse(VLC *vlc, int nb_bits, int nb_codes, av_free(buf); \ return AVERROR(EINVAL); \ } \ + max_len = FFMAX(max_len, len); \ buf[j].bits = len; \ GET_DATA(buf[j].code, codes, i, codes_wrap, codes_size); \ if (buf[j].code >= (1LL<<buf[j].bits)) { \ @@ -321,6 +322,7 @@ int ff_init_vlc_sparse(VLC *vlc, int nb_bits, int nb_codes, COPY(len && len <= nb_bits); nb_codes = j; + vlc->depth = (max_len + nb_bits - 1) / nb_bits; return vlc_common_end(vlc, nb_bits, nb_codes, buf, flags, localbuf); } @@ -332,7 +334,7 @@ int ff_init_vlc_from_lengths(VLC *vlc, int nb_bits, int nb_codes, { VLCcode localbuf[LOCALBUF_ELEMS], *buf = localbuf; uint64_t code; - int ret, j, len_max = FFMIN(32, 3 * nb_bits); + int ret, j, len_max = FFMIN(32, 3 * nb_bits), max_len = 0; ret = vlc_common_init(vlc, nb_bits, nb_codes, &buf, flags); if (ret < 0) @@ -344,6 +346,7 @@ int ff_init_vlc_from_lengths(VLC *vlc, int nb_bits, int nb_codes, if (len > 0) { unsigned sym; + max_len = FFMAX(max_len, len); buf[j].bits = len; if (symbols) GET_DATA(sym, symbols, i, symbols_wrap, symbols_size) @@ -353,6 +356,7 @@ int ff_init_vlc_from_lengths(VLC *vlc, int nb_bits, int nb_codes, buf[j++].code = code; } else if (len < 0) { len = -len; + max_len = FFMAX(max_len, len); } else continue; if (len > len_max || code & ((1U << (32 - len)) - 1)) { @@ -365,6 +369,7 @@ int ff_init_vlc_from_lengths(VLC *vlc, int nb_bits, int nb_codes, goto fail; } } + vlc->depth = (max_len + nb_bits - 1) / nb_bits; return vlc_common_end(vlc, nb_bits, j, buf, flags, localbuf); fail: if (buf != localbuf) diff --git a/libavcodec/vlc.h b/libavcodec/vlc.h index e63c484755..71127b0618 100644 --- a/libavcodec/vlc.h +++ b/libavcodec/vlc.h @@ -30,6 +30,7 @@ typedef struct VLCElem { typedef struct VLC { int bits; + int depth; VLCElem *table; int table_size, table_allocated; } VLC; -- 2.39.1