Re: [FFmpeg-devel] [PATCH] aactab: add and initialize 2D VLC tables for USAC Mps212

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Lynne:
> On 05/05/2025 15:52, Andreas Rheinhardt wrote:
>> Lynne:
>>> ---
>>>   libavcodec/aac/aacdec_tab.c |   54 ++
>>>   libavcodec/aactab.c         | 1820 +++++++++++++++++++++++++++++++++++
>>>   libavcodec/aactab.h         |   20 +
>>>   3 files changed, 1894 insertions(+)
>>>
>>
>> 1. This should only be applied if it is used which this patch does not.
> 
> Just posting this for reviews.
>>> diff --git a/libavcodec/aac/aacdec_tab.c b/libavcodec/aac/aacdec_tab.c
>>> index 45a84a9a72..5ba20c0d8a 100644
>>> --- a/libavcodec/aac/aacdec_tab.c
>>> +++ b/libavcodec/aac/aacdec_tab.c
>>> @@ -111,6 +111,16 @@ const AVChannelLayout ff_aac_ch_layout[] = {
>>>   VLCElem ff_vlc_scalefactors[352];
>>>   const VLCElem *ff_vlc_spectral[11];
>>>   +const VLCElem *ff_vlc_cld_lav3_2D[2][2];
>>> +const VLCElem *ff_vlc_cld_lav5_2D[2][2];
>>> +const VLCElem *ff_vlc_cld_lav7_2D[2][2];
>>> +const VLCElem *ff_vlc_cld_lav9_2D[2][2];
>>> +
>>> +const VLCElem *ff_vlc_icc_lav1_2D[2][2];
>>> +const VLCElem *ff_vlc_icc_lav3_2D[2][2];
>>> +const VLCElem *ff_vlc_icc_lav5_2D[2][2];
>>> +const VLCElem *ff_vlc_icc_lav7_2D[2][2];
>>> +
>>>   /// Huffman tables for SBR
>>>     static const uint8_t sbr_huffman_tab[][2] = {
>>> @@ -279,6 +289,50 @@ static av_cold void aacdec_common_init(void)
>>>                                         0);
>>>       }
>>>   +#define LAV_N_PAIR(NAME, NB) \
>>> +    ff_vlc_ ## NAME[0][0] = ff_vlc_init_tables(&state, NB, NB, \
>>> +                                               ff_aac_ ## NAME ##
>>> _0_0_bits, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_0_bits), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_0_bits), \
>>> +                                               ff_aac_ ## NAME ##
>>> _0_0_codes, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_0_codes), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_0_codes), \
>>> +                                               0); \
>>> +    ff_vlc_ ## NAME[0][1] = ff_vlc_init_tables(&state, NB, NB, \
>>> +                                               ff_aac_ ## NAME ##
>>> _0_1_bits, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_1_bits), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_1_bits), \
>>> +                                               ff_aac_ ## NAME ##
>>> _0_1_codes, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_1_codes), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _0_1_codes), \
>>> +                                               0); \
>>> +    ff_vlc_ ## NAME[1][0] = ff_vlc_init_tables(&state, NB, NB, \
>>> +                                               ff_aac_ ## NAME ##
>>> _1_0_bits, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_0_bits), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_0_bits), \
>>> +                                               ff_aac_ ## NAME ##
>>> _1_0_codes, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_0_codes), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_0_codes), \
>>> +                                               0); \
>>> +    ff_vlc_ ## NAME[1][1] = ff_vlc_init_tables(&state, NB, NB, \
>>> +                                               ff_aac_ ## NAME ##
>>> _1_1_bits, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_1_bits), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_1_bits), \
>>> +                                               ff_aac_ ## NAME ##
>>> _1_1_codes, \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_1_codes), \
>>> +                                               sizeof(ff_aac_ ##
>>> NAME ## _1_1_codes), \
>>> +                                               0);
>>
>> 2. I am very much surprised that it works at all (does it?). You use the
>> same state and therefore the same static storage as before; you add new
>> VLCs, yet you do not increase the size of vlc_buf.
> 
> The tables are all rather small.

When I decrease the size of vlc_buf by only one element, initialization
fails with an abort, as expected. And so it should be for you, because
there is just no space in vlc_buf left.

> 
>> 3. Can't you initialize this in a loop like ff_aac_sbr_vlc? This would
>> remove code duplication as well as relocations.
> 
> I didn't know that was possible. vlc.h is poorly documented, with many
> overlapping, wrapped, renamed and macro'd functions that code from
> different decade have gotten adapted to.

Why should this not be possible? You have an example in this very
function. And what exactly is poorly documented?

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".