Big disagree, please keep this log intact. Thanks. On Tue, Nov 10, 2020 at 1:31 PM Andreas Rheinhardt < andreas.rheinha...@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.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".
