[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 16:44:09 EEST 2020


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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/44304b8b/attachment.sig>


More information about the ffmpeg-devel mailing list