diff mbox series

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

Message ID 20200822203954.4940-1-jeebjp@gmail.com
State Accepted
Headers show
Series None | expand

Commit Message

Jan Ekström Aug. 22, 2020, 8:39 p.m. UTC 
Validates the set channel layout as well as verifies that the received
layout to the function matches the reference layout, so that it matches
the implemented re-ordering logic.

Fixes #8845
---
 libavcodec/aacdec_template.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Carl Eugen Hoyos Aug. 22, 2020, 8:42 p.m. UTC | #1 
Am Sa., 22. Aug. 2020 um 22:40 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>
> Validates the set channel layout as well as verifies that the received
> layout to the function matches the reference layout, so that it matches
> the implemented re-ordering logic.
>
> Fixes #8845

Did you also see tickets #8859 and #8860?

Carl Eugen
Andreas Rheinhardt Aug. 22, 2020, 8:42 p.m. UTC | #2 
Jan Ekström:
> Validates the set channel layout as well as verifies that the received
> layout to the function matches the reference layout, so that it matches
> the implemented re-ordering logic.
> 
> Fixes #8845

What about the new tickets #8859, #8860?

- Andreas
Jan Ekström Aug. 22, 2020, 8:49 p.m. UTC | #3 
On Sat, Aug 22, 2020 at 11:42 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Sa., 22. Aug. 2020 um 22:40 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >
> > Validates the set channel layout as well as verifies that the received
> > layout to the function matches the reference layout, so that it matches
> > the implemented re-ordering logic.
> >
> > Fixes #8845
>
> Did you also see tickets #8859 and #8860?
>

No, but I just tested them and each and every version of the patch set
on the mailing list starting from the 18th did indeed make them not
happen, incl. the current one.

Jan
Jan Ekström Aug. 22, 2020, 9:08 p.m. UTC | #4 
On Sat, Aug 22, 2020 at 11:39 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Validates the set channel layout as well as verifies that the received
> layout to the function matches the reference layout, so that it matches
> the implemented re-ordering logic.
>
> Fixes #8845
> ---
>  libavcodec/aacdec_template.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index 9f7016790e..6473de4ee9 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,60 @@ 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) {
> +        const uint8_t (*reference_layout_map)[3] = aac_channel_layout_map[12];
> +        for (int j = 0; 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;
> +        }
> +
>          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++;
> +

These new newlines are extraneous now that the checks before each
assignment are gone.

I don't really care either way at this point, but I did just locally
clean them up - to just make the diff smaller (and to only contain the
bit that matters).

Jan
Carl Eugen Hoyos Aug. 22, 2020, 9:20 p.m. UTC | #5 
Am Sa., 22. Aug. 2020 um 23:15 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>
> On Sat, Aug 22, 2020 at 11:42 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >
> > Am Sa., 22. Aug. 2020 um 22:40 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> > >
> > > Validates the set channel layout as well as verifies that the received
> > > layout to the function matches the reference layout, so that it matches
> > > the implemented re-ordering logic.
> > >
> > > Fixes #8845
> >
> > Did you also see tickets #8859 and #8860?
> >
>
> No, but I just tested them and each and every version of the patch set
> on the mailing list starting from the 18th did indeed make them not
> happen, incl. the current one.

Thank you for confirming this!

Carl Eugen
Paul B Mahol Aug. 24, 2020, 8:04 p.m. UTC | #6 
On 8/22/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am Sa., 22. Aug. 2020 um 23:15 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
>>
>> On Sat, Aug 22, 2020 at 11:42 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> wrote:
>> >
>> > Am Sa., 22. Aug. 2020 um 22:40 Uhr schrieb Jan Ekström
>> > <jeebjp@gmail.com>:
>> > >
>> > > Validates the set channel layout as well as verifies that the received
>> > > layout to the function matches the reference layout, so that it
>> > > matches
>> > > the implemented re-ordering logic.
>> > >
>> > > Fixes #8845
>> >
>> > Did you also see tickets #8859 and #8860?
>> >
>>
>> No, but I just tested them and each and every version of the patch set
>> on the mailing list starting from the 18th did indeed make them not
>> happen, incl. the current one.
>
> Thank you for confirming this!

Patches should be fine.

There is only one way to find if that patch breaks good/normal files.

>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jan Ekström Aug. 24, 2020, 9:36 p.m. UTC | #7 
On Mon, Aug 24, 2020 at 11:04 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 8/22/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > Am Sa., 22. Aug. 2020 um 23:15 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> >>
> >> On Sat, Aug 22, 2020 at 11:42 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> >> wrote:
> >> >
> >> > Am Sa., 22. Aug. 2020 um 22:40 Uhr schrieb Jan Ekström
> >> > <jeebjp@gmail.com>:
> >> > >
> >> > > Validates the set channel layout as well as verifies that the received
> >> > > layout to the function matches the reference layout, so that it
> >> > > matches
> >> > > the implemented re-ordering logic.
> >> > >
> >> > > Fixes #8845
> >> >
> >> > Did you also see tickets #8859 and #8860?
> >> >
> >>
> >> No, but I just tested them and each and every version of the patch set
> >> on the mailing list starting from the 18th did indeed make them not
> >> happen, incl. the current one.
> >
> > Thank you for confirming this!
>
> Patches should be fine.
>
> There is only one way to find if that patch breaks good/normal files.
>

Thanks for taking a look. Applied patches 1 and 2 as
de1d6aa0a3d4358e9975e0d6fe5c655fd38694fe and
d6f293353c94c7ce200f6e0975ae3de49787f91f.

With 1/2 while doing final verifications I noticed I had accidentally
left one case of setting layout to UINT64_MAX, so there is a small
diff to the posted version:
diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 10ceeca261..5906ed20b3 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -410,15 +410,14 @@ static uint64_t sniff_channel_order(uint8_t
(*layout_map)[3], int tags)
     while (i < tags && layout_map[i][2] == AAC_CHANNEL_LFE) {
         e2c_vec[i] = (struct elem_to_channel) {
             .av_position  = UINT64_MAX,
             .syn_ele      = TYPE_LFE,
             .elem_id      = layout_map[i][1],
             .aac_position = AAC_CHANNEL_LFE
         };
-        layout |= e2c_vec[i].av_position;
         i++;
     }

     // The previous checks would end up at 8 at this point for 22.2
     if (layout == PREFIX_FOR_22POINT2 && tags == 16 && i == 8) {
         const uint8_t (*reference_layout_map)[3] = aac_channel_layout_map[12];
         for (int j = 0; j < tags; j++) {

UINT64_MAX is not supposed to be applied to the layout bit mask, so
this was just a mistake by me while making the original patch.

As for real life cases, the only files I've seen so far beyond 10.2
have been 22.2 broadcast samples, and those still successfully get
parsed as they just have channel_config = 13.

Jan
diff mbox series

Patch

diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 9f7016790e..6473de4ee9 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,60 @@  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) {
+        const uint8_t (*reference_layout_map)[3] = aac_channel_layout_map[12];
+        for (int j = 0; 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;
+        }
+
         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++;
+
         i += assign_pair(e2c_vec, layout_map, i,
                          AV_CH_TOP_FRONT_LEFT,
                          AV_CH_TOP_FRONT_RIGHT,
                          AAC_CHANNEL_FRONT,
                          &layout);
+
         i += assign_pair(e2c_vec, layout_map, i,
                          AV_CH_TOP_SIDE_LEFT,
                          AV_CH_TOP_SIDE_RIGHT,
                          AAC_CHANNEL_SIDE,
                          &layout);
+
         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++;
+
         i += assign_pair(e2c_vec, layout_map, i,
                          AV_CH_TOP_BACK_LEFT,
                          AV_CH_TOP_BACK_RIGHT,
                          AAC_CHANNEL_BACK,
                          &layout);
+
         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++;
+
         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++;
+
         i += assign_pair(e2c_vec, layout_map, i,
                          AV_CH_BOTTOM_FRONT_LEFT,
                          AV_CH_BOTTOM_FRONT_RIGHT,
@@ -449,9 +464,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