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.rheinha...@gmail.com> wrote: > Paul B Mahol: > > On Tue, Nov 10, 2020 at 12:31 PM Andreas Rheinhardt < > > andreas.rheinha...@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.rheinha...@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.rheinha...@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.rheinha...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
