Re: [FFmpeg-devel] [PATCH 3/4 v2] avcodec/aacdec: add support for channel configuration 14

[James Almer <jamrial@gmail.com>]

On 10/28/2022 9:50 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 10/28/2022 9:31 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> It corresponds to the 7.1(top) layout.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavcodec/aacdec_template.c | 23 ++++++++++++++++++++++-
>>>>    libavcodec/aacdectab.h       |  6 +++---
>>>>    libavcodec/mpeg4audio.c      |  5 +++--
>>>>    libavcodec/mpeg4audio.h      |  2 +-
>>>>    4 files changed, 29 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>>>> index c10bc743b6..4b8e29ba71 100644
>>>> --- a/libavcodec/aacdec_template.c
>>>> +++ b/libavcodec/aacdec_template.c
>>>> @@ -404,6 +404,21 @@ static uint64_t sniff_channel_order(uint8_t
>>>> (*layout_map)[3], int tags)
>>>>            i++;
>>>>        }
>>>>    +    // The previous checks would end up at 4 at this point for
>>>> chan_config 14
>>>> +    if (layout == AV_CH_LAYOUT_5POINT1 && tags == 5 && i == 4) {
>>>> +        const uint8_t (*reference_layout_map)[3] =
>>>> aac_channel_layout_map[13];
>>>> +        for (int j = 0; j < tags; j++) {
>>>> +            if (layout_map[j][0] != reference_layout_map[j][0] ||
>>>> +                layout_map[j][2] != reference_layout_map[j][2])
>>>> +                goto end_of_layout_definition;
>>>> +        }
>>>> +
>>>> +        i += assign_pair(e2c_vec, layout_map, i,
>>>> +                         AV_CH_TOP_FRONT_LEFT,
>>>> +                         AV_CH_TOP_FRONT_RIGHT,
>>>> +                         AAC_CHANNEL_FRONT,
>>>> +                         &layout);
>>>> +    }
>>>>        // The previous checks would end up at 8 at this point for 22.2
>>>>        if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) {
>>>>            const uint8_t (*reference_layout_map)[3] =
>>>> aac_channel_layout_map[12];
>>>> @@ -633,7 +648,7 @@ static int set_default_channel_config(AACContext
>>>> *ac, AVCodecContext *avctx,
>>>>                                          int channel_config)
>>>>    {
>>>>        if (channel_config < 1 || (channel_config > 7 && channel_config
>>>> < 11) ||
>>>> -        channel_config > 13) {
>>>> +        channel_config > 14) {
>>>>            av_log(avctx, AV_LOG_ERROR,
>>>>                   "invalid default channel configuration (%d)\n",
>>>>                   channel_config);
>>>> @@ -717,6 +732,12 @@ static ChannelElement *get_che(AACContext *ac,
>>>> int type, int elem_id)
>>>>        /* For indexed channel configurations map the channels solely
>>>> based
>>>>         * on position. */
>>>>        switch (ac->oc[1].m4ac.chan_config) {
>>>> +    case 14:
>>>> +        if (ac->tags_mapped > 2 && ((type == TYPE_CPE && elem_id <
>>>> 3) ||
>>>> +                                    (type == TYPE_LFE && elem_id <
>>>> 1))) {
>>>> +            ac->tags_mapped++;
>>>> +            return ac->tag_che_map[type][elem_id] =
>>>> ac->che[type][elem_id];
>>>> +        }
>>>>        case 13:
>>>>            if (ac->tags_mapped > 3 && ((type == TYPE_CPE && elem_id <
>>>> 8) ||
>>>>                                        (type == TYPE_SCE && elem_id <
>>>> 6) ||
>>>> diff --git a/libavcodec/aacdectab.h b/libavcodec/aacdectab.h
>>>> index b820f10556..e9bbd1fe13 100644
>>>> --- a/libavcodec/aacdectab.h
>>>> +++ b/libavcodec/aacdectab.h
>>>> @@ -68,8 +68,8 @@ static const uint8_t
>>>> aac_channel_layout_map[16][16][3] = {
>>>>          { TYPE_SCE, 5, AAC_CHANNEL_FRONT }, // SCE6 = BtFC,
>>>>          { TYPE_CPE, 7, AAC_CHANNEL_FRONT }, // CPE8 = BtFL and BtFR
>>>>        },
>>>> +    { { TYPE_SCE, 0, AAC_CHANNEL_FRONT }, { TYPE_CPE, 0,
>>>> AAC_CHANNEL_FRONT }, { TYPE_CPE, 1, AAC_CHANNEL_SIDE }, { TYPE_LFE,
>>>> 0, AAC_CHANNEL_LFE }, { TYPE_CPE, 2, AAC_CHANNEL_FRONT  }, },
>>>>        { { 0, } },
>>>> -    /* TODO: Add 7+1 TOP configuration */
>>>>    };
>>>>      #if FF_API_OLD_CHANNEL_LAYOUT
>>>> @@ -84,8 +84,8 @@ static const uint64_t aac_channel_layout[] = {
>>>>        AV_CH_LAYOUT_6POINT1,
>>>>        AV_CH_LAYOUT_7POINT1,
>>>>        AV_CH_LAYOUT_22POINT2,
>>>> +    AV_CH_LAYOUT_7POINT1_TOP,
>>>>        0,
>>>> -    /* AV_CH_LAYOUT_7POINT1_TOP, */
>>>>    };
>>>>    #endif
>>>>    @@ -100,8 +100,8 @@ static const AVChannelLayout aac_ch_layout[] = {
>>>>        AV_CHANNEL_LAYOUT_6POINT1,
>>>>        AV_CHANNEL_LAYOUT_7POINT1,
>>>>        AV_CHANNEL_LAYOUT_22POINT2,
>>>> +    AV_CHANNEL_LAYOUT_7POINT1_TOP,
>>>>        { 0 },
>>>> -    /* AV_CHANNEL_LAYOUT_7POINT1_TOP, */
>>>>    };
>>>>      #endif /* AVCODEC_AACDECTAB_H */
>>>> diff --git a/libavcodec/mpeg4audio.c b/libavcodec/mpeg4audio.c
>>>> index e38a8c0852..4a996411d1 100644
>>>> --- a/libavcodec/mpeg4audio.c
>>>> +++ b/libavcodec/mpeg4audio.c
>>>> @@ -56,7 +56,7 @@ static int parse_config_ALS(GetBitContext *gb,
>>>> MPEG4AudioConfig *c, void *logctx
>>>>        return 0;
>>>>    }
>>>>    -const uint8_t ff_mpeg4audio_channels[14] = {
>>>> +const uint8_t ff_mpeg4audio_channels[15] = {
>>>>        0,
>>>>        1, // mono (1/0)
>>>>        2, // stereo (2/0)
>>>> @@ -70,7 +70,8 @@ const uint8_t ff_mpeg4audio_channels[14] = {
>>>>        0,
>>>>        7, // 3/3.1
>>>>        8, // 3/2/2.1
>>>> -    24 // 3/3/3 - 5/2/3 - 3/0/0.2
>>>> +    24, // 3/3/3 - 5/2/3 - 3/0/0.2
>>>> +    8 // 3/2.1 - 2/0
>>>
>>> Is it guaranteed that 8 will always be the last element of this array?
>>> If not, you should use "8,", so that the comma won't have to be added
>>> when someone makes an addition in the future.
>>
>> What does the pedantic gcc option say about this? I recall it complained
>> about either having or missing the comma in the last element, so given
>> that it makes no real difference I'd go with whatever keeps it quiet.
>>
> 
> You are confusing this with enum declarations: In C90, a trailing comma
> was not allowed in their declaration; C99 lifted this restriction (we
> have enums that make use of this). But trailing commas were always
> allowed in initializers.
> 
> - Andreas
> 
> PS: Do you use -pedantic?

No, but i remember someone started contributing patches long ago to 
silence -pedantic warnings, so might as well not introduce new ones.

Will add the comma then.
_______________________________________________
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".