* [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
@ 2023-06-26 21:57 Paul B Mahol
2023-06-27 19:48 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-26 21:57 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1: Type: text/plain, Size: 16 bytes --]
Patch attached.
[-- Attachment #2: 0001-libavcodec-vlc-auto-calculate-depth.patch --]
[-- Type: text/x-patch, Size: 3314 bytes --]
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
[-- Attachment #3: Type: text/plain, Size: 251 bytes --]
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-26 21:57 [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth Paul B Mahol
@ 2023-06-27 19:48 ` Andreas Rheinhardt
2023-06-27 20:35 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-27 19:48 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 19:48 ` Andreas Rheinhardt
@ 2023-06-27 20:35 ` Paul B Mahol
2023-06-27 20:50 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-27 20:35 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 20:35 ` Paul B Mahol
@ 2023-06-27 20:50 ` Andreas Rheinhardt
2023-06-27 21:27 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-27 20:50 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 20:50 ` Andreas Rheinhardt
@ 2023-06-27 21:27 ` Paul B Mahol
2023-06-27 21:46 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-27 21:27 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 21:27 ` Paul B Mahol
@ 2023-06-27 21:46 ` Andreas Rheinhardt
2023-06-27 22:00 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-27 21:46 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 21:46 ` Andreas Rheinhardt
@ 2023-06-27 22:00 ` Paul B Mahol
2023-06-27 22:27 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-27 22:00 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 22:00 ` Paul B Mahol
@ 2023-06-27 22:27 ` Andreas Rheinhardt
2023-06-28 6:43 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-27 22:27 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-27 22:27 ` Andreas Rheinhardt
@ 2023-06-28 6:43 ` Paul B Mahol
2023-06-28 9:12 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-28 6:43 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 6:43 ` Paul B Mahol
@ 2023-06-28 9:12 ` Andreas Rheinhardt
2023-06-28 9:57 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-28 9:12 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 9:12 ` Andreas Rheinhardt
@ 2023-06-28 9:57 ` Paul B Mahol
2023-06-28 15:16 ` Paul B Mahol
2023-06-28 16:27 ` Andreas Rheinhardt
0 siblings, 2 replies; 22+ messages in thread
From: Paul B Mahol @ 2023-06-28 9:57 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 9:57 ` Paul B Mahol
@ 2023-06-28 15:16 ` Paul B Mahol
2023-06-28 17:35 ` Andreas Rheinhardt
2023-06-28 16:27 ` Andreas Rheinhardt
1 sibling, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-28 15:16 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>>
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 15:16 ` Paul B Mahol
@ 2023-06-28 17:35 ` Andreas Rheinhardt
2023-06-29 18:56 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-28 17:35 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 17:35 ` Andreas Rheinhardt
@ 2023-06-29 18:56 ` Paul B Mahol
2023-06-29 19:06 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-29 18:56 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-29 18:56 ` Paul B Mahol
@ 2023-06-29 19:06 ` Andreas Rheinhardt
2023-06-30 11:37 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-29 19:06 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-29 19:06 ` Andreas Rheinhardt
@ 2023-06-30 11:37 ` Paul B Mahol
2023-06-30 12:36 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-30 11:37 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-30 11:37 ` Paul B Mahol
@ 2023-06-30 12:36 ` Andreas Rheinhardt
2023-06-30 17:41 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-30 12:36 UTC (permalink / raw)
To: ffmpeg-devel
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...
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-30 12:36 ` Andreas Rheinhardt
@ 2023-06-30 17:41 ` Paul B Mahol
2023-07-02 9:16 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-06-30 17:41 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-30 17:41 ` Paul B Mahol
@ 2023-07-02 9:16 ` Paul B Mahol
2023-07-02 23:57 ` Andreas Rheinhardt
0 siblings, 1 reply; 22+ messages in thread
From: Paul B Mahol @ 2023-07-02 9:16 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Will apply this soon as there are no more comments.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-07-02 9:16 ` Paul B Mahol
@ 2023-07-02 23:57 ` Andreas Rheinhardt
2023-07-03 11:22 ` Paul B Mahol
0 siblings, 1 reply; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-07-02 23:57 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> Will apply this soon as there are no more comments.
My objection still stands.
- 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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-07-02 23:57 ` Andreas Rheinhardt
@ 2023-07-03 11:22 ` Paul B Mahol
0 siblings, 0 replies; 22+ messages in thread
From: Paul B Mahol @ 2023-07-03 11:22 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth
2023-06-28 9:57 ` Paul B Mahol
2023-06-28 15:16 ` Paul B Mahol
@ 2023-06-28 16:27 ` Andreas Rheinhardt
1 sibling, 0 replies; 22+ messages in thread
From: Andreas Rheinhardt @ 2023-06-28 16:27 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-07-03 11:15 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26 21:57 [FFmpeg-devel] [PATCH] avcodec/vlc: auto calculate depth Paul B Mahol
2023-06-27 19:48 ` Andreas Rheinhardt
2023-06-27 20:35 ` Paul B Mahol
2023-06-27 20:50 ` Andreas Rheinhardt
2023-06-27 21:27 ` Paul B Mahol
2023-06-27 21:46 ` Andreas Rheinhardt
2023-06-27 22:00 ` Paul B Mahol
2023-06-27 22:27 ` Andreas Rheinhardt
2023-06-28 6:43 ` Paul B Mahol
2023-06-28 9:12 ` Andreas Rheinhardt
2023-06-28 9:57 ` Paul B Mahol
2023-06-28 15:16 ` Paul B Mahol
2023-06-28 17:35 ` Andreas Rheinhardt
2023-06-29 18:56 ` Paul B Mahol
2023-06-29 19:06 ` Andreas Rheinhardt
2023-06-30 11:37 ` Paul B Mahol
2023-06-30 12:36 ` Andreas Rheinhardt
2023-06-30 17:41 ` Paul B Mahol
2023-07-02 9:16 ` Paul B Mahol
2023-07-02 23:57 ` Andreas Rheinhardt
2023-07-03 11:22 ` Paul B Mahol
2023-06-28 16:27 ` Andreas Rheinhardt
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git