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

Paul B Mahol onemda at gmail.com
Tue Nov 10 14:37:26 EET 2020


Big disagree, please keep this log intact.
Thanks.

On Tue, Nov 10, 2020 at 1:31 PM Andreas Rheinhardt <
andreas.rheinhardt at 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 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".


More information about the ffmpeg-devel mailing list