Message ID | 20200821215800.27162-3-jeebjp@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | avcodec/aacdec_template: improvements to 22.2 layout logic | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On Sat, Aug 22, 2020 at 12:57:59AM +0300, Jan Ekström wrote: > This way we can check that we have exactly the required things for 22.2. > > Fixes #8845 > --- > libavcodec/aacdec_template.c | 47 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c > index 9f7016790e..63604d39fd 100644 > --- a/libavcodec/aacdec_template.c > +++ b/libavcodec/aacdec_template.c > @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, > return num_pos_channels; > } > > +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) > static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > { > int i, n, total_non_cc_elements; > @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > } > > // The previous checks would end up at 8 at this point for 22.2 > - if (tags == 16 && i == 8) { > + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { > + if (layout_map[i][0] != TYPE_SCE || > + layout_map[i][2] != AAC_CHANNEL_FRONT) > + goto end_of_layout_definition; > + > 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] > }; layout |= e2c_vec[i].av_position; i++; > + > + if (layout_map[i][0] != TYPE_CPE || > + layout_map[i][2] != AAC_CHANNEL_FRONT) > + 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); > + > + if (layout_map[i][0] != TYPE_CPE || > + layout_map[i][2] != AAC_CHANNEL_SIDE) > + goto end_of_layout_definition; > + > i += assign_pair(e2c_vec, layout_map, i, > AV_CH_TOP_SIDE_LEFT, > AV_CH_TOP_SIDE_RIGHT, > AAC_CHANNEL_SIDE, > &layout); > + > + if (layout_map[i][0] != TYPE_SCE || > + layout_map[i][2] != AAC_CHANNEL_FRONT) > + goto end_of_layout_definition; > + > 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] > }; layout |= e2c_vec[i].av_position; i++; > + > + if (layout_map[i][0] != TYPE_CPE || > + layout_map[i][2] != AAC_CHANNEL_BACK) > + goto end_of_layout_definition; > + > i += assign_pair(e2c_vec, layout_map, i, > AV_CH_TOP_BACK_LEFT, > AV_CH_TOP_BACK_RIGHT, > AAC_CHANNEL_BACK, > &layout); > + > + if (layout_map[i][0] != TYPE_SCE || > + layout_map[i][2] != AAC_CHANNEL_BACK) > + goto end_of_layout_definition; > + > 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] > }; layout |= e2c_vec[i].av_position; i++; > + > + > + if (layout_map[i][0] != TYPE_SCE || > + layout_map[i][2] != AAC_CHANNEL_FRONT) > + goto end_of_layout_definition; > + > 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] > }; layout |= e2c_vec[i].av_position; i++; > + > + if (layout_map[i][0] != TYPE_CPE || > + layout_map[i][2] != AAC_CHANNEL_FRONT) > + goto end_of_layout_definition; > + > i += assign_pair(e2c_vec, layout_map, i, > AV_CH_BOTTOM_FRONT_LEFT, > AV_CH_BOTTOM_FRONT_RIGHT, > @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > &layout); > } > > +end_of_layout_definition: > + > total_non_cc_elements = n = i; Probably ok if you intend for partial matches to be accepted partially, because i think this would accept whatever comes before a mismatch but not after, but maybe iam missing something thx [...]
On Sat, Aug 22, 2020 at 2:17 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Aug 22, 2020 at 12:57:59AM +0300, Jan Ekström wrote: > > This way we can check that we have exactly the required things for 22.2. > > > > Fixes #8845 > > --- > > libavcodec/aacdec_template.c | 47 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c > > index 9f7016790e..63604d39fd 100644 > > --- a/libavcodec/aacdec_template.c > > +++ b/libavcodec/aacdec_template.c > > @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, > > return num_pos_channels; > > } > > > > +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) > > static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > { > > int i, n, total_non_cc_elements; > > @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > } > > > > // The previous checks would end up at 8 at this point for 22.2 > > - if (tags == 16 && i == 8) { > > + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { > > + if (layout_map[i][0] != TYPE_SCE || > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > + goto end_of_layout_definition; > > + > > 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] > > }; layout |= e2c_vec[i].av_position; i++; > > + > > + if (layout_map[i][0] != TYPE_CPE || > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > + 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); > > + > > + if (layout_map[i][0] != TYPE_CPE || > > + layout_map[i][2] != AAC_CHANNEL_SIDE) > > + goto end_of_layout_definition; > > + > > i += assign_pair(e2c_vec, layout_map, i, > > AV_CH_TOP_SIDE_LEFT, > > AV_CH_TOP_SIDE_RIGHT, > > AAC_CHANNEL_SIDE, > > &layout); > > + > > + if (layout_map[i][0] != TYPE_SCE || > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > + goto end_of_layout_definition; > > + > > 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] > > }; layout |= e2c_vec[i].av_position; i++; > > + > > + if (layout_map[i][0] != TYPE_CPE || > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > + goto end_of_layout_definition; > > + > > i += assign_pair(e2c_vec, layout_map, i, > > AV_CH_TOP_BACK_LEFT, > > AV_CH_TOP_BACK_RIGHT, > > AAC_CHANNEL_BACK, > > &layout); > > + > > + if (layout_map[i][0] != TYPE_SCE || > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > + goto end_of_layout_definition; > > + > > 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] > > }; layout |= e2c_vec[i].av_position; i++; > > + > > + > > + if (layout_map[i][0] != TYPE_SCE || > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > + goto end_of_layout_definition; > > + > > 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] > > }; layout |= e2c_vec[i].av_position; i++; > > + > > + if (layout_map[i][0] != TYPE_CPE || > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > + goto end_of_layout_definition; > > + > > i += assign_pair(e2c_vec, layout_map, i, > > AV_CH_BOTTOM_FRONT_LEFT, > > AV_CH_BOTTOM_FRONT_RIGHT, > > @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > &layout); > > } > > > > +end_of_layout_definition: > > + > > total_non_cc_elements = n = i; > > Probably ok if you intend for partial matches to be accepted partially, > because i think this would accept whatever comes before a mismatch but > not after, but maybe iam missing something > Yes, it will partially match things that get this far - although to get to this point to begin with you need to get 12/12 right and have the exact correct amount of tags and matches before that. Not to mention that a partial match will never end up in the 22.2 specific re-ordering logic. I can switch this to a really long if statement that goes through the next X channel coding elements if that's really required. For the original reasoning on this, I was just following the "check before assigning" style that was utilized earlier in the function, trying to mimic the same same style :P . Jan
On Sat, Aug 22, 2020 at 02:36:09PM +0300, Jan Ekström wrote: > On Sat, Aug 22, 2020 at 2:17 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Aug 22, 2020 at 12:57:59AM +0300, Jan Ekström wrote: > > > This way we can check that we have exactly the required things for 22.2. > > > > > > Fixes #8845 > > > --- > > > libavcodec/aacdec_template.c | 47 ++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c > > > index 9f7016790e..63604d39fd 100644 > > > --- a/libavcodec/aacdec_template.c > > > +++ b/libavcodec/aacdec_template.c > > > @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, > > > return num_pos_channels; > > > } > > > > > > +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) > > > static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > { > > > int i, n, total_non_cc_elements; > > > @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > } > > > > > > // The previous checks would end up at 8 at this point for 22.2 > > > - if (tags == 16 && i == 8) { > > > + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { > > > + if (layout_map[i][0] != TYPE_SCE || > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > + goto end_of_layout_definition; > > > + > > > 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] > > > }; layout |= e2c_vec[i].av_position; i++; > > > + > > > + if (layout_map[i][0] != TYPE_CPE || > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > + 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); > > > + > > > + if (layout_map[i][0] != TYPE_CPE || > > > + layout_map[i][2] != AAC_CHANNEL_SIDE) > > > + goto end_of_layout_definition; > > > + > > > i += assign_pair(e2c_vec, layout_map, i, > > > AV_CH_TOP_SIDE_LEFT, > > > AV_CH_TOP_SIDE_RIGHT, > > > AAC_CHANNEL_SIDE, > > > &layout); > > > + > > > + if (layout_map[i][0] != TYPE_SCE || > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > + goto end_of_layout_definition; > > > + > > > 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] > > > }; layout |= e2c_vec[i].av_position; i++; > > > + > > > + if (layout_map[i][0] != TYPE_CPE || > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > + goto end_of_layout_definition; > > > + > > > i += assign_pair(e2c_vec, layout_map, i, > > > AV_CH_TOP_BACK_LEFT, > > > AV_CH_TOP_BACK_RIGHT, > > > AAC_CHANNEL_BACK, > > > &layout); > > > + > > > + if (layout_map[i][0] != TYPE_SCE || > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > + goto end_of_layout_definition; > > > + > > > 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] > > > }; layout |= e2c_vec[i].av_position; i++; > > > + > > > + > > > + if (layout_map[i][0] != TYPE_SCE || > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > + goto end_of_layout_definition; > > > + > > > 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] > > > }; layout |= e2c_vec[i].av_position; i++; > > > + > > > + if (layout_map[i][0] != TYPE_CPE || > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > + goto end_of_layout_definition; > > > + > > > i += assign_pair(e2c_vec, layout_map, i, > > > AV_CH_BOTTOM_FRONT_LEFT, > > > AV_CH_BOTTOM_FRONT_RIGHT, > > > @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > &layout); > > > } > > > > > > +end_of_layout_definition: > > > + > > > total_non_cc_elements = n = i; > > > > Probably ok if you intend for partial matches to be accepted partially, > > because i think this would accept whatever comes before a mismatch but > > not after, but maybe iam missing something > > > > Yes, it will partially match things that get this far - although to > get to this point to begin with you need to get 12/12 right and have > the exact correct amount of tags and matches before that. Not to > mention that a partial match will never end up in the 22.2 specific > re-ordering logic. > > I can switch this to a really long if statement that goes through the > next X channel coding elements if that's really required. For the > original reasoning on this, I was just following the "check before > assigning" style that was utilized earlier in the function, trying to > mimic the same same style :P . The "check before assigning" style IIUC does continue with the next on failure. While the newly added code stops. I think the question is does this partial assigning make sense ? Does this produce something sensible if its hit. If it would make sense then it should be fine but if the resulting channels missing reordering or missing half make no sense then its maybe better not to do the assigning only half in such odd cases thx [...]
On Sat, Aug 22, 2020 at 4:44 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Aug 22, 2020 at 02:36:09PM +0300, Jan Ekström wrote: > > On Sat, Aug 22, 2020 at 2:17 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Sat, Aug 22, 2020 at 12:57:59AM +0300, Jan Ekström wrote: > > > > This way we can check that we have exactly the required things for 22.2. > > > > > > > > Fixes #8845 > > > > --- > > > > libavcodec/aacdec_template.c | 47 ++++++++++++++++++++++++++++++++++-- > > > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c > > > > index 9f7016790e..63604d39fd 100644 > > > > --- a/libavcodec/aacdec_template.c > > > > +++ b/libavcodec/aacdec_template.c > > > > @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, > > > > return num_pos_channels; > > > > } > > > > > > > > +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) > > > > static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > { > > > > int i, n, total_non_cc_elements; > > > > @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > } > > > > > > > > // The previous checks would end up at 8 at this point for 22.2 > > > > - if (tags == 16 && i == 8) { > > > > + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > + goto end_of_layout_definition; > > > > + > > > > 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] > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > + > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > + 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); > > > > + > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > + layout_map[i][2] != AAC_CHANNEL_SIDE) > > > > + goto end_of_layout_definition; > > > > + > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > AV_CH_TOP_SIDE_LEFT, > > > > AV_CH_TOP_SIDE_RIGHT, > > > > AAC_CHANNEL_SIDE, > > > > &layout); > > > > + > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > + goto end_of_layout_definition; > > > > + > > > > 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] > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > + > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > > + goto end_of_layout_definition; > > > > + > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > AV_CH_TOP_BACK_LEFT, > > > > AV_CH_TOP_BACK_RIGHT, > > > > AAC_CHANNEL_BACK, > > > > &layout); > > > > + > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > > + goto end_of_layout_definition; > > > > + > > > > 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] > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > + > > > > + > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > + goto end_of_layout_definition; > > > > + > > > > 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] > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > + > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > + goto end_of_layout_definition; > > > > + > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > AV_CH_BOTTOM_FRONT_LEFT, > > > > AV_CH_BOTTOM_FRONT_RIGHT, > > > > @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > &layout); > > > > } > > > > > > > > +end_of_layout_definition: > > > > + > > > > total_non_cc_elements = n = i; > > > > > > Probably ok if you intend for partial matches to be accepted partially, > > > because i think this would accept whatever comes before a mismatch but > > > not after, but maybe iam missing something > > > > > > > Yes, it will partially match things that get this far - although to > > get to this point to begin with you need to get 12/12 right and have > > the exact correct amount of tags and matches before that. Not to > > mention that a partial match will never end up in the 22.2 specific > > re-ordering logic. > > > > > I can switch this to a really long if statement that goes through the > > next X channel coding elements if that's really required. For the > > original reasoning on this, I was just following the "check before > > assigning" style that was utilized earlier in the function, trying to > > mimic the same same style :P . > > The "check before assigning" style IIUC does continue with the next on > failure. While the newly added code stops. > I think the question is does this partial assigning make sense ? > Does this produce something sensible if its hit. > If it would make sense then it should be fine but if the resulting > channels missing reordering or missing half make no sense then its > maybe better not to do the assigning only half in such odd cases > The channels wouldn't really make much more sense than the stuff already getting assigned in the primary part of the function for many a fuzzing sample. In order to not have a single very long if, and since I actually have the expected layout list defined in aacdectab.h , I think something like the following might work? const uint8_t (*reference_layout_map)[3] = aac_channel_layout_map[12]; for (int j = i; 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; } Would this be good enough for you? Jan
On Sat, Aug 22, 2020 at 05:01:05PM +0300, Jan Ekström wrote: > On Sat, Aug 22, 2020 at 4:44 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Aug 22, 2020 at 02:36:09PM +0300, Jan Ekström wrote: > > > On Sat, Aug 22, 2020 at 2:17 PM Michael Niedermayer > > > <michael@niedermayer.cc> wrote: > > > > > > > > On Sat, Aug 22, 2020 at 12:57:59AM +0300, Jan Ekström wrote: > > > > > This way we can check that we have exactly the required things for 22.2. > > > > > > > > > > Fixes #8845 > > > > > --- > > > > > libavcodec/aacdec_template.c | 47 ++++++++++++++++++++++++++++++++++-- > > > > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c > > > > > index 9f7016790e..63604d39fd 100644 > > > > > --- a/libavcodec/aacdec_template.c > > > > > +++ b/libavcodec/aacdec_template.c > > > > > @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, > > > > > return num_pos_channels; > > > > > } > > > > > > > > > > +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) > > > > > static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > > { > > > > > int i, n, total_non_cc_elements; > > > > > @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > > } > > > > > > > > > > // The previous checks would end up at 8 at this point for 22.2 > > > > > - if (tags == 16 && i == 8) { > > > > > + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { > > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > 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] > > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > > + > > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > > + 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); > > > > > + > > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > > + layout_map[i][2] != AAC_CHANNEL_SIDE) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > > AV_CH_TOP_SIDE_LEFT, > > > > > AV_CH_TOP_SIDE_RIGHT, > > > > > AAC_CHANNEL_SIDE, > > > > > &layout); > > > > > + > > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > 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] > > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > > + > > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > > AV_CH_TOP_BACK_LEFT, > > > > > AV_CH_TOP_BACK_RIGHT, > > > > > AAC_CHANNEL_BACK, > > > > > &layout); > > > > > + > > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > > + layout_map[i][2] != AAC_CHANNEL_BACK) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > 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] > > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > > + > > > > > + > > > > > + if (layout_map[i][0] != TYPE_SCE || > > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > 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] > > > > > }; layout |= e2c_vec[i].av_position; i++; > > > > > + > > > > > + if (layout_map[i][0] != TYPE_CPE || > > > > > + layout_map[i][2] != AAC_CHANNEL_FRONT) > > > > > + goto end_of_layout_definition; > > > > > + > > > > > i += assign_pair(e2c_vec, layout_map, i, > > > > > AV_CH_BOTTOM_FRONT_LEFT, > > > > > AV_CH_BOTTOM_FRONT_RIGHT, > > > > > @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) > > > > > &layout); > > > > > } > > > > > > > > > > +end_of_layout_definition: > > > > > + > > > > > total_non_cc_elements = n = i; > > > > > > > > Probably ok if you intend for partial matches to be accepted partially, > > > > because i think this would accept whatever comes before a mismatch but > > > > not after, but maybe iam missing something > > > > > > > > > > Yes, it will partially match things that get this far - although to > > > get to this point to begin with you need to get 12/12 right and have > > > the exact correct amount of tags and matches before that. Not to > > > mention that a partial match will never end up in the 22.2 specific > > > re-ordering logic. > > > > > > > > I can switch this to a really long if statement that goes through the > > > next X channel coding elements if that's really required. For the > > > original reasoning on this, I was just following the "check before > > > assigning" style that was utilized earlier in the function, trying to > > > mimic the same same style :P . > > > > The "check before assigning" style IIUC does continue with the next on > > failure. While the newly added code stops. > > I think the question is does this partial assigning make sense ? > > Does this produce something sensible if its hit. > > If it would make sense then it should be fine but if the resulting > > channels missing reordering or missing half make no sense then its > > maybe better not to do the assigning only half in such odd cases > > > > The channels wouldn't really make much more sense than the stuff > already getting assigned in the primary part of the function for many > a fuzzing sample. there are 2 things. For fuzzed samples anything is fine that doesnt lead to bugs/undefined behavior/crashes/security holes But i presume the code before 22.2 is writen as it is not because of fuzzed samples but because of odd real samples. ... > > In order to not have a single very long if, and since I actually have > the expected layout list defined in aacdectab.h , I think something > like the following might work? > > const uint8_t (*reference_layout_map)[3] = aac_channel_layout_map[12]; > for (int j = i; 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; > } > > Would this be good enough for you? this looks simpler and more robust, yes thanks [...]
diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c index 9f7016790e..63604d39fd 100644 --- a/libavcodec/aacdec_template.c +++ b/libavcodec/aacdec_template.c @@ -266,6 +266,7 @@ static int count_paired_channels(uint8_t (*layout_map)[3], int tags, int pos, return num_pos_channels; } +#define PREFIX_FOR_22POINT2 (AV_CH_LAYOUT_7POINT1_WIDE_BACK|AV_CH_BACK_CENTER|AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT|AV_CH_LOW_FREQUENCY_2) static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) { int i, n, total_non_cc_elements; @@ -402,46 +403,86 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) } // The previous checks would end up at 8 at this point for 22.2 - if (tags == 16 && i == 8) { + if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) { + if (layout_map[i][0] != TYPE_SCE || + layout_map[i][2] != AAC_CHANNEL_FRONT) + goto end_of_layout_definition; + 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] }; layout |= e2c_vec[i].av_position; i++; + + if (layout_map[i][0] != TYPE_CPE || + layout_map[i][2] != AAC_CHANNEL_FRONT) + 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); + + if (layout_map[i][0] != TYPE_CPE || + layout_map[i][2] != AAC_CHANNEL_SIDE) + goto end_of_layout_definition; + i += assign_pair(e2c_vec, layout_map, i, AV_CH_TOP_SIDE_LEFT, AV_CH_TOP_SIDE_RIGHT, AAC_CHANNEL_SIDE, &layout); + + if (layout_map[i][0] != TYPE_SCE || + layout_map[i][2] != AAC_CHANNEL_FRONT) + goto end_of_layout_definition; + 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] }; layout |= e2c_vec[i].av_position; i++; + + if (layout_map[i][0] != TYPE_CPE || + layout_map[i][2] != AAC_CHANNEL_BACK) + goto end_of_layout_definition; + i += assign_pair(e2c_vec, layout_map, i, AV_CH_TOP_BACK_LEFT, AV_CH_TOP_BACK_RIGHT, AAC_CHANNEL_BACK, &layout); + + if (layout_map[i][0] != TYPE_SCE || + layout_map[i][2] != AAC_CHANNEL_BACK) + goto end_of_layout_definition; + 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] }; layout |= e2c_vec[i].av_position; i++; + + + if (layout_map[i][0] != TYPE_SCE || + layout_map[i][2] != AAC_CHANNEL_FRONT) + goto end_of_layout_definition; + 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] }; layout |= e2c_vec[i].av_position; i++; + + if (layout_map[i][0] != TYPE_CPE || + layout_map[i][2] != AAC_CHANNEL_FRONT) + goto end_of_layout_definition; + i += assign_pair(e2c_vec, layout_map, i, AV_CH_BOTTOM_FRONT_LEFT, AV_CH_BOTTOM_FRONT_RIGHT, @@ -449,9 +490,11 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) &layout); } +end_of_layout_definition: + total_non_cc_elements = n = i; - if (tags == 16 && total_non_cc_elements == 16) { + if (layout == AV_CH_LAYOUT_22POINT2) { // 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