[FFmpeg-devel] [PATCH 2/3 v2] avcodec/aacdec_template: add more checks to make sure only 22.2 gets to 22.2
Michael Niedermayer
michael at niedermayer.cc
Sat Aug 22 23:19:12 EEST 2020
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 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.
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship: All citizens are under surveillance, all their steps and
actions recorded, for the politicians to enforce control.
Democracy: All politicians are under surveillance, all their steps and
actions recorded, for the citizens to enforce control.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200822/6013c9b9/attachment.sig>
More information about the ffmpeg-devel
mailing list