diff mbox series

[FFmpeg-devel,2/3,v2] avcodec/aacdec_template: add more checks to make sure only 22.2 gets to 22.2

Message ID 20200821215800.27162-3-jeebjp@gmail.com
State Superseded
Headers show
Series avcodec/aacdec_template: improvements to 22.2 layout logic | expand

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. 21, 2020, 9:57 p.m. UTC 
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(-)

Comments

Michael Niedermayer Aug. 22, 2020, 11:17 a.m. UTC | #1 
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

[...]
Jan Ekström Aug. 22, 2020, 11:36 a.m. UTC | #2 
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
Michael Niedermayer Aug. 22, 2020, 1:44 p.m. UTC | #3 
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

[...]
Jan Ekström Aug. 22, 2020, 2:01 p.m. UTC | #4 
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
Michael Niedermayer Aug. 22, 2020, 8:19 p.m. UTC | #5 
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 mbox series

Patch

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