> From a12637c97c3140a1676f20b19c91647313379b39 Mon Sep 17 00:00:00 2001
> From: pkviet <pkv.str...@gmail.com>
> Date: Sun, 9 Sep 2018 16:47:32 +0200
> Subject: [PATCH 3/3] avcodec/aacdec: Translate pce to avutil channel_layout
>
> This commit enables the native aac decoder to translate pce to
> ffmpeg channel layouts without any user input.
> For all pce generated by the native aac encoder a table is used.
> For more general pce (up to 20 channels) a custom layout is built.
> Fixes trac issue 7273 with patch 1 of the series.
>
> Signed-off-by: pkviet <pkv.str...@gmail.com>
> ---
> libavcodec/aacdec_template.c | 195
> +++++++++++++++++++++++++++++++++++++++++--
> libavcodec/aacdectab.h | 43 ++++++++++
> 2 files changed, 233 insertions(+), 5 deletions(-)
>
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.
> 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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
