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.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".
