[FFmpeg-devel] [PATCH 3/3] avcodec/aacdec: Translate PCE to avutil channel layouts
pkv.stream
pkv.stream at gmail.com
Wed Oct 24 23:05:08 EEST 2018
> Thanks for the patch. Overall I like this approach, but this patch has some
> must-fix issues. In general make sure make fate-aac works with address
> sanitizer.
Done, I have incorporated all your comments.
All fate tests are successful now.
Thanks very much.
>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>> index b60b31a92c..53b7ea6669 100644
>> --- a/libavcodec/aacdec_template.c
>> +++ b/libavcodec/aacdec_template.c
>> @@ -155,6 +155,26 @@ static av_cold int che_configure(AACContext *ac,
>> return 0;
>> }
>>
>> +static uint8_t* pce_reorder_map(uint64_t layout)
>> +{
>> + uint8_t map[8] = {0, 1, 2, 3, 4, 5, 6, 7};
>> + uint8_t map8[8] = { 2, 0, 1, 6, 7, 4, 5, 3 };
>> + uint8_t map7[7] = { 2, 0, 1, 4, 5, 6, 3 };
>> + uint8_t map6[6] = { 2, 0, 1, 4, 5, 3 };
>> + uint8_t map5[5] = { 2, 0, 1, 4, 3 };
>> + if (layout == AV_CH_LAYOUT_7POINT1 || layout == AV_CH_LAYOUT_7POINT1_WIDE ||
>> + layout == AV_CH_LAYOUT_7POINT1_WIDE_BACK)
>> + return map8;
>> + if (layout == AV_CH_LAYOUT_6POINT1 || layout == AV_CH_LAYOUT_6POINT1_BACK ||
>> + layout == AV_CH_LAYOUT_6POINT1_FRONT)
>> + return map7;
>> + if (layout == AV_CH_LAYOUT_5POINT1 || layout == AV_CH_LAYOUT_5POINT1_BACK)
>> + return map6;
>> + if (layout == AV_CH_LAYOUT_4POINT1)
>> + return map5;
>> + return map;
> You can't return local stack arrays like this. Return pointers to const arrays
> that live outside the function. Consider building with -Wreturn-stack-address.
>
>> +}
>> +
>> static int frame_configure_elements(AVCodecContext *avctx)
>> {
>> AACContext *ac = avctx->priv_data;
>> @@ -180,10 +200,15 @@ static int frame_configure_elements(AVCodecContext *avctx)
>> if ((ret = ff_get_buffer(avctx, ac->frame, 0)) < 0)
>> return ret;
>>
>> + /* reorder channels in case pce table was used with LFE channel */
>> + uint8_t reorder[8] = { 0 };
>> + if (ac->oc[1].m4ac.chan_config == 0 && ac->oc[1].channel_layout && avctx->channels < 9)
> Can we ever hit this if with channel_layout == 0? If we can it seems
> like all zero output is not what we want.
> If we can't then what are we checking it for?
>
>> + memcpy(reorder, pce_reorder_map(ac->oc[1].channel_layout), avctx->channels * sizeof(uint8_t));
>> /* map output channel pointers to AVFrame data */
>> for (ch = 0; ch < avctx->channels; ch++) {
>> + int ch_remapped = avctx->channels < 9 ? reorder[ch] : ch;
>> if (ac->output_element[ch])
>> - ac->output_element[ch]->ret = (INTFLOAT *)ac->frame->extended_data[ch];
>> + ac->output_element[ch]->ret = (INTFLOAT *)ac->frame->extended_data[ch_remapped];
>> }
>>
>> return 0;
> [...]
>> +static uint64_t convert_layout_map_to_av_layout(uint8_t layout_map[MAX_ELEM_ID * 4][3])
>> +{
>> + int i, config;
>> + config = 0;
>> + // look up pce table for channel layout correspondance used by native encoder and decoder
> nit: "correspondence"
>> + for (i = 1; i < PCE_LAYOUT_NBR; i++) {
>> + if (memcmp(layout_map, pce_channel_layout_map[i], sizeof(uint8_t) * 3 * PCE_MAX_TAG) == 0) {
> nit: use variables with sizeof. e.g. PCE_MAX_TAG * sizeof(layout_map[0])
>> + config = i;
>> + break;
>> + }
>> + }
>> +
>> + switch(config) {
> nit: this should probably live as a table with: pce_channel_layout_map
>> + case 1: return AV_CH_LAYOUT_HEXADECAGONAL;
>> + case 2: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER |
>> + AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT |
>> + AV_CH_TOP_FRONT_CENTER | AV_CH_TOP_BACK_CENTER |
>> + AV_CH_TOP_BACK_LEFT | AV_CH_TOP_BACK_RIGHT;
>> + case 3: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER |
>> + AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT |
>> + AV_CH_TOP_FRONT_CENTER | AV_CH_TOP_BACK_LEFT |
>> + AV_CH_TOP_BACK_RIGHT;
>> + case 4: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER |
>> + AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT |
>> + AV_CH_TOP_FRONT_CENTER | AV_CH_TOP_BACK_CENTER;
>> + case 5: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER |
>> + AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT |
>> + AV_CH_TOP_FRONT_CENTER;
>> + case 6: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER |
>> + AV_CH_TOP_FRONT_LEFT | AV_CH_TOP_FRONT_RIGHT;
>> + case 7: return AV_CH_LAYOUT_6POINT0_FRONT | AV_CH_BACK_CENTER |
>> + AV_CH_BACK_LEFT | AV_CH_BACK_RIGHT | AV_CH_TOP_CENTER;
>> + case 8: return AV_CH_LAYOUT_OCTAGONAL | AV_CH_TOP_CENTER;
>> + case 9: return AV_CH_LAYOUT_OCTAGONAL;
>> + case 10: return AV_CH_LAYOUT_7POINT1;
>> + case 11: return AV_CH_LAYOUT_7POINT1_WIDE;
>> + case 12: return AV_CH_LAYOUT_7POINT1_WIDE_BACK;
>> + case 13: return AV_CH_LAYOUT_6POINT1;
>> + case 14: return AV_CH_LAYOUT_6POINT1_BACK;
>> + case 15: return AV_CH_LAYOUT_7POINT0;
>> + case 16: return AV_CH_LAYOUT_7POINT0_FRONT;
>> + case 17: return AV_CH_LAYOUT_HEXAGONAL;
>> + case 18: return AV_CH_LAYOUT_6POINT1_FRONT;
>> + case 19: return AV_CH_LAYOUT_6POINT0;
>> + case 20: return AV_CH_LAYOUT_5POINT1;
>> + case 21: return AV_CH_LAYOUT_5POINT1_BACK;
>> + case 22: return AV_CH_LAYOUT_4POINT1;
>> + case 23: return AV_CH_LAYOUT_6POINT0_FRONT;
>> + case 24: return AV_CH_LAYOUT_5POINT0;
>> + case 25: return AV_CH_LAYOUT_5POINT0_BACK;
>> + case 26: return AV_CH_LAYOUT_4POINT0;
>> + case 27: return AV_CH_LAYOUT_3POINT1;
>> + case 28: return AV_CH_LAYOUT_QUAD;
>> + case 29: return AV_CH_LAYOUT_2_2;
>> + case 30: return AV_CH_LAYOUT_2POINT1;
>> + case 31: return AV_CH_LAYOUT_2_1;
>> + case 32: return AV_CH_LAYOUT_SURROUND;
>> + case 33: return AV_CH_LAYOUT_STEREO;
>> + case 34: return AV_CH_LAYOUT_MONO;
>> + case 0:
>> + default: return 0;
>> + }
>> +}
>> +
>> +static uint64_t sniff_channel_order(AACContext *ac,
>> + uint8_t (*layout_map)[3], int tags)
>> {
>> int i, n, total_non_cc_elements;
>> struct elem_to_channel e2c_vec[4 * MAX_ELEM_ID] = { { 0 } };
>> int num_front_channels, num_side_channels, num_back_channels;
>> + int num_front_channels_SCE, num_front_channels_CPE, num_LFE_channels;
>> + int num_side_channels_CPE, num_back_channels_SCE, num_back_channels_CPE;
>> + int channels;
>> uint64_t layout;
>>
>> + if (ac->oc[1].m4ac.chan_config == 0) {
>> + // first use table to find layout
>> + if (PCE_MAX_TAG >= tags)
>> + layout = convert_layout_map_to_av_layout(layout_map);
>> + if (layout > 0) {
> If the first if is false, layout is uninitialized, maybe move up
> `layout = 0` from below
>> + char buf[64];
>> + av_get_channel_layout_string(buf, sizeof(buf), -1, layout);
>> + av_log(ac->avctx, AV_LOG_WARNING, "Using PCE table: channel layout decoded as %s (%#llx)\n", buf, layout);
>> + return layout;
>> + }
>> + // build a custom layout directly from pce (CC elements are not taken into account)
>> + layout = 0;
> [...]
>> diff --git a/libavcodec/aacdectab.h b/libavcodec/aacdectab.h
>> index baf51a74bf..bdd1f15839 100644
>> --- a/libavcodec/aacdectab.h
>> +++ b/libavcodec/aacdectab.h
>> @@ -71,4 +71,47 @@ static const uint64_t aac_channel_layout[16] = {
>> /* AV_CH_LAYOUT_7POINT1_TOP, */
>> };
>>
>> +#define PCE_LAYOUT_NBR 35
>> +#define PCE_MAX_TAG 10
>> +
>> +/* number of tags for the pce_channel_layout_map table */
>> +static const int8_t tags_pce_config[35] = {0, 10, 9, 8, 7, 8, 7, 6, 6, 5, 5, 5, 5, 5, 5, 4, 4, 4, 4, 4, 4, 4, 4, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 1, 1};
>> +
>> +static const uint8_t pce_channel_layout_map[35][10][3] = {
> nit: Use your defines in these decls so the compiler will shout if
> they don't match.
>> + { { 0, } },
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list