[FFmpeg-devel] [PATCH 2/3 v2] avcodec/aacdec_template: add more checks to make sure only 22.2 gets to 22.2
Jan Ekström
jeebjp at gmail.com
Sat Aug 22 17:01:05 EEST 2020
On Sat, Aug 22, 2020 at 4:44 PM Michael Niedermayer
<michael at 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 at 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
More information about the ffmpeg-devel
mailing list