diff mbox series

[FFmpeg-devel,004/114] avcodec/bitstream: Allow static VLC tables to be bigger than needed

Message ID 20201110104851.321029-5-andreas.rheinhardt@gmail.com
State Superseded
Headers show
Series VLC, esp. init_vlc patches
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 10, 2020, 10:47 a.m. UTC
Right now the allocated size of the VLC table of a static VLC has to
exactly match the size actually used for the VLC: If it is not enough,
abort is called; if it is more than enough, an error message is
emitted. This is no problem when one wants to initialize an individual
VLC via one of the INIT_VLC macros as one just hardcodes the needed
size. Yet it is an obstacle when one wants to initialize several VLCs
in a loop as one then needs to add an array for the sizes/offsets of
the VLC tables (unless max_depth of all arrays is one in which case
the sizes are derivable from the number of bits used).

Yet said size array is not necessary if one removes the warning for too
big buffers. The reason is that the amount of entries needed for the
table is of course generated as a byproduct of initializing the VLC.
So one can proceed as follows:

static VLC vlcs[NUM];
static VLC_TYPE vlc_table[BUF_SIZE][2];

for (int i = 0, offset = 0; i < NUM; i++) {
    vlcs[i].table           = &vlc_table[offset];
    vlcs[i].table_allocated = BUF_SIZE - offset;
    init_vlc();
    offset += vlcs[i].table_size;
}

Of course, BUF_SIZE should be equal to the number of entries actually
needed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/bitstream.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Paul B Mahol Nov. 10, 2020, 11:24 a.m. UTC | #1
I do not think this is a good direction.

On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Right now the allocated size of the VLC table of a static VLC has to
> exactly match the size actually used for the VLC: If it is not enough,
> abort is called; if it is more than enough, an error message is
> emitted. This is no problem when one wants to initialize an individual
> VLC via one of the INIT_VLC macros as one just hardcodes the needed
> size. Yet it is an obstacle when one wants to initialize several VLCs
> in a loop as one then needs to add an array for the sizes/offsets of
> the VLC tables (unless max_depth of all arrays is one in which case
> the sizes are derivable from the number of bits used).
>
> Yet said size array is not necessary if one removes the warning for too
> big buffers. The reason is that the amount of entries needed for the
> table is of course generated as a byproduct of initializing the VLC.
> So one can proceed as follows:
>
> static VLC vlcs[NUM];
> static VLC_TYPE vlc_table[BUF_SIZE][2];
>
> for (int i = 0, offset = 0; i < NUM; i++) {
>     vlcs[i].table           = &vlc_table[offset];
>     vlcs[i].table_allocated = BUF_SIZE - offset;
>     init_vlc();
>     offset += vlcs[i].table_size;
> }
>
> Of course, BUF_SIZE should be equal to the number of entries actually
> needed.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/bitstream.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> index 03d39ad88c..4ffec7e17a 100644
> --- a/libavcodec/bitstream.c
> +++ b/libavcodec/bitstream.c
> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits, int
> nb_codes, VLCcode *codes,
>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>
>      if (flags & INIT_VLC_USE_NEW_STATIC) {
> -        if(vlc->table_size != vlc->table_allocated)
> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
> vlc->table_size, vlc->table_allocated);
> -
>          av_assert0(ret >= 0);
>          *vlc_arg = *vlc;
>      } else {
> --
> 2.25.1
>
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 10, 2020, 11:26 a.m. UTC | #2
Paul B Mahol:
> I do not think this is a good direction.
> 

Why?

> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Right now the allocated size of the VLC table of a static VLC has to
>> exactly match the size actually used for the VLC: If it is not enough,
>> abort is called; if it is more than enough, an error message is
>> emitted. This is no problem when one wants to initialize an individual
>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>> size. Yet it is an obstacle when one wants to initialize several VLCs
>> in a loop as one then needs to add an array for the sizes/offsets of
>> the VLC tables (unless max_depth of all arrays is one in which case
>> the sizes are derivable from the number of bits used).
>>
>> Yet said size array is not necessary if one removes the warning for too
>> big buffers. The reason is that the amount of entries needed for the
>> table is of course generated as a byproduct of initializing the VLC.
>> So one can proceed as follows:
>>
>> static VLC vlcs[NUM];
>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>
>> for (int i = 0, offset = 0; i < NUM; i++) {
>>     vlcs[i].table           = &vlc_table[offset];
>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>     init_vlc();
>>     offset += vlcs[i].table_size;
>> }
>>
>> Of course, BUF_SIZE should be equal to the number of entries actually
>> needed.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/bitstream.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>> index 03d39ad88c..4ffec7e17a 100644
>> --- a/libavcodec/bitstream.c
>> +++ b/libavcodec/bitstream.c
>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits, int
>> nb_codes, VLCcode *codes,
>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>
>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>> -        if(vlc->table_size != vlc->table_allocated)
>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>> vlc->table_size, vlc->table_allocated);
>> -
>>          av_assert0(ret >= 0);
>>          *vlc_arg = *vlc;
>>      } else {
>> --
>> 2.25.1
>>
Paul B Mahol Nov. 10, 2020, 11:28 a.m. UTC | #3
Because it is a bad idea.
No need to change code that worked for ages.

On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Paul B Mahol:
> > I do not think this is a good direction.
> >
>
> Why?
>
> > On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
> >
> >> Right now the allocated size of the VLC table of a static VLC has to
> >> exactly match the size actually used for the VLC: If it is not enough,
> >> abort is called; if it is more than enough, an error message is
> >> emitted. This is no problem when one wants to initialize an individual
> >> VLC via one of the INIT_VLC macros as one just hardcodes the needed
> >> size. Yet it is an obstacle when one wants to initialize several VLCs
> >> in a loop as one then needs to add an array for the sizes/offsets of
> >> the VLC tables (unless max_depth of all arrays is one in which case
> >> the sizes are derivable from the number of bits used).
> >>
> >> Yet said size array is not necessary if one removes the warning for too
> >> big buffers. The reason is that the amount of entries needed for the
> >> table is of course generated as a byproduct of initializing the VLC.
> >> So one can proceed as follows:
> >>
> >> static VLC vlcs[NUM];
> >> static VLC_TYPE vlc_table[BUF_SIZE][2];
> >>
> >> for (int i = 0, offset = 0; i < NUM; i++) {
> >>     vlcs[i].table           = &vlc_table[offset];
> >>     vlcs[i].table_allocated = BUF_SIZE - offset;
> >>     init_vlc();
> >>     offset += vlcs[i].table_size;
> >> }
> >>
> >> Of course, BUF_SIZE should be equal to the number of entries actually
> >> needed.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >> ---
> >>  libavcodec/bitstream.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> >> index 03d39ad88c..4ffec7e17a 100644
> >> --- a/libavcodec/bitstream.c
> >> +++ b/libavcodec/bitstream.c
> >> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits, int
> >> nb_codes, VLCcode *codes,
> >>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
> >>
> >>      if (flags & INIT_VLC_USE_NEW_STATIC) {
> >> -        if(vlc->table_size != vlc->table_allocated)
> >> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
> >> vlc->table_size, vlc->table_allocated);
> >> -
> >>          av_assert0(ret >= 0);
> >>          *vlc_arg = *vlc;
> >>      } else {
> >> --
> >> 2.25.1
> >>
>
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 10, 2020, 11:30 a.m. UTC | #4
Paul B Mahol:
> Because it is a bad idea.

And I still like to hear a reason for this.

> No need to change code that worked for ages.
> 

It allows to remove useless tables and it simplifies making more VLCs
that should be static static.

> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Paul B Mahol:
>>> I do not think this is a good direction.
>>>
>>
>> Why?
>>
>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
>>> andreas.rheinhardt@gmail.com> wrote:
>>>
>>>> Right now the allocated size of the VLC table of a static VLC has to
>>>> exactly match the size actually used for the VLC: If it is not enough,
>>>> abort is called; if it is more than enough, an error message is
>>>> emitted. This is no problem when one wants to initialize an individual
>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>>>> size. Yet it is an obstacle when one wants to initialize several VLCs
>>>> in a loop as one then needs to add an array for the sizes/offsets of
>>>> the VLC tables (unless max_depth of all arrays is one in which case
>>>> the sizes are derivable from the number of bits used).
>>>>
>>>> Yet said size array is not necessary if one removes the warning for too
>>>> big buffers. The reason is that the amount of entries needed for the
>>>> table is of course generated as a byproduct of initializing the VLC.
>>>> So one can proceed as follows:
>>>>
>>>> static VLC vlcs[NUM];
>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>>>
>>>> for (int i = 0, offset = 0; i < NUM; i++) {
>>>>     vlcs[i].table           = &vlc_table[offset];
>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>>>     init_vlc();
>>>>     offset += vlcs[i].table_size;
>>>> }
>>>>
>>>> Of course, BUF_SIZE should be equal to the number of entries actually
>>>> needed.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>> ---
>>>>  libavcodec/bitstream.c | 3 ---
>>>>  1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>>>> index 03d39ad88c..4ffec7e17a 100644
>>>> --- a/libavcodec/bitstream.c
>>>> +++ b/libavcodec/bitstream.c
>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits, int
>>>> nb_codes, VLCcode *codes,
>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>>>
>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>>>> -        if(vlc->table_size != vlc->table_allocated)
>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>>>> vlc->table_size, vlc->table_allocated);
>>>> -
>>>>          av_assert0(ret >= 0);
>>>>          *vlc_arg = *vlc;
>>>>      } else {
>>>> --
>>>> 2.25.1
>>>>
>>
Paul B Mahol Nov. 10, 2020, 11:36 a.m. UTC | #5
On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Paul B Mahol:
> > Because it is a bad idea.
>
> And I still like to hear a reason for this.
>
>
Code is there so correct intended size is always allocated.


> > No need to change code that worked for ages.
> >
>
> It allows to remove useless tables and it simplifies making more VLCs
> that should be static static.
>

What is static static? How does it allows to remove useless tables?


>
> > On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
> >
> >> Paul B Mahol:
> >>> I do not think this is a good direction.
> >>>
> >>
> >> Why?
> >>
> >>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
> >>> andreas.rheinhardt@gmail.com> wrote:
> >>>
> >>>> Right now the allocated size of the VLC table of a static VLC has to
> >>>> exactly match the size actually used for the VLC: If it is not enough,
> >>>> abort is called; if it is more than enough, an error message is
> >>>> emitted. This is no problem when one wants to initialize an individual
> >>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
> >>>> size. Yet it is an obstacle when one wants to initialize several VLCs
> >>>> in a loop as one then needs to add an array for the sizes/offsets of
> >>>> the VLC tables (unless max_depth of all arrays is one in which case
> >>>> the sizes are derivable from the number of bits used).
> >>>>
> >>>> Yet said size array is not necessary if one removes the warning for
> too
> >>>> big buffers. The reason is that the amount of entries needed for the
> >>>> table is of course generated as a byproduct of initializing the VLC.
> >>>> So one can proceed as follows:
> >>>>
> >>>> static VLC vlcs[NUM];
> >>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
> >>>>
> >>>> for (int i = 0, offset = 0; i < NUM; i++) {
> >>>>     vlcs[i].table           = &vlc_table[offset];
> >>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
> >>>>     init_vlc();
> >>>>     offset += vlcs[i].table_size;
> >>>> }
> >>>>
> >>>> Of course, BUF_SIZE should be equal to the number of entries actually
> >>>> needed.
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>> ---
> >>>>  libavcodec/bitstream.c | 3 ---
> >>>>  1 file changed, 3 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> >>>> index 03d39ad88c..4ffec7e17a 100644
> >>>> --- a/libavcodec/bitstream.c
> >>>> +++ b/libavcodec/bitstream.c
> >>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
> int
> >>>> nb_codes, VLCcode *codes,
> >>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
> >>>>
> >>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
> >>>> -        if(vlc->table_size != vlc->table_allocated)
> >>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
> >>>> vlc->table_size, vlc->table_allocated);
> >>>> -
> >>>>          av_assert0(ret >= 0);
> >>>>          *vlc_arg = *vlc;
> >>>>      } else {
> >>>> --
> >>>> 2.25.1
> >>>>
> >>
>
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 10, 2020, 12:14 p.m. UTC | #6
Paul B Mahol:
> On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Paul B Mahol:
>>> Because it is a bad idea.
>>
>> And I still like to hear a reason for this.
>>
>>
> Code is there so correct intended size is always allocated.
> 

No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
flag set and without it.
If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
too) allocates the needed space for the VLC table (actually, it
overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
this case the whole given VLC is treated as uninitialized, i.e.
vlc->table_allocated is ignored.

If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
at least vlc->table_allocated elements and vlc->table_allocated must be
sufficient for the whole VLC: This buffer will never be (re)allocated;
it actually points to static storage and not to anything that is
heap-allocated. Given the error message that this commit intends to
remove there is currently a requirement for table_allocated to be
exactly equal to the actually required size. And this requires tables of
the sizes if VLCs are initialized in a loop (alternatively one can of
course unroll the loop and hardcode the values via one of the INIT_VLC
macros).

> 
>>> No need to change code that worked for ages.
>>>
>>
>> It allows to remove useless tables and it simplifies making more VLCs
>> that should be static static.
>>
> 
> What is static static? How does it allows to remove useless tables?
> 

And this requirement is just a pointless straitjacket. If one drops it,
one can remove the useless tables by using the pattern mentioned in the
commit message. Several commits of this series provide examples for
this; e.g. #12
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
#40
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
#53
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
#57
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html) or
#88 (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).

These are only examples where existing offsets tables were removed;
making VLCs static like in #68
(https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
making these VLCs static is especially beneficial for a multithreaded
decoder like this one so that the VLCs can be shared between all
threads) would be more complicated if the requirement of always using
exactly the required size were not dropped.

> 
>>
>>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@gmail.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> I do not think this is a good direction.
>>>>>
>>>>
>>>> Why?
>>>>
>>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>>
>>>>>> Right now the allocated size of the VLC table of a static VLC has to
>>>>>> exactly match the size actually used for the VLC: If it is not enough,
>>>>>> abort is called; if it is more than enough, an error message is
>>>>>> emitted. This is no problem when one wants to initialize an individual
>>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>>>>>> size. Yet it is an obstacle when one wants to initialize several VLCs
>>>>>> in a loop as one then needs to add an array for the sizes/offsets of
>>>>>> the VLC tables (unless max_depth of all arrays is one in which case
>>>>>> the sizes are derivable from the number of bits used).
>>>>>>
>>>>>> Yet said size array is not necessary if one removes the warning for
>> too
>>>>>> big buffers. The reason is that the amount of entries needed for the
>>>>>> table is of course generated as a byproduct of initializing the VLC.
>>>>>> So one can proceed as follows:
>>>>>>
>>>>>> static VLC vlcs[NUM];
>>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>>>>>
>>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
>>>>>>     vlcs[i].table           = &vlc_table[offset];
>>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>>>>>     init_vlc();
>>>>>>     offset += vlcs[i].table_size;
>>>>>> }
>>>>>>
>>>>>> Of course, BUF_SIZE should be equal to the number of entries actually
>>>>>> needed.
>>>>>>
>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>>>> ---
>>>>>>  libavcodec/bitstream.c | 3 ---
>>>>>>  1 file changed, 3 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>>>>>> index 03d39ad88c..4ffec7e17a 100644
>>>>>> --- a/libavcodec/bitstream.c
>>>>>> +++ b/libavcodec/bitstream.c
>>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
>> int
>>>>>> nb_codes, VLCcode *codes,
>>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>>>>>
>>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>>>>>> -        if(vlc->table_size != vlc->table_allocated)
>>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>>>>>> vlc->table_size, vlc->table_allocated);
>>>>>> -
>>>>>>          av_assert0(ret >= 0);
>>>>>>          *vlc_arg = *vlc;
>>>>>>      } else {
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>
>>
Paul B Mahol Nov. 10, 2020, 12:23 p.m. UTC | #7
Than how one is supposed to guess correct size to put when this log is gone?

On Tue, Nov 10, 2020 at 1:15 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Paul B Mahol:
> > On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
> >
> >> Paul B Mahol:
> >>> Because it is a bad idea.
> >>
> >> And I still like to hear a reason for this.
> >>
> >>
> > Code is there so correct intended size is always allocated.
> >
>
> No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
> flag set and without it.
> If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
> too) allocates the needed space for the VLC table (actually, it
> overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
> this case the whole given VLC is treated as uninitialized, i.e.
> vlc->table_allocated is ignored.
>
> If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
> at least vlc->table_allocated elements and vlc->table_allocated must be
> sufficient for the whole VLC: This buffer will never be (re)allocated;
> it actually points to static storage and not to anything that is
> heap-allocated. Given the error message that this commit intends to
> remove there is currently a requirement for table_allocated to be
> exactly equal to the actually required size. And this requires tables of
> the sizes if VLCs are initialized in a loop (alternatively one can of
> course unroll the loop and hardcode the values via one of the INIT_VLC
> macros).
>
> >
> >>> No need to change code that worked for ages.
> >>>
> >>
> >> It allows to remove useless tables and it simplifies making more VLCs
> >> that should be static static.
> >>
> >
> > What is static static? How does it allows to remove useless tables?
> >
>
> And this requirement is just a pointless straitjacket. If one drops it,
> one can remove the useless tables by using the pattern mentioned in the
> commit message. Several commits of this series provide examples for
> this; e.g. #12
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
> #40
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
> #53
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
> #57
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html) or
> #88 (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).
>
> These are only examples where existing offsets tables were removed;
> making VLCs static like in #68
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
> making these VLCs static is especially beneficial for a multithreaded
> decoder like this one so that the VLCs can be shared between all
> threads) would be more complicated if the requirement of always using
> exactly the required size were not dropped.
>
> >
> >>
> >>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@gmail.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> I do not think this is a good direction.
> >>>>>
> >>>>
> >>>> Why?
> >>>>
> >>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@gmail.com> wrote:
> >>>>>
> >>>>>> Right now the allocated size of the VLC table of a static VLC has to
> >>>>>> exactly match the size actually used for the VLC: If it is not
> enough,
> >>>>>> abort is called; if it is more than enough, an error message is
> >>>>>> emitted. This is no problem when one wants to initialize an
> individual
> >>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
> >>>>>> size. Yet it is an obstacle when one wants to initialize several
> VLCs
> >>>>>> in a loop as one then needs to add an array for the sizes/offsets of
> >>>>>> the VLC tables (unless max_depth of all arrays is one in which case
> >>>>>> the sizes are derivable from the number of bits used).
> >>>>>>
> >>>>>> Yet said size array is not necessary if one removes the warning for
> >> too
> >>>>>> big buffers. The reason is that the amount of entries needed for the
> >>>>>> table is of course generated as a byproduct of initializing the VLC.
> >>>>>> So one can proceed as follows:
> >>>>>>
> >>>>>> static VLC vlcs[NUM];
> >>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
> >>>>>>
> >>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
> >>>>>>     vlcs[i].table           = &vlc_table[offset];
> >>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
> >>>>>>     init_vlc();
> >>>>>>     offset += vlcs[i].table_size;
> >>>>>> }
> >>>>>>
> >>>>>> Of course, BUF_SIZE should be equal to the number of entries
> actually
> >>>>>> needed.
> >>>>>>
> >>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>>>> ---
> >>>>>>  libavcodec/bitstream.c | 3 ---
> >>>>>>  1 file changed, 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> >>>>>> index 03d39ad88c..4ffec7e17a 100644
> >>>>>> --- a/libavcodec/bitstream.c
> >>>>>> +++ b/libavcodec/bitstream.c
> >>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
> >> int
> >>>>>> nb_codes, VLCcode *codes,
> >>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
> >>>>>>
> >>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
> >>>>>> -        if(vlc->table_size != vlc->table_allocated)
> >>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
> >>>>>> vlc->table_size, vlc->table_allocated);
> >>>>>> -
> >>>>>>          av_assert0(ret >= 0);
> >>>>>>          *vlc_arg = *vlc;
> >>>>>>      } else {
> >>>>>> --
> >>>>>> 2.25.1
> >>>>>>
> >>>>
> >>
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 10, 2020, 12:30 p.m. UTC | #8
Paul B Mahol:
> Than how one is supposed to guess correct size to put when this log is gone?
> 
The developer who wants to make VLCs static just has to use a huge array
(in order not to run into the abort) and get the value of the final
offset parameter (either via av_log or via a debugger); or you just sum
the VLC.table_size values before you make the VLCs static. It is no big
deal and it is not much different from what it is now.

> On Tue, Nov 10, 2020 at 1:15 PM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Paul B Mahol:
>>> On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@gmail.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> Because it is a bad idea.
>>>>
>>>> And I still like to hear a reason for this.
>>>>
>>>>
>>> Code is there so correct intended size is always allocated.
>>>
>>
>> No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
>> flag set and without it.
>> If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
>> too) allocates the needed space for the VLC table (actually, it
>> overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
>> this case the whole given VLC is treated as uninitialized, i.e.
>> vlc->table_allocated is ignored.
>>
>> If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
>> at least vlc->table_allocated elements and vlc->table_allocated must be
>> sufficient for the whole VLC: This buffer will never be (re)allocated;
>> it actually points to static storage and not to anything that is
>> heap-allocated. Given the error message that this commit intends to
>> remove there is currently a requirement for table_allocated to be
>> exactly equal to the actually required size. And this requires tables of
>> the sizes if VLCs are initialized in a loop (alternatively one can of
>> course unroll the loop and hardcode the values via one of the INIT_VLC
>> macros).
>>
>>>
>>>>> No need to change code that worked for ages.
>>>>>
>>>>
>>>> It allows to remove useless tables and it simplifies making more VLCs
>>>> that should be static static.
>>>>
>>>
>>> What is static static? How does it allows to remove useless tables?
>>>
>>
>> And this requirement is just a pointless straitjacket. If one drops it,
>> one can remove the useless tables by using the pattern mentioned in the
>> commit message. Several commits of this series provide examples for
>> this; e.g. #12
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
>> #40
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
>> #53
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
>> #57
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html) or
>> #88 (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).
>>
>> These are only examples where existing offsets tables were removed;
>> making VLCs static like in #68
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
>> making these VLCs static is especially beneficial for a multithreaded
>> decoder like this one so that the VLCs can be shared between all
>> threads) would be more complicated if the requirement of always using
>> exactly the required size were not dropped.
>>
>>>
>>>>
>>>>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>> I do not think this is a good direction.
>>>>>>>
>>>>>>
>>>>>> Why?
>>>>>>
>>>>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
>>>>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>>>>
>>>>>>>> Right now the allocated size of the VLC table of a static VLC has to
>>>>>>>> exactly match the size actually used for the VLC: If it is not
>> enough,
>>>>>>>> abort is called; if it is more than enough, an error message is
>>>>>>>> emitted. This is no problem when one wants to initialize an
>> individual
>>>>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>>>>>>>> size. Yet it is an obstacle when one wants to initialize several
>> VLCs
>>>>>>>> in a loop as one then needs to add an array for the sizes/offsets of
>>>>>>>> the VLC tables (unless max_depth of all arrays is one in which case
>>>>>>>> the sizes are derivable from the number of bits used).
>>>>>>>>
>>>>>>>> Yet said size array is not necessary if one removes the warning for
>>>> too
>>>>>>>> big buffers. The reason is that the amount of entries needed for the
>>>>>>>> table is of course generated as a byproduct of initializing the VLC.
>>>>>>>> So one can proceed as follows:
>>>>>>>>
>>>>>>>> static VLC vlcs[NUM];
>>>>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>>>>>>>
>>>>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
>>>>>>>>     vlcs[i].table           = &vlc_table[offset];
>>>>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>>>>>>>     init_vlc();
>>>>>>>>     offset += vlcs[i].table_size;
>>>>>>>> }
>>>>>>>>
>>>>>>>> Of course, BUF_SIZE should be equal to the number of entries
>> actually
>>>>>>>> needed.
>>>>>>>>
>>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>>>>>> ---
>>>>>>>>  libavcodec/bitstream.c | 3 ---
>>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>>>>>>>> index 03d39ad88c..4ffec7e17a 100644
>>>>>>>> --- a/libavcodec/bitstream.c
>>>>>>>> +++ b/libavcodec/bitstream.c
>>>>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
>>>> int
>>>>>>>> nb_codes, VLCcode *codes,
>>>>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>>>>>>>
>>>>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>>>>>>>> -        if(vlc->table_size != vlc->table_allocated)
>>>>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>>>>>>>> vlc->table_size, vlc->table_allocated);
>>>>>>>> -
>>>>>>>>          av_assert0(ret >= 0);
>>>>>>>>          *vlc_arg = *vlc;
>>>>>>>>      } else {
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>
>>>>
Paul B Mahol Nov. 10, 2020, 12:37 p.m. UTC | #9
Big disagree, please keep this log intact.
Thanks.

On Tue, Nov 10, 2020 at 1:31 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Paul B Mahol:
> > Than how one is supposed to guess correct size to put when this log is
> gone?
> >
> The developer who wants to make VLCs static just has to use a huge array
> (in order not to run into the abort) and get the value of the final
> offset parameter (either via av_log or via a debugger); or you just sum
> the VLC.table_size values before you make the VLCs static. It is no big
> deal and it is not much different from what it is now.
>
> > On Tue, Nov 10, 2020 at 1:15 PM Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@gmail.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> Because it is a bad idea.
> >>>>
> >>>> And I still like to hear a reason for this.
> >>>>
> >>>>
> >>> Code is there so correct intended size is always allocated.
> >>>
> >>
> >> No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
> >> flag set and without it.
> >> If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
> >> too) allocates the needed space for the VLC table (actually, it
> >> overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
> >> this case the whole given VLC is treated as uninitialized, i.e.
> >> vlc->table_allocated is ignored.
> >>
> >> If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
> >> at least vlc->table_allocated elements and vlc->table_allocated must be
> >> sufficient for the whole VLC: This buffer will never be (re)allocated;
> >> it actually points to static storage and not to anything that is
> >> heap-allocated. Given the error message that this commit intends to
> >> remove there is currently a requirement for table_allocated to be
> >> exactly equal to the actually required size. And this requires tables of
> >> the sizes if VLCs are initialized in a loop (alternatively one can of
> >> course unroll the loop and hardcode the values via one of the INIT_VLC
> >> macros).
> >>
> >>>
> >>>>> No need to change code that worked for ages.
> >>>>>
> >>>>
> >>>> It allows to remove useless tables and it simplifies making more VLCs
> >>>> that should be static static.
> >>>>
> >>>
> >>> What is static static? How does it allows to remove useless tables?
> >>>
> >>
> >> And this requirement is just a pointless straitjacket. If one drops it,
> >> one can remove the useless tables by using the pattern mentioned in the
> >> commit message. Several commits of this series provide examples for
> >> this; e.g. #12
> >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
> >> #40
> >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
> >> #53
> >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
> >> #57
> >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html)
> or
> >> #88 (
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).
> >>
> >> These are only examples where existing offsets tables were removed;
> >> making VLCs static like in #68
> >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
> >> making these VLCs static is especially beneficial for a multithreaded
> >> decoder like this one so that the VLCs can be shared between all
> >> threads) would be more complicated if the requirement of always using
> >> exactly the required size were not dropped.
> >>
> >>>
> >>>>
> >>>>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@gmail.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>> I do not think this is a good direction.
> >>>>>>>
> >>>>>>
> >>>>>> Why?
> >>>>>>
> >>>>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
> >>>>>>> andreas.rheinhardt@gmail.com> wrote:
> >>>>>>>
> >>>>>>>> Right now the allocated size of the VLC table of a static VLC has
> to
> >>>>>>>> exactly match the size actually used for the VLC: If it is not
> >> enough,
> >>>>>>>> abort is called; if it is more than enough, an error message is
> >>>>>>>> emitted. This is no problem when one wants to initialize an
> >> individual
> >>>>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the
> needed
> >>>>>>>> size. Yet it is an obstacle when one wants to initialize several
> >> VLCs
> >>>>>>>> in a loop as one then needs to add an array for the sizes/offsets
> of
> >>>>>>>> the VLC tables (unless max_depth of all arrays is one in which
> case
> >>>>>>>> the sizes are derivable from the number of bits used).
> >>>>>>>>
> >>>>>>>> Yet said size array is not necessary if one removes the warning
> for
> >>>> too
> >>>>>>>> big buffers. The reason is that the amount of entries needed for
> the
> >>>>>>>> table is of course generated as a byproduct of initializing the
> VLC.
> >>>>>>>> So one can proceed as follows:
> >>>>>>>>
> >>>>>>>> static VLC vlcs[NUM];
> >>>>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
> >>>>>>>>
> >>>>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
> >>>>>>>>     vlcs[i].table           = &vlc_table[offset];
> >>>>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
> >>>>>>>>     init_vlc();
> >>>>>>>>     offset += vlcs[i].table_size;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> Of course, BUF_SIZE should be equal to the number of entries
> >> actually
> >>>>>>>> needed.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> >>>>>>>> ---
> >>>>>>>>  libavcodec/bitstream.c | 3 ---
> >>>>>>>>  1 file changed, 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> >>>>>>>> index 03d39ad88c..4ffec7e17a 100644
> >>>>>>>> --- a/libavcodec/bitstream.c
> >>>>>>>> +++ b/libavcodec/bitstream.c
> >>>>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int
> nb_bits,
> >>>> int
> >>>>>>>> nb_codes, VLCcode *codes,
> >>>>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
> >>>>>>>>
> >>>>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
> >>>>>>>> -        if(vlc->table_size != vlc->table_allocated)
> >>>>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
> >>>>>>>> vlc->table_size, vlc->table_allocated);
> >>>>>>>> -
> >>>>>>>>          av_assert0(ret >= 0);
> >>>>>>>>          *vlc_arg = *vlc;
> >>>>>>>>      } else {
> >>>>>>>> --
> >>>>>>>> 2.25.1
> >>>>>>>>
> >>>>>>
> >>>>
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 10, 2020, 12:38 p.m. UTC | #10
Andreas Rheinhardt:
> Paul B Mahol:
>> Than how one is supposed to guess correct size to put when this log is gone?
>>
> The developer who wants to make VLCs static just has to use a huge array
> (in order not to run into the abort) and get the value of the final
> offset parameter (either via av_log or via a debugger); or you just sum
> the VLC.table_size values before you make the VLCs static. It is no big
> deal and it is not much different from what it is now.
> 

I forgot to mention that this last statement only applies to finding the
total size of the buffer. But as long as this log statement is there,
this is not enough: One really needs all the sizes (or offsets as one
prefers) which is a bit more work (and leads to some unnecessary code
and unnecessary tables).

>> On Tue, Nov 10, 2020 at 1:15 PM Andreas Rheinhardt <
>> andreas.rheinhardt@gmail.com> wrote:
>>
>>> Paul B Mahol:
>>>> On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>
>>>>> Paul B Mahol:
>>>>>> Because it is a bad idea.
>>>>>
>>>>> And I still like to hear a reason for this.
>>>>>
>>>>>
>>>> Code is there so correct intended size is always allocated.
>>>>
>>>
>>> No. There are two ways to create a VLC: With the INIT_VLC_USE_NEW_STATIC
>>> flag set and without it.
>>> If it is unset, ff_init_vlc_sparse() (and ff_init_vlc_from_lengths(),
>>> too) allocates the needed space for the VLC table (actually, it
>>> overallocates) and the VLC must in turn be freed via ff_free_vlc(). In
>>> this case the whole given VLC is treated as uninitialized, i.e.
>>> vlc->table_allocated is ignored.
>>>
>>> If it is set, then vlc->table must point to an array VLC_TYPE[][2] with
>>> at least vlc->table_allocated elements and vlc->table_allocated must be
>>> sufficient for the whole VLC: This buffer will never be (re)allocated;
>>> it actually points to static storage and not to anything that is
>>> heap-allocated. Given the error message that this commit intends to
>>> remove there is currently a requirement for table_allocated to be
>>> exactly equal to the actually required size. And this requires tables of
>>> the sizes if VLCs are initialized in a loop (alternatively one can of
>>> course unroll the loop and hardcode the values via one of the INIT_VLC
>>> macros).
>>>
>>>>
>>>>>> No need to change code that worked for ages.
>>>>>>
>>>>>
>>>>> It allows to remove useless tables and it simplifies making more VLCs
>>>>> that should be static static.
>>>>>
>>>>
>>>> What is static static? How does it allows to remove useless tables?
>>>>
>>>
>>> And this requirement is just a pointless straitjacket. If one drops it,
>>> one can remove the useless tables by using the pattern mentioned in the
>>> commit message. Several commits of this series provide examples for
>>> this; e.g. #12
>>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272139.html),
>>> #40
>>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272167.html),
>>> #53
>>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272179.html),
>>> #57
>>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272183.html) or
>>> #88 (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272215.html).
>>>
>>> These are only examples where existing offsets tables were removed;
>>> making VLCs static like in #68
>>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2020-November/272195.html;
>>> making these VLCs static is especially beneficial for a multithreaded
>>> decoder like this one so that the VLCs can be shared between all
>>> threads) would be more complicated if the requirement of always using
>>> exactly the required size were not dropped.
>>>
>>>>
>>>>>
>>>>>> On Tue, Nov 10, 2020 at 12:26 PM Andreas Rheinhardt <
>>>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>>>
>>>>>>> Paul B Mahol:
>>>>>>>> I do not think this is a good direction.
>>>>>>>>
>>>>>>>
>>>>>>> Why?
>>>>>>>
>>>>>>>> On Tue, Nov 10, 2020 at 11:50 AM Andreas Rheinhardt <
>>>>>>>> andreas.rheinhardt@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> Right now the allocated size of the VLC table of a static VLC has to
>>>>>>>>> exactly match the size actually used for the VLC: If it is not
>>> enough,
>>>>>>>>> abort is called; if it is more than enough, an error message is
>>>>>>>>> emitted. This is no problem when one wants to initialize an
>>> individual
>>>>>>>>> VLC via one of the INIT_VLC macros as one just hardcodes the needed
>>>>>>>>> size. Yet it is an obstacle when one wants to initialize several
>>> VLCs
>>>>>>>>> in a loop as one then needs to add an array for the sizes/offsets of
>>>>>>>>> the VLC tables (unless max_depth of all arrays is one in which case
>>>>>>>>> the sizes are derivable from the number of bits used).
>>>>>>>>>
>>>>>>>>> Yet said size array is not necessary if one removes the warning for
>>>>> too
>>>>>>>>> big buffers. The reason is that the amount of entries needed for the
>>>>>>>>> table is of course generated as a byproduct of initializing the VLC.
>>>>>>>>> So one can proceed as follows:
>>>>>>>>>
>>>>>>>>> static VLC vlcs[NUM];
>>>>>>>>> static VLC_TYPE vlc_table[BUF_SIZE][2];
>>>>>>>>>
>>>>>>>>> for (int i = 0, offset = 0; i < NUM; i++) {
>>>>>>>>>     vlcs[i].table           = &vlc_table[offset];
>>>>>>>>>     vlcs[i].table_allocated = BUF_SIZE - offset;
>>>>>>>>>     init_vlc();
>>>>>>>>>     offset += vlcs[i].table_size;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Of course, BUF_SIZE should be equal to the number of entries
>>> actually
>>>>>>>>> needed.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  libavcodec/bitstream.c | 3 ---
>>>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
>>>>>>>>> index 03d39ad88c..4ffec7e17a 100644
>>>>>>>>> --- a/libavcodec/bitstream.c
>>>>>>>>> +++ b/libavcodec/bitstream.c
>>>>>>>>> @@ -281,9 +281,6 @@ static int vlc_common_end(VLC *vlc, int nb_bits,
>>>>> int
>>>>>>>>> nb_codes, VLCcode *codes,
>>>>>>>>>      int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
>>>>>>>>>
>>>>>>>>>      if (flags & INIT_VLC_USE_NEW_STATIC) {
>>>>>>>>> -        if(vlc->table_size != vlc->table_allocated)
>>>>>>>>> -            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n",
>>>>>>>>> vlc->table_size, vlc->table_allocated);
>>>>>>>>> -
>>>>>>>>>          av_assert0(ret >= 0);
>>>>>>>>>          *vlc_arg = *vlc;
>>>>>>>>>      } else {
>>>>>>>>> --
>>>>>>>>> 2.25.1
>>>>>>>>>
>>>>>>>
>>>>>
Michael Niedermayer Nov. 10, 2020, 4:35 p.m. UTC | #11
On Tue, Nov 10, 2020 at 01:37:26PM +0100, Paul B Mahol wrote:
> Big disagree, please keep this log intact.

i agree, for people writing alot of codecs like paul is doing this av_log
is helpful

Thanks

[...]
Paul B Mahol Nov. 10, 2020, 7:38 p.m. UTC | #12
On Tue, Nov 10, 2020 at 5:35 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Nov 10, 2020 at 01:37:26PM +0100, Paul B Mahol wrote:
> > Big disagree, please keep this log intact.
>
> i agree, for people writing alot of codecs like paul is doing this av_log
> is helpful
>
> It can be made less verbose, e.g. into debug.
But I see no point in removing it completely.


> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- Aristotle
> _______________________________________________
> 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".
Andreas Rheinhardt Nov. 12, 2020, 4:42 a.m. UTC | #13
Paul B Mahol:
> On Tue, Nov 10, 2020 at 5:35 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
>> On Tue, Nov 10, 2020 at 01:37:26PM +0100, Paul B Mahol wrote:
>>> Big disagree, please keep this log intact.
>>
>> i agree, for people writing alot of codecs like paul is doing this av_log
>> is helpful
>>
>> It can be made less verbose, e.g. into debug.
> But I see no point in removing it completely.
> 

Another alternative would be to add a flag that indicates that it is
expected that allocated_size is bigger than the needed size.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
index 03d39ad88c..4ffec7e17a 100644
--- a/libavcodec/bitstream.c
+++ b/libavcodec/bitstream.c
@@ -281,9 +281,6 @@  static int vlc_common_end(VLC *vlc, int nb_bits, int nb_codes, VLCcode *codes,
     int ret = build_table(vlc, nb_bits, nb_codes, codes, flags);
 
     if (flags & INIT_VLC_USE_NEW_STATIC) {
-        if(vlc->table_size != vlc->table_allocated)
-            av_log(NULL, AV_LOG_ERROR, "needed %d had %d\n", vlc->table_size, vlc->table_allocated);
-
         av_assert0(ret >= 0);
         *vlc_arg = *vlc;
     } else {