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

[Lynne]

On 05/05/2025 16:57, Andreas Rheinhardt wrote:
> 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?
I start with the tables, not the function that initializes them. All I 
knew is that VLCs are required to be in separate codes+bits tables.
There are multiple variants of VLC init, so I have no idea whether one 
variant can be adapted to another.

Again, I'm just posting this for reviews.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA2FEA5F03F034464.asc
Type: application/pgp-keys
Size: 624 bytes
Desc: OpenPGP public key
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250505/003e3558/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250505/003e3558/attachment.sig>