diff mbox series

[FFmpeg-devel,7/7] avcodec/aacdec_template: add support for 22.2 / channel_config 13

Message ID 20200801110730.30642-8-jeebjp@gmail.com
State Accepted
Headers show
Series 22.2 channel layout support for AAC decoding
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Aug. 1, 2020, 11:07 a.m. UTC
---
 libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 11 deletions(-)

Comments

Michael Niedermayer Aug. 15, 2020, 11:15 p.m. UTC | #1
On Sat, Aug 01, 2020 at 02:07:30PM +0300, Jan Ekström wrote:
> ---
>  libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
>  1 file changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index 21db12fdab..8c5048cc13 100644
> --- a/libavcodec/aacdec_template.c
> +++ b/libavcodec/aacdec_template.c
> @@ -387,17 +387,77 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags)
>          i++;
>      }
>  
> -    // Must choose a stable sort
> +    // The previous checks would end up at 8 at this point for 22.2
> +    if (tags == 16 && i == 8) {
> +        e2c_vec[i] = (struct elem_to_channel) {
> +            .av_position  = AV_CH_TOP_FRONT_CENTER,
> +            .syn_ele      = layout_map[i][0],
> +            .elem_id      = layout_map[i][1],
> +            .aac_position = layout_map[i][2]
> +        }; i++;
> +        i += assign_pair(e2c_vec, layout_map, i,
> +                         AV_CH_TOP_FRONT_LEFT,
> +                         AV_CH_TOP_FRONT_RIGHT,
> +                         AAC_CHANNEL_FRONT);
> +        i += assign_pair(e2c_vec, layout_map, i,
> +                         AV_CH_TOP_SIDE_LEFT,
> +                         AV_CH_TOP_SIDE_RIGHT,
> +                         AAC_CHANNEL_SIDE);
> +        e2c_vec[i] = (struct elem_to_channel) {
> +            .av_position  = AV_CH_TOP_CENTER,
> +            .syn_ele      = layout_map[i][0],
> +            .elem_id      = layout_map[i][1],
> +            .aac_position = layout_map[i][2]
> +        }; i++;
> +        i += assign_pair(e2c_vec, layout_map, i,
> +                         AV_CH_TOP_BACK_LEFT,
> +                         AV_CH_TOP_BACK_RIGHT,
> +                         AAC_CHANNEL_BACK);
> +        e2c_vec[i] = (struct elem_to_channel) {
> +            .av_position  = AV_CH_TOP_BACK_CENTER,
> +            .syn_ele      = layout_map[i][0],

Does this code assume all types are CPE ?
because if thats not true i can exceed the tags


> +            .elem_id      = layout_map[i][1],
> +            .aac_position = layout_map[i][2]
> +        }; i++;
> +        e2c_vec[i] = (struct elem_to_channel) {
> +            .av_position  = AV_CH_BOTTOM_FRONT_CENTER,
> +            .syn_ele      = layout_map[i][0],
> +            .elem_id      = layout_map[i][1],
> +            .aac_position = layout_map[i][2]
> +        }; i++;
> +        i += assign_pair(e2c_vec, layout_map, i,
> +                         AV_CH_BOTTOM_FRONT_LEFT,
> +                         AV_CH_BOTTOM_FRONT_RIGHT,
> +                         AAC_CHANNEL_FRONT);
> +    }
> +
>      total_non_cc_elements = n = i;
> -    do {
> -        int next_n = 0;
> -        for (i = 1; i < n; i++)
> -            if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
> -                FFSWAP(struct elem_to_channel, e2c_vec[i - 1], e2c_vec[i]);
> -                next_n = i;
> -            }
> -        n = next_n;
> -    } while (n > 0);
> +
> +    if (tags == 16 && total_non_cc_elements == 16) {
> +        // For 22.2 reorder the result as needed
> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[0]);   // FL & FR first (final), FC third
> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[1]);   // FC second (final), FLc & FRc third
> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[2]);   // LFE1 third (final), FLc & FRc seventh
> +        FFSWAP(struct elem_to_channel, e2c_vec[4], e2c_vec[3]);   // BL & BR fourth (final), SiL & SiR fifth
> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[4]);   // FLc & FRc fifth (final), SiL & SiR seventh
> +        FFSWAP(struct elem_to_channel, e2c_vec[7], e2c_vec[6]);   // LFE2 seventh (final), SiL & SiR eight (final)
> +        FFSWAP(struct elem_to_channel, e2c_vec[9], e2c_vec[8]);   // TpFL & TpFR ninth (final), TFC tenth (final)
> +        FFSWAP(struct elem_to_channel, e2c_vec[11], e2c_vec[10]); // TC eleventh (final), TpSiL & TpSiR twelth
> +        FFSWAP(struct elem_to_channel, e2c_vec[12], e2c_vec[11]); // TpBL & TpBR twelth (final), TpSiL & TpSiR thirteenth (final)

Does this not need to check the assumtations from the comments ?

thx

[...]
Paul B Mahol Aug. 18, 2020, 9:17 a.m. UTC | #2
I think there is open bug report about SEGV regarding this commit on trac.

Please revert or fix ASAP!

On 8/16/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Aug 01, 2020 at 02:07:30PM +0300, Jan Ekström wrote:
>> ---
>>  libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
>>  1 file changed, 78 insertions(+), 11 deletions(-)
>>
>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>> index 21db12fdab..8c5048cc13 100644
>> --- a/libavcodec/aacdec_template.c
>> +++ b/libavcodec/aacdec_template.c
>> @@ -387,17 +387,77 @@ static uint64_t sniff_channel_order(uint8_t
>> (*layout_map)[3], int tags)
>>          i++;
>>      }
>>
>> -    // Must choose a stable sort
>> +    // The previous checks would end up at 8 at this point for 22.2
>> +    if (tags == 16 && i == 8) {
>> +        e2c_vec[i] = (struct elem_to_channel) {
>> +            .av_position  = AV_CH_TOP_FRONT_CENTER,
>> +            .syn_ele      = layout_map[i][0],
>> +            .elem_id      = layout_map[i][1],
>> +            .aac_position = layout_map[i][2]
>> +        }; i++;
>> +        i += assign_pair(e2c_vec, layout_map, i,
>> +                         AV_CH_TOP_FRONT_LEFT,
>> +                         AV_CH_TOP_FRONT_RIGHT,
>> +                         AAC_CHANNEL_FRONT);
>> +        i += assign_pair(e2c_vec, layout_map, i,
>> +                         AV_CH_TOP_SIDE_LEFT,
>> +                         AV_CH_TOP_SIDE_RIGHT,
>> +                         AAC_CHANNEL_SIDE);
>> +        e2c_vec[i] = (struct elem_to_channel) {
>> +            .av_position  = AV_CH_TOP_CENTER,
>> +            .syn_ele      = layout_map[i][0],
>> +            .elem_id      = layout_map[i][1],
>> +            .aac_position = layout_map[i][2]
>> +        }; i++;
>> +        i += assign_pair(e2c_vec, layout_map, i,
>> +                         AV_CH_TOP_BACK_LEFT,
>> +                         AV_CH_TOP_BACK_RIGHT,
>> +                         AAC_CHANNEL_BACK);
>> +        e2c_vec[i] = (struct elem_to_channel) {
>> +            .av_position  = AV_CH_TOP_BACK_CENTER,
>> +            .syn_ele      = layout_map[i][0],
>
> Does this code assume all types are CPE ?
> because if thats not true i can exceed the tags
>
>
>> +            .elem_id      = layout_map[i][1],
>> +            .aac_position = layout_map[i][2]
>> +        }; i++;
>> +        e2c_vec[i] = (struct elem_to_channel) {
>> +            .av_position  = AV_CH_BOTTOM_FRONT_CENTER,
>> +            .syn_ele      = layout_map[i][0],
>> +            .elem_id      = layout_map[i][1],
>> +            .aac_position = layout_map[i][2]
>> +        }; i++;
>> +        i += assign_pair(e2c_vec, layout_map, i,
>> +                         AV_CH_BOTTOM_FRONT_LEFT,
>> +                         AV_CH_BOTTOM_FRONT_RIGHT,
>> +                         AAC_CHANNEL_FRONT);
>> +    }
>> +
>>      total_non_cc_elements = n = i;
>> -    do {
>> -        int next_n = 0;
>> -        for (i = 1; i < n; i++)
>> -            if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
>> -                FFSWAP(struct elem_to_channel, e2c_vec[i - 1],
>> e2c_vec[i]);
>> -                next_n = i;
>> -            }
>> -        n = next_n;
>> -    } while (n > 0);
>> +
>> +    if (tags == 16 && total_non_cc_elements == 16) {
>> +        // For 22.2 reorder the result as needed
>> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[0]);   // FL &
>> FR first (final), FC third
>> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[1]);   // FC
>> second (final), FLc & FRc third
>> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[2]);   // LFE1
>> third (final), FLc & FRc seventh
>> +        FFSWAP(struct elem_to_channel, e2c_vec[4], e2c_vec[3]);   // BL &
>> BR fourth (final), SiL & SiR fifth
>> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[4]);   // FLc
>> & FRc fifth (final), SiL & SiR seventh
>> +        FFSWAP(struct elem_to_channel, e2c_vec[7], e2c_vec[6]);   // LFE2
>> seventh (final), SiL & SiR eight (final)
>> +        FFSWAP(struct elem_to_channel, e2c_vec[9], e2c_vec[8]);   // TpFL
>> & TpFR ninth (final), TFC tenth (final)
>> +        FFSWAP(struct elem_to_channel, e2c_vec[11], e2c_vec[10]); // TC
>> eleventh (final), TpSiL & TpSiR twelth
>> +        FFSWAP(struct elem_to_channel, e2c_vec[12], e2c_vec[11]); // TpBL
>> & TpBR twelth (final), TpSiL & TpSiR thirteenth (final)
>
> Does this not need to check the assumtations from the comments ?
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Awnsering whenever a program halts or runs forever is
> On a turing machine, in general impossible (turings halting problem).
> On any real computer, always possible as a real computer has a finite
> number
> of states N, and will either halt in less than N cycles or never halt.
>
Jan Ekström Aug. 18, 2020, 9:45 a.m. UTC | #3
On Sun, Aug 16, 2020 at 2:15 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Aug 01, 2020 at 02:07:30PM +0300, Jan Ekström wrote:
> > ---
> >  libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 78 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> > index 21db12fdab..8c5048cc13 100644
> > --- a/libavcodec/aacdec_template.c
> > +++ b/libavcodec/aacdec_template.c
> > @@ -387,17 +387,77 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags)
> >          i++;
> >      }
> >
> > -    // Must choose a stable sort
> > +    // The previous checks would end up at 8 at this point for 22.2
> > +    if (tags == 16 && i == 8) {
> > +        e2c_vec[i] = (struct elem_to_channel) {
> > +            .av_position  = AV_CH_TOP_FRONT_CENTER,
> > +            .syn_ele      = layout_map[i][0],
> > +            .elem_id      = layout_map[i][1],
> > +            .aac_position = layout_map[i][2]
> > +        }; i++;
> > +        i += assign_pair(e2c_vec, layout_map, i,
> > +                         AV_CH_TOP_FRONT_LEFT,
> > +                         AV_CH_TOP_FRONT_RIGHT,
> > +                         AAC_CHANNEL_FRONT);
> > +        i += assign_pair(e2c_vec, layout_map, i,
> > +                         AV_CH_TOP_SIDE_LEFT,
> > +                         AV_CH_TOP_SIDE_RIGHT,
> > +                         AAC_CHANNEL_SIDE);
> > +        e2c_vec[i] = (struct elem_to_channel) {
> > +            .av_position  = AV_CH_TOP_CENTER,
> > +            .syn_ele      = layout_map[i][0],
> > +            .elem_id      = layout_map[i][1],
> > +            .aac_position = layout_map[i][2]
> > +        }; i++;
> > +        i += assign_pair(e2c_vec, layout_map, i,
> > +                         AV_CH_TOP_BACK_LEFT,
> > +                         AV_CH_TOP_BACK_RIGHT,
> > +                         AAC_CHANNEL_BACK);
> > +        e2c_vec[i] = (struct elem_to_channel) {
> > +            .av_position  = AV_CH_TOP_BACK_CENTER,
> > +            .syn_ele      = layout_map[i][0],
>
> Does this code assume all types are CPE ?
> because if thats not true i can exceed the tags
>

Sorry for responding late, I have been tired and didn't find a good
spot to write things down.

No, it should not assume that all types are CPE. 22.2 utilizes both
CPE, SCE as well as LFE. The full definition of 22.2 was added in
93a2913ac8a3aa25c05fd30036da89cb493e68ee with each coding element
being documented as it is in the spec.

OK, then I did misunderstand what exactly the `tags` variable means
and how it can go over (if that is a problem other than 22.2 with more
coding elements than in standard samples not hitting this logic).

>
> > +            .elem_id      = layout_map[i][1],
> > +            .aac_position = layout_map[i][2]
> > +        }; i++;
> > +        e2c_vec[i] = (struct elem_to_channel) {
> > +            .av_position  = AV_CH_BOTTOM_FRONT_CENTER,
> > +            .syn_ele      = layout_map[i][0],
> > +            .elem_id      = layout_map[i][1],
> > +            .aac_position = layout_map[i][2]
> > +        }; i++;
> > +        i += assign_pair(e2c_vec, layout_map, i,
> > +                         AV_CH_BOTTOM_FRONT_LEFT,
> > +                         AV_CH_BOTTOM_FRONT_RIGHT,
> > +                         AAC_CHANNEL_FRONT);
> > +    }
> > +
> >      total_non_cc_elements = n = i;
> > -    do {
> > -        int next_n = 0;
> > -        for (i = 1; i < n; i++)
> > -            if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
> > -                FFSWAP(struct elem_to_channel, e2c_vec[i - 1], e2c_vec[i]);
> > -                next_n = i;
> > -            }
> > -        n = next_n;
> > -    } while (n > 0);
> > +
> > +    if (tags == 16 && total_non_cc_elements == 16) {
> > +        // For 22.2 reorder the result as needed
> > +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[0]);   // FL & FR first (final), FC third
> > +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[1]);   // FC second (final), FLc & FRc third
> > +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[2]);   // LFE1 third (final), FLc & FRc seventh
> > +        FFSWAP(struct elem_to_channel, e2c_vec[4], e2c_vec[3]);   // BL & BR fourth (final), SiL & SiR fifth
> > +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[4]);   // FLc & FRc fifth (final), SiL & SiR seventh
> > +        FFSWAP(struct elem_to_channel, e2c_vec[7], e2c_vec[6]);   // LFE2 seventh (final), SiL & SiR eight (final)
> > +        FFSWAP(struct elem_to_channel, e2c_vec[9], e2c_vec[8]);   // TpFL & TpFR ninth (final), TFC tenth (final)
> > +        FFSWAP(struct elem_to_channel, e2c_vec[11], e2c_vec[10]); // TC eleventh (final), TpSiL & TpSiR twelth
> > +        FFSWAP(struct elem_to_channel, e2c_vec[12], e2c_vec[11]); // TpBL & TpBR twelth (final), TpSiL & TpSiR thirteenth (final)
>
> Does this not need to check the assumtations from the comments ?

Technically, if the input is not standard 22.2 AAC coding order, it
would lead to incorrect output order. I did wish for comments on this,
since I was torn between this and just rewriting the checks to not be
like the rest of the function and instead utilizing a hard-coded order
if the elements seemed to be there.

Additionally, I did think if I should utilize the identifier (13) for
additional checks, but the function so far completely ignored that and
based on tags.

Thus, I just attempted to implement it in a similar way as the rest of
the code in the function, and did not receive comments about it.

Jan

>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Awnsering whenever a program halts or runs forever is
> On a turing machine, in general impossible (turings halting problem).
> On any real computer, always possible as a real computer has a finite number
> of states N, and will either halt in less than N cycles or never halt.
> _______________________________________________
> 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".
Jan Ekström Aug. 18, 2020, 9:45 a.m. UTC | #4
On Tue, Aug 18, 2020 at 12:17 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> I think there is open bug report about SEGV regarding this commit on trac.
>
> Please revert or fix ASAP!
>

Thank you for letting me know about this. I will take a look at this
after work. Please do not yell at me, I am not your enemy.

Jan

> On 8/16/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sat, Aug 01, 2020 at 02:07:30PM +0300, Jan Ekström wrote:
> >> ---
> >>  libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
> >>  1 file changed, 78 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> >> index 21db12fdab..8c5048cc13 100644
> >> --- a/libavcodec/aacdec_template.c
> >> +++ b/libavcodec/aacdec_template.c
> >> @@ -387,17 +387,77 @@ static uint64_t sniff_channel_order(uint8_t
> >> (*layout_map)[3], int tags)
> >>          i++;
> >>      }
> >>
> >> -    // Must choose a stable sort
> >> +    // The previous checks would end up at 8 at this point for 22.2
> >> +    if (tags == 16 && i == 8) {
> >> +        e2c_vec[i] = (struct elem_to_channel) {
> >> +            .av_position  = AV_CH_TOP_FRONT_CENTER,
> >> +            .syn_ele      = layout_map[i][0],
> >> +            .elem_id      = layout_map[i][1],
> >> +            .aac_position = layout_map[i][2]
> >> +        }; i++;
> >> +        i += assign_pair(e2c_vec, layout_map, i,
> >> +                         AV_CH_TOP_FRONT_LEFT,
> >> +                         AV_CH_TOP_FRONT_RIGHT,
> >> +                         AAC_CHANNEL_FRONT);
> >> +        i += assign_pair(e2c_vec, layout_map, i,
> >> +                         AV_CH_TOP_SIDE_LEFT,
> >> +                         AV_CH_TOP_SIDE_RIGHT,
> >> +                         AAC_CHANNEL_SIDE);
> >> +        e2c_vec[i] = (struct elem_to_channel) {
> >> +            .av_position  = AV_CH_TOP_CENTER,
> >> +            .syn_ele      = layout_map[i][0],
> >> +            .elem_id      = layout_map[i][1],
> >> +            .aac_position = layout_map[i][2]
> >> +        }; i++;
> >> +        i += assign_pair(e2c_vec, layout_map, i,
> >> +                         AV_CH_TOP_BACK_LEFT,
> >> +                         AV_CH_TOP_BACK_RIGHT,
> >> +                         AAC_CHANNEL_BACK);
> >> +        e2c_vec[i] = (struct elem_to_channel) {
> >> +            .av_position  = AV_CH_TOP_BACK_CENTER,
> >> +            .syn_ele      = layout_map[i][0],
> >
> > Does this code assume all types are CPE ?
> > because if thats not true i can exceed the tags
> >
> >
> >> +            .elem_id      = layout_map[i][1],
> >> +            .aac_position = layout_map[i][2]
> >> +        }; i++;
> >> +        e2c_vec[i] = (struct elem_to_channel) {
> >> +            .av_position  = AV_CH_BOTTOM_FRONT_CENTER,
> >> +            .syn_ele      = layout_map[i][0],
> >> +            .elem_id      = layout_map[i][1],
> >> +            .aac_position = layout_map[i][2]
> >> +        }; i++;
> >> +        i += assign_pair(e2c_vec, layout_map, i,
> >> +                         AV_CH_BOTTOM_FRONT_LEFT,
> >> +                         AV_CH_BOTTOM_FRONT_RIGHT,
> >> +                         AAC_CHANNEL_FRONT);
> >> +    }
> >> +
> >>      total_non_cc_elements = n = i;
> >> -    do {
> >> -        int next_n = 0;
> >> -        for (i = 1; i < n; i++)
> >> -            if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
> >> -                FFSWAP(struct elem_to_channel, e2c_vec[i - 1],
> >> e2c_vec[i]);
> >> -                next_n = i;
> >> -            }
> >> -        n = next_n;
> >> -    } while (n > 0);
> >> +
> >> +    if (tags == 16 && total_non_cc_elements == 16) {
> >> +        // For 22.2 reorder the result as needed
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[0]);   // FL &
> >> FR first (final), FC third
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[1]);   // FC
> >> second (final), FLc & FRc third
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[2]);   // LFE1
> >> third (final), FLc & FRc seventh
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[4], e2c_vec[3]);   // BL &
> >> BR fourth (final), SiL & SiR fifth
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[4]);   // FLc
> >> & FRc fifth (final), SiL & SiR seventh
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[7], e2c_vec[6]);   // LFE2
> >> seventh (final), SiL & SiR eight (final)
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[9], e2c_vec[8]);   // TpFL
> >> & TpFR ninth (final), TFC tenth (final)
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[11], e2c_vec[10]); // TC
> >> eleventh (final), TpSiL & TpSiR twelth
> >> +        FFSWAP(struct elem_to_channel, e2c_vec[12], e2c_vec[11]); // TpBL
> >> & TpBR twelth (final), TpSiL & TpSiR thirteenth (final)
> >
> > Does this not need to check the assumtations from the comments ?
> >
> > thx
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Awnsering whenever a program halts or runs forever is
> > On a turing machine, in general impossible (turings halting problem).
> > On any real computer, always possible as a real computer has a finite
> > number
> > of states N, and will either halt in less than N cycles or never halt.
> >
> _______________________________________________
> 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".
Michael Niedermayer Aug. 18, 2020, 12:39 p.m. UTC | #5
On Tue, Aug 18, 2020 at 12:45:17PM +0300, Jan Ekström wrote:
> On Sun, Aug 16, 2020 at 2:15 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sat, Aug 01, 2020 at 02:07:30PM +0300, Jan Ekström wrote:
> > > ---
> > >  libavcodec/aacdec_template.c | 89 +++++++++++++++++++++++++++++++-----
> > >  1 file changed, 78 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> > > index 21db12fdab..8c5048cc13 100644
> > > --- a/libavcodec/aacdec_template.c
> > > +++ b/libavcodec/aacdec_template.c
> > > @@ -387,17 +387,77 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags)
> > >          i++;
> > >      }
> > >
> > > -    // Must choose a stable sort
> > > +    // The previous checks would end up at 8 at this point for 22.2
> > > +    if (tags == 16 && i == 8) {
> > > +        e2c_vec[i] = (struct elem_to_channel) {
> > > +            .av_position  = AV_CH_TOP_FRONT_CENTER,
> > > +            .syn_ele      = layout_map[i][0],
> > > +            .elem_id      = layout_map[i][1],
> > > +            .aac_position = layout_map[i][2]
> > > +        }; i++;
> > > +        i += assign_pair(e2c_vec, layout_map, i,
> > > +                         AV_CH_TOP_FRONT_LEFT,
> > > +                         AV_CH_TOP_FRONT_RIGHT,
> > > +                         AAC_CHANNEL_FRONT);
> > > +        i += assign_pair(e2c_vec, layout_map, i,
> > > +                         AV_CH_TOP_SIDE_LEFT,
> > > +                         AV_CH_TOP_SIDE_RIGHT,
> > > +                         AAC_CHANNEL_SIDE);
> > > +        e2c_vec[i] = (struct elem_to_channel) {
> > > +            .av_position  = AV_CH_TOP_CENTER,
> > > +            .syn_ele      = layout_map[i][0],
> > > +            .elem_id      = layout_map[i][1],
> > > +            .aac_position = layout_map[i][2]
> > > +        }; i++;
> > > +        i += assign_pair(e2c_vec, layout_map, i,
> > > +                         AV_CH_TOP_BACK_LEFT,
> > > +                         AV_CH_TOP_BACK_RIGHT,
> > > +                         AAC_CHANNEL_BACK);
> > > +        e2c_vec[i] = (struct elem_to_channel) {
> > > +            .av_position  = AV_CH_TOP_BACK_CENTER,
> > > +            .syn_ele      = layout_map[i][0],
> >
> > Does this code assume all types are CPE ?
> > because if thats not true i can exceed the tags
> >
> 
> Sorry for responding late, I have been tired and didn't find a good
> spot to write things down.
> 
> No, it should not assume that all types are CPE. 22.2 utilizes both
> CPE, SCE as well as LFE. The full definition of 22.2 was added in
> 93a2913ac8a3aa25c05fd30036da89cb493e68ee with each coding element
> being documented as it is in the spec.
> 
> OK, then I did misunderstand what exactly the `tags` variable means
> and how it can go over (if that is a problem other than 22.2 with more
> coding elements than in standard samples not hitting this logic).

the fuzzer found a case where the layout_map beyond tags seems to contain
apparent bad values (different in each run and way too large)
the code then eventually crashes. 
I did not investigate this further as you seem to be activly working on
22.2. 
I think you already have some testcases but i can provide the fuzzer testcase
if you want another one

thx

[...]
Jan Ekström Aug. 18, 2020, 1:47 p.m. UTC | #6
On Tue, Aug 18, 2020 at 12:45 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Tue, Aug 18, 2020 at 12:17 PM Paul B Mahol <onemda@gmail.com> wrote:
> >
> > I think there is open bug report about SEGV regarding this commit on trac.
> >
> > Please revert or fix ASAP!
> >
>
> Thank you for letting me know about this. I will take a look at this
> after work. Please do not yell at me, I am not your enemy.
>
> Jan
>

If this is about https://trac.ffmpeg.org/ticket/8845 , that is a
*fuzzer* sample causing issues.

While it is not nice, and I *am* now looking at it, why on earth do
you have to go to this "revert or fix ASAP!" drama?

It is not a normal stream, nor is it a FATE test I happened to break
(I did check before pushing that my code passes existing FATE tests,
and I was planning on adding a new one as I got through everything
else I've been poking around). Thus, I think you can bring such things
to my knowledge in a less aggressive way. I hope you understand this
point.

Jan
Paul B Mahol Aug. 18, 2020, 4:44 p.m. UTC | #7
On 8/18/20, Jan Ekström <jeebjp@gmail.com> wrote:
> On Tue, Aug 18, 2020 at 12:45 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>> On Tue, Aug 18, 2020 at 12:17 PM Paul B Mahol <onemda@gmail.com> wrote:
>> >
>> > I think there is open bug report about SEGV regarding this commit on
>> > trac.
>> >
>> > Please revert or fix ASAP!
>> >
>>
>> Thank you for letting me know about this. I will take a look at this
>> after work. Please do not yell at me, I am not your enemy.
>>
>> Jan
>>
>
> If this is about https://trac.ffmpeg.org/ticket/8845 , that is a
> *fuzzer* sample causing issues.
>
> While it is not nice, and I *am* now looking at it, why on earth do
> you have to go to this "revert or fix ASAP!" drama?

Because open security issues are never ok.
Sorry for disturbing you.

>
> It is not a normal stream, nor is it a FATE test I happened to break
> (I did check before pushing that my code passes existing FATE tests,
> and I was planning on adding a new one as I got through everything
> else I've been poking around). Thus, I think you can bring such things
> to my knowledge in a less aggressive way. I hope you understand this
> point.
>
> Jan
> _______________________________________________
> 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".
Jan Ekström Aug. 19, 2020, 2:06 p.m. UTC | #8
On Tue, Aug 18, 2020 at 7:45 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 8/18/20, Jan Ekström <jeebjp@gmail.com> wrote:
> > On Tue, Aug 18, 2020 at 12:45 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >>
> >> On Tue, Aug 18, 2020 at 12:17 PM Paul B Mahol <onemda@gmail.com> wrote:
> >> >
> >> > I think there is open bug report about SEGV regarding this commit on
> >> > trac.
> >> >
> >> > Please revert or fix ASAP!
> >> >
> >>
> >> Thank you for letting me know about this. I will take a look at this
> >> after work. Please do not yell at me, I am not your enemy.
> >>
> >> Jan
> >>
> >
> > If this is about https://trac.ffmpeg.org/ticket/8845 , that is a
> > *fuzzer* sample causing issues.
> >
> > While it is not nice, and I *am* now looking at it, why on earth do
> > you have to go to this "revert or fix ASAP!" drama?
>
> Because open security issues are never ok.
> Sorry for disturbing you.
>

It's OK, you just would have gotten my attention quicker in other ways
if you wanted to get that.

For future reference, please keep in mind that I do attempt to not do
public hurrying or shaming in a very public media unless I feel there
is clearly no other way forward, and thus I do get upset if people
consider me as someone requiring such behavior, as I consider myself
being able to respond to queries relatively quickly and to not create
complete breakages in master/releases and leave them be even to the
disdain of others and with no response to queries.

You can usually get in touch with me more quickly on IRC, ML requires
me to a) open it up and b) have enough time on a non-dayjob related
machine to word a proper response.

Jan
diff mbox series

Patch

diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 21db12fdab..8c5048cc13 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -387,17 +387,77 @@  static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags)
         i++;
     }
 
-    // Must choose a stable sort
+    // The previous checks would end up at 8 at this point for 22.2
+    if (tags == 16 && i == 8) {
+        e2c_vec[i] = (struct elem_to_channel) {
+            .av_position  = AV_CH_TOP_FRONT_CENTER,
+            .syn_ele      = layout_map[i][0],
+            .elem_id      = layout_map[i][1],
+            .aac_position = layout_map[i][2]
+        }; i++;
+        i += assign_pair(e2c_vec, layout_map, i,
+                         AV_CH_TOP_FRONT_LEFT,
+                         AV_CH_TOP_FRONT_RIGHT,
+                         AAC_CHANNEL_FRONT);
+        i += assign_pair(e2c_vec, layout_map, i,
+                         AV_CH_TOP_SIDE_LEFT,
+                         AV_CH_TOP_SIDE_RIGHT,
+                         AAC_CHANNEL_SIDE);
+        e2c_vec[i] = (struct elem_to_channel) {
+            .av_position  = AV_CH_TOP_CENTER,
+            .syn_ele      = layout_map[i][0],
+            .elem_id      = layout_map[i][1],
+            .aac_position = layout_map[i][2]
+        }; i++;
+        i += assign_pair(e2c_vec, layout_map, i,
+                         AV_CH_TOP_BACK_LEFT,
+                         AV_CH_TOP_BACK_RIGHT,
+                         AAC_CHANNEL_BACK);
+        e2c_vec[i] = (struct elem_to_channel) {
+            .av_position  = AV_CH_TOP_BACK_CENTER,
+            .syn_ele      = layout_map[i][0],
+            .elem_id      = layout_map[i][1],
+            .aac_position = layout_map[i][2]
+        }; i++;
+        e2c_vec[i] = (struct elem_to_channel) {
+            .av_position  = AV_CH_BOTTOM_FRONT_CENTER,
+            .syn_ele      = layout_map[i][0],
+            .elem_id      = layout_map[i][1],
+            .aac_position = layout_map[i][2]
+        }; i++;
+        i += assign_pair(e2c_vec, layout_map, i,
+                         AV_CH_BOTTOM_FRONT_LEFT,
+                         AV_CH_BOTTOM_FRONT_RIGHT,
+                         AAC_CHANNEL_FRONT);
+    }
+
     total_non_cc_elements = n = i;
-    do {
-        int next_n = 0;
-        for (i = 1; i < n; i++)
-            if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
-                FFSWAP(struct elem_to_channel, e2c_vec[i - 1], e2c_vec[i]);
-                next_n = i;
-            }
-        n = next_n;
-    } while (n > 0);
+
+    if (tags == 16 && total_non_cc_elements == 16) {
+        // For 22.2 reorder the result as needed
+        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[0]);   // FL & FR first (final), FC third
+        FFSWAP(struct elem_to_channel, e2c_vec[2], e2c_vec[1]);   // FC second (final), FLc & FRc third
+        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[2]);   // LFE1 third (final), FLc & FRc seventh
+        FFSWAP(struct elem_to_channel, e2c_vec[4], e2c_vec[3]);   // BL & BR fourth (final), SiL & SiR fifth
+        FFSWAP(struct elem_to_channel, e2c_vec[6], e2c_vec[4]);   // FLc & FRc fifth (final), SiL & SiR seventh
+        FFSWAP(struct elem_to_channel, e2c_vec[7], e2c_vec[6]);   // LFE2 seventh (final), SiL & SiR eight (final)
+        FFSWAP(struct elem_to_channel, e2c_vec[9], e2c_vec[8]);   // TpFL & TpFR ninth (final), TFC tenth (final)
+        FFSWAP(struct elem_to_channel, e2c_vec[11], e2c_vec[10]); // TC eleventh (final), TpSiL & TpSiR twelth
+        FFSWAP(struct elem_to_channel, e2c_vec[12], e2c_vec[11]); // TpBL & TpBR twelth (final), TpSiL & TpSiR thirteenth (final)
+    } else {
+        // For everything else, utilize the AV channel position define as a
+        // stable sort.
+        do {
+            int next_n = 0;
+            for (i = 1; i < n; i++)
+                if (e2c_vec[i - 1].av_position > e2c_vec[i].av_position) {
+                    FFSWAP(struct elem_to_channel, e2c_vec[i - 1], e2c_vec[i]);
+                    next_n = i;
+                }
+            n = next_n;
+        } while (n > 0);
+
+    }
 
     layout = 0;
     for (i = 0; i < total_non_cc_elements; i++) {
@@ -535,7 +595,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 > 12) {
+        channel_config > 13) {
         av_log(avctx, AV_LOG_ERROR,
                "invalid default channel configuration (%d)\n",
                channel_config);
@@ -615,6 +675,13 @@  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 13:
+        if (ac->tags_mapped > 3 && ((type == TYPE_CPE && elem_id < 8) ||
+                                    (type == TYPE_SCE && elem_id < 6) ||
+                                    (type == TYPE_LFE && elem_id < 2))) {
+            ac->tags_mapped++;
+            return ac->tag_che_map[type][elem_id] = ac->che[type][elem_id];
+        }
     case 12:
     case 7:
         if (ac->tags_mapped == 3 && type == TYPE_CPE) {