| Message ID | 20200801110730.30642-8-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | 22.2 channel layout support for AAC decoding | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 [...]
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. >
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".
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".
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 [...]
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
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".
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 --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) {