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

[Paul B Mahol]

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 at gmail.com> wrote:

> Paul B Mahol:
> > On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt <
> > andreas.rheinhardt at 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 at 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 at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".