[FFmpeg-devel] [PATCH] avcodec/atrac3plus: reorder channels to match the output layout

[FFmpeg-devel] [PATCH] avcodec/atrac3plus: reorder channels to match the output layout

[James Almer]

The order in which the channels are coded in the bitstream do not always follow
the native, bitmask-based order of channels both signaled by the WAV container
and forced by this same decoder. This is the case with layouts containing an
LFE channel, as it's always coded last.

Fixes ticket #9964.

Signed-off-by: James Almer <jamrial@gmail.com>
---
6.1 and 7.1 untested, but much like 5.1 they were wrong before this change.

 libavcodec/atrac3plusdec.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
index ee71645a3c..8c3d20af76 100644
--- a/libavcodec/atrac3plusdec.c
+++ b/libavcodec/atrac3plusdec.c
@@ -65,6 +65,7 @@ typedef struct ATRAC3PContext {
 
     int num_channel_blocks;     ///< number of channel blocks
     uint8_t channel_blocks[5];  ///< channel configuration descriptor
+    AVChannelLayout coded_ch_layout; ///< order of channels as coded in the bitstream
 } ATRAC3PContext;
 
 static av_cold int atrac3p_decode_close(AVCodecContext *avctx)
@@ -74,6 +75,8 @@ static av_cold int atrac3p_decode_close(AVCodecContext *avctx)
     av_freep(&ctx->ch_units);
     av_freep(&ctx->fdsp);
 
+    av_channel_layout_uninit(&ctx->coded_ch_layout);
+
     ff_mdct_end(&ctx->mdct_ctx);
     ff_mdct_end(&ctx->ipqf_dct_ctx);
 
@@ -84,6 +87,7 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
                                       AVCodecContext *avctx)
 {
     int channels = avctx->ch_layout.nb_channels;
+    int ret;
     memset(ctx->channel_blocks, 0, sizeof(ctx->channel_blocks));
 
     av_channel_layout_uninit(&avctx->ch_layout);
@@ -92,17 +96,20 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_MONO;
         ctx->num_channel_blocks = 1;
         ctx->channel_blocks[0]  = CH_UNIT_MONO;
+        ctx->coded_ch_layout    = avctx->ch_layout;
         break;
     case 2:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_STEREO;
         ctx->num_channel_blocks = 1;
         ctx->channel_blocks[0]  = CH_UNIT_STEREO;
+        ctx->coded_ch_layout    = avctx->ch_layout;
         break;
     case 3:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_SURROUND;
         ctx->num_channel_blocks = 2;
         ctx->channel_blocks[0]  = CH_UNIT_STEREO;
         ctx->channel_blocks[1]  = CH_UNIT_MONO;
+        ctx->coded_ch_layout    = avctx->ch_layout;
         break;
     case 4:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_4POINT0;
@@ -110,6 +117,7 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         ctx->channel_blocks[0]  = CH_UNIT_STEREO;
         ctx->channel_blocks[1]  = CH_UNIT_MONO;
         ctx->channel_blocks[2]  = CH_UNIT_MONO;
+        ctx->coded_ch_layout    = avctx->ch_layout;
         break;
     case 6:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_5POINT1_BACK;
@@ -118,6 +126,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         ctx->channel_blocks[1]  = CH_UNIT_MONO;
         ctx->channel_blocks[2]  = CH_UNIT_STEREO;
         ctx->channel_blocks[3]  = CH_UNIT_MONO;
+        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+LFE");
+        if (ret < 0)
+            return ret;
         break;
     case 7:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_6POINT1_BACK;
@@ -127,6 +138,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         ctx->channel_blocks[2]  = CH_UNIT_STEREO;
         ctx->channel_blocks[3]  = CH_UNIT_MONO;
         ctx->channel_blocks[4]  = CH_UNIT_MONO;
+        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+BC+LFE");
+        if (ret < 0)
+            return ret;
         break;
     case 8:
         avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_7POINT1;
@@ -136,6 +150,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         ctx->channel_blocks[2]  = CH_UNIT_STEREO;
         ctx->channel_blocks[3]  = CH_UNIT_STEREO;
         ctx->channel_blocks[4]  = CH_UNIT_MONO;
+        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+SL+SR+LFE");
+        if (ret < 0)
+            return ret;
         break;
     default:
         av_log(avctx, AV_LOG_ERROR,
@@ -377,10 +394,12 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
         reconstruct_frame(ctx, &ctx->ch_units[ch_block],
                           channels_to_process, avctx);
 
-        for (i = 0; i < channels_to_process; i++)
-            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
+        for (i = 0; i < channels_to_process; i++) {
+            enum AVChannel ch = av_channel_layout_channel_from_index(&ctx->coded_ch_layout, out_ch_index + i);
+            int idx = av_channel_layout_index_from_channel(&frame->ch_layout, ch);
+            memcpy(samples_p[idx], ctx->outp_buf[i],
                    ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
-
+        }
         ch_block++;
         out_ch_index += channels_to_process;
     }
-- 
2.38.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/atrac3plus: reorder channels to match the output layout

[Andreas Rheinhardt]

James Almer:
> The order in which the channels are coded in the bitstream do not always follow
> the native, bitmask-based order of channels both signaled by the WAV container
> and forced by this same decoder. This is the case with layouts containing an
> LFE channel, as it's always coded last.
> 
> Fixes ticket #9964.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> 6.1 and 7.1 untested, but much like 5.1 they were wrong before this change.
> 
>  libavcodec/atrac3plusdec.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
> index ee71645a3c..8c3d20af76 100644
> --- a/libavcodec/atrac3plusdec.c
> +++ b/libavcodec/atrac3plusdec.c
> @@ -65,6 +65,7 @@ typedef struct ATRAC3PContext {
>  
>      int num_channel_blocks;     ///< number of channel blocks
>      uint8_t channel_blocks[5];  ///< channel configuration descriptor
> +    AVChannelLayout coded_ch_layout; ///< order of channels as coded in the bitstream
>  } ATRAC3PContext;
>  
>  static av_cold int atrac3p_decode_close(AVCodecContext *avctx)
> @@ -74,6 +75,8 @@ static av_cold int atrac3p_decode_close(AVCodecContext *avctx)
>      av_freep(&ctx->ch_units);
>      av_freep(&ctx->fdsp);
>  
> +    av_channel_layout_uninit(&ctx->coded_ch_layout);
> +
>      ff_mdct_end(&ctx->mdct_ctx);
>      ff_mdct_end(&ctx->ipqf_dct_ctx);
>  
> @@ -84,6 +87,7 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>                                        AVCodecContext *avctx)
>  {
>      int channels = avctx->ch_layout.nb_channels;
> +    int ret;
>      memset(ctx->channel_blocks, 0, sizeof(ctx->channel_blocks));
>  
>      av_channel_layout_uninit(&avctx->ch_layout);
> @@ -92,17 +96,20 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_MONO;
>          ctx->num_channel_blocks = 1;
>          ctx->channel_blocks[0]  = CH_UNIT_MONO;
> +        ctx->coded_ch_layout    = avctx->ch_layout;
>          break;
>      case 2:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_STEREO;
>          ctx->num_channel_blocks = 1;
>          ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> +        ctx->coded_ch_layout    = avctx->ch_layout;
>          break;
>      case 3:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_SURROUND;
>          ctx->num_channel_blocks = 2;
>          ctx->channel_blocks[0]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[1]  = CH_UNIT_MONO;
> +        ctx->coded_ch_layout    = avctx->ch_layout;
>          break;
>      case 4:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_4POINT0;
> @@ -110,6 +117,7 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>          ctx->channel_blocks[0]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[1]  = CH_UNIT_MONO;
>          ctx->channel_blocks[2]  = CH_UNIT_MONO;
> +        ctx->coded_ch_layout    = avctx->ch_layout;
>          break;
>      case 6:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_5POINT1_BACK;
> @@ -118,6 +126,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>          ctx->channel_blocks[1]  = CH_UNIT_MONO;
>          ctx->channel_blocks[2]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[3]  = CH_UNIT_MONO;
> +        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+LFE");
> +        if (ret < 0)
> +            return ret;
>          break;
>      case 7:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_6POINT1_BACK;
> @@ -127,6 +138,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>          ctx->channel_blocks[2]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[3]  = CH_UNIT_MONO;
>          ctx->channel_blocks[4]  = CH_UNIT_MONO;
> +        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+BC+LFE");
> +        if (ret < 0)
> +            return ret;
>          break;
>      case 8:
>          avctx->ch_layout        = (AVChannelLayout)AV_CHANNEL_LAYOUT_7POINT1;
> @@ -136,6 +150,9 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
>          ctx->channel_blocks[2]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[3]  = CH_UNIT_STEREO;
>          ctx->channel_blocks[4]  = CH_UNIT_MONO;
> +        ret = av_channel_layout_from_string(&ctx->coded_ch_layout, "FL+FR+FC+BL+BR+SL+SR+LFE");
> +        if (ret < 0)
> +            return ret;
>          break;
>      default:
>          av_log(avctx, AV_LOG_ERROR,
> @@ -377,10 +394,12 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>          reconstruct_frame(ctx, &ctx->ch_units[ch_block],
>                            channels_to_process, avctx);
>  
> -        for (i = 0; i < channels_to_process; i++)
> -            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
> +        for (i = 0; i < channels_to_process; i++) {
> +            enum AVChannel ch = av_channel_layout_channel_from_index(&ctx->coded_ch_layout, out_ch_index + i);
> +            int idx = av_channel_layout_index_from_channel(&frame->ch_layout, ch);
> +            memcpy(samples_p[idx], ctx->outp_buf[i],
>                     ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
> -
> +        }
>          ch_block++;
>          out_ch_index += channels_to_process;
>      }

Wouldn't it be simpler to just use a hardcoded array like { 0, 1, 2, 4,
5, 6, 3 } for 6.1 (and other arrays for the other layouts) instead of
using the channel layout API to do it (after all, the order of channels
in enum AVChannel won't change)? This would avoid the allocations
inherent in av_channel_layout_from_string() as well as the function
calls in atrac3p_decode_frame().

Apart from that, setting a channel layout via assignment is forbidden
(yes, i know it is safe here, because you are just copying an explicit
layout once more).

- Andreas

_______________________________________________
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".

[FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout

[James Almer]

The order in which the channels are coded in the bitstream do not always follow
the native, bitmask-based order of channels both signaled by the WAV container
and forced by this same decoder. This is the case with layouts containing an
LFE channel, as it's always coded last.

Fixes ticket #9964.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/atrac3plusdec.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
index ee71645a3c..9e12f21930 100644
--- a/libavcodec/atrac3plusdec.c
+++ b/libavcodec/atrac3plusdec.c
@@ -48,6 +48,17 @@
 #include "atrac.h"
 #include "atrac3plus.h"
 
+static uint8_t channel_map[8][8] = {
+    { 0, },
+    { 0, 1, },
+    { 0, 1, 2, },
+    { 0, 1, 2, 3, },
+    { 0, },
+    { 0, 1, 2, 4, 5, 3, },
+    { 0, 1, 2, 4, 5, 8, 3, },
+    { 0, 1, 2, 4, 5, 9, 10, 3, },
+};
+
 typedef struct ATRAC3PContext {
     GetBitContext gb;
     AVFloatDSPContext *fdsp;
@@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
                           channels_to_process, avctx);
 
         for (i = 0; i < channels_to_process; i++)
-            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
+            memcpy(samples_p[channel_map[frame->ch_layout.nb_channels - 1][out_ch_index + i]], ctx->outp_buf[i],
                    ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
 
         ch_block++;
-- 
2.38.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout

[Andreas Rheinhardt]

James Almer:
> The order in which the channels are coded in the bitstream do not always follow
> the native, bitmask-based order of channels both signaled by the WAV container
> and forced by this same decoder. This is the case with layouts containing an
> LFE channel, as it's always coded last.
> 
> Fixes ticket #9964.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/atrac3plusdec.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
> index ee71645a3c..9e12f21930 100644
> --- a/libavcodec/atrac3plusdec.c
> +++ b/libavcodec/atrac3plusdec.c
> @@ -48,6 +48,17 @@
>  #include "atrac.h"
>  #include "atrac3plus.h"
>  
> +static uint8_t channel_map[8][8] = {
> +    { 0, },
> +    { 0, 1, },
> +    { 0, 1, 2, },
> +    { 0, 1, 2, 3, },
> +    { 0, },
> +    { 0, 1, 2, 4, 5, 3, },
> +    { 0, 1, 2, 4, 5, 8, 3, },
> +    { 0, 1, 2, 4, 5, 9, 10, 3, },
> +};
> +
>  typedef struct ATRAC3PContext {
>      GetBitContext gb;
>      AVFloatDSPContext *fdsp;
> @@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>                            channels_to_process, avctx);
>  
>          for (i = 0; i < channels_to_process; i++)
> -            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
> +            memcpy(samples_p[channel_map[frame->ch_layout.nb_channels - 1][out_ch_index + i]], ctx->outp_buf[i],
>                     ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
>  
>          ch_block++;

Looking at the last two entries, it seems to me that you simply used the
numerical values of the AV_CHAN_* constants, i.e. as if you wanted to
write { AV_CHAN_FRONT_LEFT, AV_CHAN_FRONT_RIGHT, AV_CHAN_FRONT_CENTER,
AV_CHAN_BACK_LEFT, AV_CHAN_BACK_RIGHT, AV_CHAN_SIDE_LEFT,
AV_CHAN_SIDE_RIGHT, AV_CHAN_LOW_FREQUENCY } for the last entry. This is
wrong, as it conflates the enum AVChannel domain with the index domain;
it will segfault for 6.1 and 7.1, because there are no data planes with
indices 8, 9 and 10 in the output frame.

The array for 6.1 is { 0, 1, 2, 4, 5, 6, 3 }, for 7.1 it is { 0, 1, 2,
4, 5, 6, 7, 3, } (presuming that your first patch was right). The
derivation for 6.1 is as follows: The first channel in atrac is FL,
which is also the first channel in AV_CHANNEL_LAYOUT_6POINT1_BACK. So
the first entry is 0; the next channel is FR which is the second channel
in AV_CHANNEL_LAYOUT_6POINT1_BACK, so the second entry is 1. The next
atrac entry is FC, which is also the next entry in
AV_CHANNEL_LAYOUT_6POINT1_BACK, so the next entry is 2. The next atrac
entry is BL, which is not the next channel in
AV_CHANNEL_LAYOUT_6POINT1_BACK (which is lfe), but the one after that.
So the next entry is four. Similarly, the next entry is five. The atrac
entry after that is BC, which is the next (and last) entry of
AV_CHANNEL_LAYOUT_6POINT1_BACK and has index six. It doesn't matter for
this that there are several channels in enum AVChannel between BR and
BC, as these channels are not present in AV_CHANNEL_LAYOUT_6POINT1_BACK
(it would also not matter if there were any gaps in the values of the
AV_CHAN_* constants in between). The next (and last) atrac entry is LFE,
which is the fourth channel in AV_CHANNEL_LAYOUT_6POINT1_BACK and
therefore has index 3.

Is it really absolutely guaranteed that atrac only has one channel
layout per channel count? It seems to me that adding a const uint8_t
*channel_map to the context that gets set like ctx->channel_map = (const
uint8_t[]){ 0, 1, 2, 4, 5, 6, 3 } (for 6.1) would be simpler.

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout

[James Almer]

On 10/31/2022 7:13 PM, Andreas Rheinhardt wrote:
> James Almer:
>> The order in which the channels are coded in the bitstream do not always follow
>> the native, bitmask-based order of channels both signaled by the WAV container
>> and forced by this same decoder. This is the case with layouts containing an
>> LFE channel, as it's always coded last.
>>
>> Fixes ticket #9964.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/atrac3plusdec.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
>> index ee71645a3c..9e12f21930 100644
>> --- a/libavcodec/atrac3plusdec.c
>> +++ b/libavcodec/atrac3plusdec.c
>> @@ -48,6 +48,17 @@
>>   #include "atrac.h"
>>   #include "atrac3plus.h"
>>   
>> +static uint8_t channel_map[8][8] = {
>> +    { 0, },
>> +    { 0, 1, },
>> +    { 0, 1, 2, },
>> +    { 0, 1, 2, 3, },
>> +    { 0, },
>> +    { 0, 1, 2, 4, 5, 3, },
>> +    { 0, 1, 2, 4, 5, 8, 3, },
>> +    { 0, 1, 2, 4, 5, 9, 10, 3, },
>> +};
>> +
>>   typedef struct ATRAC3PContext {
>>       GetBitContext gb;
>>       AVFloatDSPContext *fdsp;
>> @@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>>                             channels_to_process, avctx);
>>   
>>           for (i = 0; i < channels_to_process; i++)
>> -            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
>> +            memcpy(samples_p[channel_map[frame->ch_layout.nb_channels - 1][out_ch_index + i]], ctx->outp_buf[i],
>>                      ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
>>   
>>           ch_block++;
> 
> Looking at the last two entries, it seems to me that you simply used the
> numerical values of the AV_CHAN_* constants, i.e. as if you wanted to
> write { AV_CHAN_FRONT_LEFT, AV_CHAN_FRONT_RIGHT, AV_CHAN_FRONT_CENTER,
> AV_CHAN_BACK_LEFT, AV_CHAN_BACK_RIGHT, AV_CHAN_SIDE_LEFT,
> AV_CHAN_SIDE_RIGHT, AV_CHAN_LOW_FREQUENCY } for the last entry. This is
> wrong, as it conflates the enum AVChannel domain with the index domain;
> it will segfault for 6.1 and 7.1, because there are no data planes with
> indices 8, 9 and 10 in the output frame.

Yeah, i mixed ids and indexes. Will fix.

> 
> The array for 6.1 is { 0, 1, 2, 4, 5, 6, 3 }, for 7.1 it is { 0, 1, 2,
> 4, 5, 6, 7, 3, } (presuming that your first patch was right). The
> derivation for 6.1 is as follows: The first channel in atrac is FL,
> which is also the first channel in AV_CHANNEL_LAYOUT_6POINT1_BACK. So
> the first entry is 0; the next channel is FR which is the second channel
> in AV_CHANNEL_LAYOUT_6POINT1_BACK, so the second entry is 1. The next
> atrac entry is FC, which is also the next entry in
> AV_CHANNEL_LAYOUT_6POINT1_BACK, so the next entry is 2. The next atrac
> entry is BL, which is not the next channel in
> AV_CHANNEL_LAYOUT_6POINT1_BACK (which is lfe), but the one after that.
> So the next entry is four. Similarly, the next entry is five. The atrac
> entry after that is BC, which is the next (and last) entry of
> AV_CHANNEL_LAYOUT_6POINT1_BACK and has index six. It doesn't matter for
> this that there are several channels in enum AVChannel between BR and
> BC, as these channels are not present in AV_CHANNEL_LAYOUT_6POINT1_BACK
> (it would also not matter if there were any gaps in the values of the
> AV_CHAN_* constants in between). The next (and last) atrac entry is LFE,
> which is the fourth channel in AV_CHANNEL_LAYOUT_6POINT1_BACK and
> therefore has index 3.
> 
> Is it really absolutely guaranteed that atrac only has one channel
> layout per channel count? It seems to me that adding a const uint8_t
> *channel_map to the context that gets set like ctx->channel_map = (const
> uint8_t[]){ 0, 1, 2, 4, 5, 6, 3 } (for 6.1) would be simpler.

I don't know if it's guaranteed, but that's what this decoder assumes 
directly during init with user set values (so nothing read from the 
bitstream for it), so I'll not assume the opposite.

> 
> - Andreas
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout

[James Almer]

On 10/31/2022 7:13 PM, Andreas Rheinhardt wrote:
> James Almer:
>> The order in which the channels are coded in the bitstream do not always follow
>> the native, bitmask-based order of channels both signaled by the WAV container
>> and forced by this same decoder. This is the case with layouts containing an
>> LFE channel, as it's always coded last.
>>
>> Fixes ticket #9964.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/atrac3plusdec.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
>> index ee71645a3c..9e12f21930 100644
>> --- a/libavcodec/atrac3plusdec.c
>> +++ b/libavcodec/atrac3plusdec.c
>> @@ -48,6 +48,17 @@
>>   #include "atrac.h"
>>   #include "atrac3plus.h"
>>   
>> +static uint8_t channel_map[8][8] = {
>> +    { 0, },
>> +    { 0, 1, },
>> +    { 0, 1, 2, },
>> +    { 0, 1, 2, 3, },
>> +    { 0, },
>> +    { 0, 1, 2, 4, 5, 3, },
>> +    { 0, 1, 2, 4, 5, 8, 3, },
>> +    { 0, 1, 2, 4, 5, 9, 10, 3, },
>> +};
>> +
>>   typedef struct ATRAC3PContext {
>>       GetBitContext gb;
>>       AVFloatDSPContext *fdsp;
>> @@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>>                             channels_to_process, avctx);
>>   
>>           for (i = 0; i < channels_to_process; i++)
>> -            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
>> +            memcpy(samples_p[channel_map[frame->ch_layout.nb_channels - 1][out_ch_index + i]], ctx->outp_buf[i],
>>                      ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
>>   
>>           ch_block++;
> 
> Looking at the last two entries, it seems to me that you simply used the
> numerical values of the AV_CHAN_* constants, i.e. as if you wanted to
> write { AV_CHAN_FRONT_LEFT, AV_CHAN_FRONT_RIGHT, AV_CHAN_FRONT_CENTER,
> AV_CHAN_BACK_LEFT, AV_CHAN_BACK_RIGHT, AV_CHAN_SIDE_LEFT,
> AV_CHAN_SIDE_RIGHT, AV_CHAN_LOW_FREQUENCY } for the last entry. This is
> wrong, as it conflates the enum AVChannel domain with the index domain;
> it will segfault for 6.1 and 7.1, because there are no data planes with
> indices 8, 9 and 10 in the output frame.
> 
> The array for 6.1 is { 0, 1, 2, 4, 5, 6, 3 }, for 7.1 it is { 0, 1, 2,
> 4, 5, 6, 7, 3, } (presuming that your first patch was right). The
> derivation for 6.1 is as follows: The first channel in atrac is FL,
> which is also the first channel in AV_CHANNEL_LAYOUT_6POINT1_BACK. So
> the first entry is 0; the next channel is FR which is the second channel
> in AV_CHANNEL_LAYOUT_6POINT1_BACK, so the second entry is 1. The next
> atrac entry is FC, which is also the next entry in
> AV_CHANNEL_LAYOUT_6POINT1_BACK, so the next entry is 2. The next atrac
> entry is BL, which is not the next channel in
> AV_CHANNEL_LAYOUT_6POINT1_BACK (which is lfe), but the one after that.
> So the next entry is four. Similarly, the next entry is five. The atrac
> entry after that is BC, which is the next (and last) entry of
> AV_CHANNEL_LAYOUT_6POINT1_BACK and has index six. It doesn't matter for
> this that there are several channels in enum AVChannel between BR and
> BC, as these channels are not present in AV_CHANNEL_LAYOUT_6POINT1_BACK
> (it would also not matter if there were any gaps in the values of the
> AV_CHAN_* constants in between). The next (and last) atrac entry is LFE,
> which is the fourth channel in AV_CHANNEL_LAYOUT_6POINT1_BACK and
> therefore has index 3.
> 
> Is it really absolutely guaranteed that atrac only has one channel
> layout per channel count? It seems to me that adding a const uint8_t
> *channel_map to the context that gets set like ctx->channel_map = (const
> uint8_t[]){ 0, 1, 2, 4, 5, 6, 3 } (for 6.1) would be simpler.

Unless i'm missing something, this would be local to set_channel_params().

> 
> - Andreas
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/atrac3plus: reorder channels to match the output layout

[Andreas Rheinhardt]

James Almer:
> On 10/31/2022 7:13 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> The order in which the channels are coded in the bitstream do not
>>> always follow
>>> the native, bitmask-based order of channels both signaled by the WAV
>>> container
>>> and forced by this same decoder. This is the case with layouts
>>> containing an
>>> LFE channel, as it's always coded last.
>>>
>>> Fixes ticket #9964.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>   libavcodec/atrac3plusdec.c | 13 ++++++++++++-
>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
>>> index ee71645a3c..9e12f21930 100644
>>> --- a/libavcodec/atrac3plusdec.c
>>> +++ b/libavcodec/atrac3plusdec.c
>>> @@ -48,6 +48,17 @@
>>>   #include "atrac.h"
>>>   #include "atrac3plus.h"
>>>   +static uint8_t channel_map[8][8] = {
>>> +    { 0, },
>>> +    { 0, 1, },
>>> +    { 0, 1, 2, },
>>> +    { 0, 1, 2, 3, },
>>> +    { 0, },
>>> +    { 0, 1, 2, 4, 5, 3, },
>>> +    { 0, 1, 2, 4, 5, 8, 3, },
>>> +    { 0, 1, 2, 4, 5, 9, 10, 3, },
>>> +};
>>> +
>>>   typedef struct ATRAC3PContext {
>>>       GetBitContext gb;
>>>       AVFloatDSPContext *fdsp;
>>> @@ -378,7 +389,7 @@ static int atrac3p_decode_frame(AVCodecContext
>>> *avctx, AVFrame *frame,
>>>                             channels_to_process, avctx);
>>>             for (i = 0; i < channels_to_process; i++)
>>> -            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
>>> +           
>>> memcpy(samples_p[channel_map[frame->ch_layout.nb_channels -
>>> 1][out_ch_index + i]], ctx->outp_buf[i],
>>>                      ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
>>>             ch_block++;
>>
>> Looking at the last two entries, it seems to me that you simply used the
>> numerical values of the AV_CHAN_* constants, i.e. as if you wanted to
>> write { AV_CHAN_FRONT_LEFT, AV_CHAN_FRONT_RIGHT, AV_CHAN_FRONT_CENTER,
>> AV_CHAN_BACK_LEFT, AV_CHAN_BACK_RIGHT, AV_CHAN_SIDE_LEFT,
>> AV_CHAN_SIDE_RIGHT, AV_CHAN_LOW_FREQUENCY } for the last entry. This is
>> wrong, as it conflates the enum AVChannel domain with the index domain;
>> it will segfault for 6.1 and 7.1, because there are no data planes with
>> indices 8, 9 and 10 in the output frame.
>>
>> The array for 6.1 is { 0, 1, 2, 4, 5, 6, 3 }, for 7.1 it is { 0, 1, 2,
>> 4, 5, 6, 7, 3, } (presuming that your first patch was right). The
>> derivation for 6.1 is as follows: The first channel in atrac is FL,
>> which is also the first channel in AV_CHANNEL_LAYOUT_6POINT1_BACK. So
>> the first entry is 0; the next channel is FR which is the second channel
>> in AV_CHANNEL_LAYOUT_6POINT1_BACK, so the second entry is 1. The next
>> atrac entry is FC, which is also the next entry in
>> AV_CHANNEL_LAYOUT_6POINT1_BACK, so the next entry is 2. The next atrac
>> entry is BL, which is not the next channel in
>> AV_CHANNEL_LAYOUT_6POINT1_BACK (which is lfe), but the one after that.
>> So the next entry is four. Similarly, the next entry is five. The atrac
>> entry after that is BC, which is the next (and last) entry of
>> AV_CHANNEL_LAYOUT_6POINT1_BACK and has index six. It doesn't matter for
>> this that there are several channels in enum AVChannel between BR and
>> BC, as these channels are not present in AV_CHANNEL_LAYOUT_6POINT1_BACK
>> (it would also not matter if there were any gaps in the values of the
>> AV_CHAN_* constants in between). The next (and last) atrac entry is LFE,
>> which is the fourth channel in AV_CHANNEL_LAYOUT_6POINT1_BACK and
>> therefore has index 3.
>>
>> Is it really absolutely guaranteed that atrac only has one channel
>> layout per channel count? It seems to me that adding a const uint8_t
>> *channel_map to the context that gets set like ctx->channel_map = (const
>> uint8_t[]){ 0, 1, 2, 4, 5, 6, 3 } (for 6.1) would be simpler.
> 
> Unless i'm missing something, this would be local to set_channel_params().
> 

Correct.

- Andreas

_______________________________________________
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".

[FFmpeg-devel] [PATCH v3] avcodec/atrac3plus: reorder channels to match the output layout

[James Almer]

The order in which the channels are coded in the bitstream do not always follow
the native, bitmask-based order of channels both signaled by the WAV container
and forced by this same decoder. This is the case with layouts containing an
LFE channel, as it's always coded last.

Fixes ticket #9964.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/atrac3plusdec.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
index ee71645a3c..2bc023cb23 100644
--- a/libavcodec/atrac3plusdec.c
+++ b/libavcodec/atrac3plusdec.c
@@ -48,6 +48,17 @@
 #include "atrac.h"
 #include "atrac3plus.h"
 
+static const uint8_t channel_map[8][8] = {
+    { 0, },
+    { 0, 1, },
+    { 0, 1, 2, },
+    { 0, 1, 2, 3, },
+    { 0, },
+    { 0, 1, 2, 4, 5, 3, },
+    { 0, 1, 2, 4, 5, 6, 3, },
+    { 0, 1, 2, 4, 5, 6, 7, 3, },
+};
+
 typedef struct ATRAC3PContext {
     GetBitContext gb;
     AVFloatDSPContext *fdsp;
@@ -65,6 +76,7 @@ typedef struct ATRAC3PContext {
 
     int num_channel_blocks;     ///< number of channel blocks
     uint8_t channel_blocks[5];  ///< channel configuration descriptor
+    const uint8_t *channel_map; ///< channel layout map
 } ATRAC3PContext;
 
 static av_cold int atrac3p_decode_close(AVCodecContext *avctx)
@@ -143,6 +155,8 @@ static av_cold int set_channel_params(ATRAC3PContext *ctx,
         return AVERROR_INVALIDDATA;
     }
 
+    ctx->channel_map = channel_map[channels - 1];
+
     return 0;
 }
 
@@ -378,7 +392,7 @@ static int atrac3p_decode_frame(AVCodecContext *avctx, AVFrame *frame,
                           channels_to_process, avctx);
 
         for (i = 0; i < channels_to_process; i++)
-            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
+            memcpy(samples_p[ctx->channel_map[out_ch_index + i]], ctx->outp_buf[i],
                    ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
 
         ch_block++;
-- 
2.38.1

_______________________________________________
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".