diff mbox series

[FFmpeg-devel] avcodec/aacdec: don't force HE-AACv2 profile if no PS info is present

Message ID 20220713175948.1955-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/aacdec: don't force HE-AACv2 profile if no PS info is present | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

James Almer July 13, 2022, 5:59 p.m. UTC
Should fix ticket #3361

Signed-off-by: James Almer <jamrial@gmail.com>
---
This also needs an update to some fate ref samples i'll upload before pushing
(fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now decoded
properly as he_aac mono, so the .s16 files need to be replaced).

 libavcodec/aacdec_template.c                      | 15 ++++++---------
 libavcodec/aacsbr_template.c                      |  1 +
 tests/fate/gapless.mak                            | 14 +++++++-------
 .../audiomatch-afconvert-16000-stereo-he2-adts    |  2 +-
 4 files changed, 15 insertions(+), 17 deletions(-)

Comments

Andreas Rheinhardt July 14, 2022, 12:10 p.m. UTC | #1
James Almer:
> Should fix ticket #3361
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This also needs an update to some fate ref samples i'll upload before pushing
> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now decoded
> properly as he_aac mono, so the .s16 files need to be replaced).
> 

We have both a fixed-point AAC as well as a floating point AAC decoder.
Is there actually a test that tests that the output they produce is
reasonably close? If not, could we make the test so that the same file
is decoded once with the fixed-point and once with the floating-point
decoder and then compared?

>  libavcodec/aacdec_template.c                      | 15 ++++++---------
>  libavcodec/aacsbr_template.c                      |  1 +
>  tests/fate/gapless.mak                            | 14 +++++++-------
>  .../audiomatch-afconvert-16000-stereo-he2-adts    |  2 +-
>  4 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index 10fba3d3b2..15c20c07d6 100644
> --- a/libavcodec/aacdec_template.c
> +++ b/libavcodec/aacdec_template.c
> @@ -967,8 +967,7 @@ static int decode_ga_specific_config(AACContext *ac, AVCodecContext *avctx,
>  
>      if (count_channels(layout_map, tags) > 1) {
>          m4ac->ps = 0;
> -    } else if (m4ac->sbr == 1 && m4ac->ps == -1)
> -        m4ac->ps = 1;
> +    }
>  
>      if (ac && (ret = output_configure(ac, layout_map, tags, OC_GLOBAL_HDR, 0)))
>          return ret;
> @@ -2572,18 +2571,16 @@ static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
>              av_log(ac->avctx, AV_LOG_ERROR, "Implicit SBR was found with a first occurrence after the first frame.\n");
>              skip_bits_long(gb, 8 * cnt - 4);
>              return res;
> -        } else if (ac->oc[1].m4ac.ps == -1 && ac->oc[1].status < OC_LOCKED &&
> -                   ac->avctx->ch_layout.nb_channels == 1) {
> -            ac->oc[1].m4ac.sbr = 1;
> -            ac->oc[1].m4ac.ps = 1;
> -            ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
> -            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
> -                             ac->oc[1].status, 1);
>          } else {
>              ac->oc[1].m4ac.sbr = 1;
>              ac->avctx->profile = FF_PROFILE_AAC_HE;
>          }
>          res = AAC_RENAME(ff_decode_sbr_extension)(ac, &che->sbr, gb, crc_flag, cnt, elem_type);
> +        if (ac->oc[1].m4ac.ps == 1 && ac->oc[1].status < OC_LOCKED &&
> +            ac->avctx->ch_layout.nb_channels == 1) {
> +            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
> +                             ac->oc[1].status, 1);
> +        }
>          break;
>      case EXT_DYNAMIC_RANGE:
>          res = decode_dynamic_range(&ac->che_drc, gb);
> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
> index b72c94b76d..f9925b40e5 100644
> --- a/libavcodec/aacsbr_template.c
> +++ b/libavcodec/aacsbr_template.c
> @@ -954,6 +954,7 @@ static void read_sbr_extension(AACContext *ac, SpectralBandReplication *sbr,
>              *num_bits_left = 0;
>          } else {
>              *num_bits_left -= ff_ps_read_data(ac->avctx, gb, &sbr->ps.common, *num_bits_left);
> +            ac->oc[1].m4ac.ps = 1;
>              ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
>          }
>          break;
> diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak
> index 68a396e187..7dd8ceb142 100644
> --- a/tests/fate/gapless.mak
> +++ b/tests/fate/gapless.mak
> @@ -47,27 +47,27 @@ fate-audiomatch-square-aac: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/squar
>  
>  fate-audiomatch-afconvert-16000-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav
>  fate-audiomatch-afconvert-16000-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav
> -fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
> -fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
> +fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
> +fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
>  fate-audiomatch-afconvert-16000-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>  fate-audiomatch-afconvert-16000-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>  fate-audiomatch-afconvert-16000-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>  fate-audiomatch-afconvert-16000-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
> -fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
> +fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ac 2 -ar 16000"
>  fate-audiomatch-afconvert-16000-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>  fate-audiomatch-afconvert-44100-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
>  fate-audiomatch-afconvert-44100-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
> -fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
> -fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
> +fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
> +fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>  fate-audiomatch-afconvert-44100-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  fate-audiomatch-afconvert-44100-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  fate-audiomatch-afconvert-44100-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  fate-audiomatch-afconvert-44100-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
> -fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav
> +fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav "-ac 2"
>  fate-audiomatch-afconvert-44100-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  
>  fate-audiomatch-dolby-44100-mono-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
> -fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
> +fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>  fate-audiomatch-dolby-44100-stereo-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  fate-audiomatch-dolby-44100-stereo-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>  fate-audiomatch-dolby-44100-stereo-he2-mp4: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he2.mp4  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
> diff --git a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
> index 32b2627946..527c9acdba 100644
> --- a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
> +++ b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
> @@ -1 +1 @@
> -presig: 5186 postsig:446 c: 0.9839 lenerr:5632
> +presig: 5154 postsig:446 c: 0.9839 lenerr:5600
James Almer July 16, 2022, 12:53 p.m. UTC | #2
On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Should fix ticket #3361
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This also needs an update to some fate ref samples i'll upload before pushing
>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now decoded
>> properly as he_aac mono, so the .s16 files need to be replaced).
>>
> 
> We have both a fixed-point AAC as well as a floating point AAC decoder.
> Is there actually a test that tests that the output they produce is
> reasonably close? If not, could we make the test so that the same file
> is decoded once with the fixed-point and once with the floating-point
> decoder and then compared?

That wouldn't help much, i think. Almost all changes to *_template.c 
files are going to affect both decoders, so a breakage would not be 
detected if you compare their output with each other as they would both 
exhibit it.

> 
>>   libavcodec/aacdec_template.c                      | 15 ++++++---------
>>   libavcodec/aacsbr_template.c                      |  1 +
>>   tests/fate/gapless.mak                            | 14 +++++++-------
>>   .../audiomatch-afconvert-16000-stereo-he2-adts    |  2 +-
>>   4 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>> index 10fba3d3b2..15c20c07d6 100644
>> --- a/libavcodec/aacdec_template.c
>> +++ b/libavcodec/aacdec_template.c
>> @@ -967,8 +967,7 @@ static int decode_ga_specific_config(AACContext *ac, AVCodecContext *avctx,
>>   
>>       if (count_channels(layout_map, tags) > 1) {
>>           m4ac->ps = 0;
>> -    } else if (m4ac->sbr == 1 && m4ac->ps == -1)
>> -        m4ac->ps = 1;
>> +    }
>>   
>>       if (ac && (ret = output_configure(ac, layout_map, tags, OC_GLOBAL_HDR, 0)))
>>           return ret;
>> @@ -2572,18 +2571,16 @@ static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
>>               av_log(ac->avctx, AV_LOG_ERROR, "Implicit SBR was found with a first occurrence after the first frame.\n");
>>               skip_bits_long(gb, 8 * cnt - 4);
>>               return res;
>> -        } else if (ac->oc[1].m4ac.ps == -1 && ac->oc[1].status < OC_LOCKED &&
>> -                   ac->avctx->ch_layout.nb_channels == 1) {
>> -            ac->oc[1].m4ac.sbr = 1;
>> -            ac->oc[1].m4ac.ps = 1;
>> -            ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
>> -            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
>> -                             ac->oc[1].status, 1);
>>           } else {
>>               ac->oc[1].m4ac.sbr = 1;
>>               ac->avctx->profile = FF_PROFILE_AAC_HE;
>>           }
>>           res = AAC_RENAME(ff_decode_sbr_extension)(ac, &che->sbr, gb, crc_flag, cnt, elem_type);
>> +        if (ac->oc[1].m4ac.ps == 1 && ac->oc[1].status < OC_LOCKED &&
>> +            ac->avctx->ch_layout.nb_channels == 1) {
>> +            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
>> +                             ac->oc[1].status, 1);
>> +        }
>>           break;
>>       case EXT_DYNAMIC_RANGE:
>>           res = decode_dynamic_range(&ac->che_drc, gb);
>> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
>> index b72c94b76d..f9925b40e5 100644
>> --- a/libavcodec/aacsbr_template.c
>> +++ b/libavcodec/aacsbr_template.c
>> @@ -954,6 +954,7 @@ static void read_sbr_extension(AACContext *ac, SpectralBandReplication *sbr,
>>               *num_bits_left = 0;
>>           } else {
>>               *num_bits_left -= ff_ps_read_data(ac->avctx, gb, &sbr->ps.common, *num_bits_left);
>> +            ac->oc[1].m4ac.ps = 1;
>>               ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
>>           }
>>           break;
>> diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak
>> index 68a396e187..7dd8ceb142 100644
>> --- a/tests/fate/gapless.mak
>> +++ b/tests/fate/gapless.mak
>> @@ -47,27 +47,27 @@ fate-audiomatch-square-aac: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/squar
>>   
>>   fate-audiomatch-afconvert-16000-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav
>>   fate-audiomatch-afconvert-16000-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav
>> -fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
>> -fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
>> +fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
>> +fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
>>   fate-audiomatch-afconvert-16000-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>   fate-audiomatch-afconvert-16000-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>   fate-audiomatch-afconvert-16000-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>>   fate-audiomatch-afconvert-16000-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>> -fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>> +fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ac 2 -ar 16000"
>>   fate-audiomatch-afconvert-16000-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
>>   fate-audiomatch-afconvert-44100-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>   fate-audiomatch-afconvert-44100-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>> -fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>> -fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>> +fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
>> +fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>   fate-audiomatch-afconvert-44100-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   fate-audiomatch-afconvert-44100-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   fate-audiomatch-afconvert-44100-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   fate-audiomatch-afconvert-44100-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>> -fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>> +fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav "-ac 2"
>>   fate-audiomatch-afconvert-44100-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   
>>   fate-audiomatch-dolby-44100-mono-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>> -fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>> +fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>   fate-audiomatch-dolby-44100-stereo-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   fate-audiomatch-dolby-44100-stereo-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>   fate-audiomatch-dolby-44100-stereo-he2-mp4: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he2.mp4  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>> diff --git a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>> index 32b2627946..527c9acdba 100644
>> --- a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>> +++ b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>> @@ -1 +1 @@
>> -presig: 5186 postsig:446 c: 0.9839 lenerr:5632
>> +presig: 5154 postsig:446 c: 0.9839 lenerr:5600
> 
> _______________________________________________
> 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".
Andreas Rheinhardt July 18, 2022, 1:57 p.m. UTC | #3
James Almer:
> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Should fix ticket #3361
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> This also needs an update to some fate ref samples i'll upload before
>>> pushing
>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>> decoded
>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>
>>
>> We have both a fixed-point AAC as well as a floating point AAC decoder.
>> Is there actually a test that tests that the output they produce is
>> reasonably close? If not, could we make the test so that the same file
>> is decoded once with the fixed-point and once with the floating-point
>> decoder and then compared?
> 
> That wouldn't help much, i think. Almost all changes to *_template.c
> files are going to affect both decoders, so a breakage would not be
> detected if you compare their output with each other as they would both
> exhibit it.
> 

I actually thought that the aac_fixed tests used checksums instead of
ref files; then changes and breakages would be visible by changes to
these files. Apparently I was wrong about that and the ref files are
used for both aac and aac_fixed. But a test like the one outlined above
would nevertheless obviate the need for a new ref file.

>>
>>>   libavcodec/aacdec_template.c                      | 15 ++++++---------
>>>   libavcodec/aacsbr_template.c                      |  1 +
>>>   tests/fate/gapless.mak                            | 14 +++++++-------
>>>   .../audiomatch-afconvert-16000-stereo-he2-adts    |  2 +-
>>>   4 files changed, 15 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
>>> index 10fba3d3b2..15c20c07d6 100644
>>> --- a/libavcodec/aacdec_template.c
>>> +++ b/libavcodec/aacdec_template.c
>>> @@ -967,8 +967,7 @@ static int decode_ga_specific_config(AACContext
>>> *ac, AVCodecContext *avctx,
>>>         if (count_channels(layout_map, tags) > 1) {
>>>           m4ac->ps = 0;
>>> -    } else if (m4ac->sbr == 1 && m4ac->ps == -1)
>>> -        m4ac->ps = 1;
>>> +    }
>>>         if (ac && (ret = output_configure(ac, layout_map, tags,
>>> OC_GLOBAL_HDR, 0)))
>>>           return ret;
>>> @@ -2572,18 +2571,16 @@ static int
>>> decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
>>>               av_log(ac->avctx, AV_LOG_ERROR, "Implicit SBR was found
>>> with a first occurrence after the first frame.\n");
>>>               skip_bits_long(gb, 8 * cnt - 4);
>>>               return res;
>>> -        } else if (ac->oc[1].m4ac.ps == -1 && ac->oc[1].status <
>>> OC_LOCKED &&
>>> -                   ac->avctx->ch_layout.nb_channels == 1) {
>>> -            ac->oc[1].m4ac.sbr = 1;
>>> -            ac->oc[1].m4ac.ps = 1;
>>> -            ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
>>> -            output_configure(ac, ac->oc[1].layout_map,
>>> ac->oc[1].layout_map_tags,
>>> -                             ac->oc[1].status, 1);
>>>           } else {
>>>               ac->oc[1].m4ac.sbr = 1;
>>>               ac->avctx->profile = FF_PROFILE_AAC_HE;
>>>           }
>>>           res = AAC_RENAME(ff_decode_sbr_extension)(ac, &che->sbr,
>>> gb, crc_flag, cnt, elem_type);
>>> +        if (ac->oc[1].m4ac.ps == 1 && ac->oc[1].status < OC_LOCKED &&
>>> +            ac->avctx->ch_layout.nb_channels == 1) {
>>> +            output_configure(ac, ac->oc[1].layout_map,
>>> ac->oc[1].layout_map_tags,
>>> +                             ac->oc[1].status, 1);
>>> +        }
>>>           break;
>>>       case EXT_DYNAMIC_RANGE:
>>>           res = decode_dynamic_range(&ac->che_drc, gb);
>>> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
>>> index b72c94b76d..f9925b40e5 100644
>>> --- a/libavcodec/aacsbr_template.c
>>> +++ b/libavcodec/aacsbr_template.c
>>> @@ -954,6 +954,7 @@ static void read_sbr_extension(AACContext *ac,
>>> SpectralBandReplication *sbr,
>>>               *num_bits_left = 0;
>>>           } else {
>>>               *num_bits_left -= ff_ps_read_data(ac->avctx, gb,
>>> &sbr->ps.common, *num_bits_left);
>>> +            ac->oc[1].m4ac.ps = 1;
>>>               ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
>>>           }
>>>           break;
>>> diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak
>>> index 68a396e187..7dd8ceb142 100644
>>> --- a/tests/fate/gapless.mak
>>> +++ b/tests/fate/gapless.mak
>>> @@ -47,27 +47,27 @@ fate-audiomatch-square-aac: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/squar
>>>     fate-audiomatch-afconvert-16000-mono-lc-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.adts 
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav
>>>   fate-audiomatch-afconvert-16000-mono-lc-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.m4a  
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav
>>> -fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts 
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
>>> -fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a  
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
>>> +fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts 
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
>>> +fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a  
>>> $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
>>>   fate-audiomatch-afconvert-16000-stereo-lc-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>>
>>>   fate-audiomatch-afconvert-16000-stereo-lc-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>>
>>>   fate-audiomatch-afconvert-16000-stereo-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>> "-ar 16000"
>>>   fate-audiomatch-afconvert-16000-stereo-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>> "-ar 16000"
>>> -fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>> "-ar 16000"
>>> +fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>> "-ac 2 -ar 16000"
>>>   fate-audiomatch-afconvert-16000-stereo-he2-m4a: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
>>> "-ar 16000"
>>>   fate-audiomatch-afconvert-44100-mono-lc-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.adts 
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>>   fate-audiomatch-afconvert-44100-mono-lc-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.m4a  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>> -fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts 
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>>> -fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>>> +fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts 
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>> +fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>>   fate-audiomatch-afconvert-44100-stereo-lc-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>>   fate-audiomatch-afconvert-44100-stereo-lc-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>>   fate-audiomatch-afconvert-44100-stereo-he-adts: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>>   fate-audiomatch-afconvert-44100-stereo-he-m4a:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>> -fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>> +fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>> "-ac 2"
>>>   fate-audiomatch-afconvert-44100-stereo-he2-m4a: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>
>>>     fate-audiomatch-dolby-44100-mono-lc-mp4:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_lc.mp4  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>> -fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
>>> +fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4  
>>> $(SAMPLES)/audiomatch/tones_44100_mono.wav
>>>   fate-audiomatch-dolby-44100-stereo-lc-mp4:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_lc.mp4  
>>> $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>   fate-audiomatch-dolby-44100-stereo-he-mp4:  CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he.mp4  
>>> $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>>   fate-audiomatch-dolby-44100-stereo-he2-mp4: CMD = audio_match
>>> $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he2.mp4 
>>> $(SAMPLES)/audiomatch/tones_44100_stereo.wav
>>> diff --git
>>> a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>>> b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>>> index 32b2627946..527c9acdba 100644
>>> --- a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>>> +++ b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
>>> @@ -1 +1 @@
>>> -presig: 5186 postsig:446 c: 0.9839 lenerr:5632
>>> +presig: 5154 postsig:446 c: 0.9839 lenerr:5600
>>
James Almer July 22, 2022, 12:46 p.m. UTC | #4
On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> Should fix ticket #3361
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> This also needs an update to some fate ref samples i'll upload before
>>>> pushing
>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>>> decoded
>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>
>>>
>>> We have both a fixed-point AAC as well as a floating point AAC decoder.
>>> Is there actually a test that tests that the output they produce is
>>> reasonably close? If not, could we make the test so that the same file
>>> is decoded once with the fixed-point and once with the floating-point
>>> decoder and then compared?
>>
>> That wouldn't help much, i think. Almost all changes to *_template.c
>> files are going to affect both decoders, so a breakage would not be
>> detected if you compare their output with each other as they would both
>> exhibit it.
>>
> 
> I actually thought that the aac_fixed tests used checksums instead of
> ref files; then changes and breakages would be visible by changes to
> these files. Apparently I was wrong about that and the ref files are
> used for both aac and aac_fixed. But a test like the one outlined above
> would nevertheless obviate the need for a new ref file.

Judging by 
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117 
it seems at least for these samples the fixed decoder does not generate 
a decoded stream comparable to the float one, so I'll just upload a new 
raw pcm file.
Andreas Rheinhardt July 22, 2022, 2:23 p.m. UTC | #5
James Almer:
> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> Should fix ticket #3361
>>>>>
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>> This also needs an update to some fate ref samples i'll upload before
>>>>> pushing
>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>>>> decoded
>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>
>>>>
>>>> We have both a fixed-point AAC as well as a floating point AAC decoder.
>>>> Is there actually a test that tests that the output they produce is
>>>> reasonably close? If not, could we make the test so that the same file
>>>> is decoded once with the fixed-point and once with the floating-point
>>>> decoder and then compared?
>>>
>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>> files are going to affect both decoders, so a breakage would not be
>>> detected if you compare their output with each other as they would both
>>> exhibit it.
>>>
>>
>> I actually thought that the aac_fixed tests used checksums instead of
>> ref files; then changes and breakages would be visible by changes to
>> these files. Apparently I was wrong about that and the ref files are
>> used for both aac and aac_fixed. But a test like the one outlined above
>> would nevertheless obviate the need for a new ref file.
> 
> Judging by
> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
> it seems at least for these samples the fixed decoder does not generate
> a decoded stream comparable to the float one, so I'll just upload a new
> raw pcm file.

When I decode both of these streams with git master, the left channel is
pretty much identical, yet the right channel of the fixed-point decoder
is silent and the right channel of the floating point decoder is not.
With this patch applied, the result are two mono streams that are pretty
much identical: The test sample created by the floating-point decoder
works with the fixed-point decoder test (if one uncomments and modifies
the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
upload new samples.

- Andreas

PS: libfdk-aac produces a file that looks pretty much like the floating
point decoder from git master. Are you sure your patch is correct?
James Almer July 22, 2022, 2:51 p.m. UTC | #6
On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> Should fix ticket #3361
>>>>>>
>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>> ---
>>>>>> This also needs an update to some fate ref samples i'll upload before
>>>>>> pushing
>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>>>>> decoded
>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>
>>>>>
>>>>> We have both a fixed-point AAC as well as a floating point AAC decoder.
>>>>> Is there actually a test that tests that the output they produce is
>>>>> reasonably close? If not, could we make the test so that the same file
>>>>> is decoded once with the fixed-point and once with the floating-point
>>>>> decoder and then compared?
>>>>
>>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>>> files are going to affect both decoders, so a breakage would not be
>>>> detected if you compare their output with each other as they would both
>>>> exhibit it.
>>>>
>>>
>>> I actually thought that the aac_fixed tests used checksums instead of
>>> ref files; then changes and breakages would be visible by changes to
>>> these files. Apparently I was wrong about that and the ref files are
>>> used for both aac and aac_fixed. But a test like the one outlined above
>>> would nevertheless obviate the need for a new ref file.
>>
>> Judging by
>> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
>> it seems at least for these samples the fixed decoder does not generate
>> a decoded stream comparable to the float one, so I'll just upload a new
>> raw pcm file.
> 
> When I decode both of these streams with git master, the left channel is
> pretty much identical, yet the right channel of the fixed-point decoder
> is silent and the right channel of the floating point decoder is not.
> With this patch applied, the result are two mono streams that are pretty
> much identical: The test sample created by the floating-point decoder
> works with the fixed-point decoder test (if one uncomments and modifies
> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
> upload new samples.

Ok, can you suggest how to add a test that decodes with the fixed point 
decoder then compares that with the output of the float decoder? Is 
there a helper in fate.sh already for this?

> 
> - Andreas
> 
> PS: libfdk-aac produces a file that looks pretty much like the floating
> point decoder from git master. Are you sure your patch is correct?

Yes, they duplicate the single channel in the stream and output it as 
stereo, something that should be done by a filter if that's what the 
user wants. Decoding a mono sample should generate a mono stream.
Andreas Rheinhardt July 22, 2022, 2:56 p.m. UTC | #7
James Almer:
> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> Should fix ticket #3361
>>>>>>>
>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>> ---
>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>> before
>>>>>>> pushing
>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>>>>>> decoded
>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>>
>>>>>>
>>>>>> We have both a fixed-point AAC as well as a floating point AAC
>>>>>> decoder.
>>>>>> Is there actually a test that tests that the output they produce is
>>>>>> reasonably close? If not, could we make the test so that the same
>>>>>> file
>>>>>> is decoded once with the fixed-point and once with the floating-point
>>>>>> decoder and then compared?
>>>>>
>>>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>>>> files are going to affect both decoders, so a breakage would not be
>>>>> detected if you compare their output with each other as they would
>>>>> both
>>>>> exhibit it.
>>>>>
>>>>
>>>> I actually thought that the aac_fixed tests used checksums instead of
>>>> ref files; then changes and breakages would be visible by changes to
>>>> these files. Apparently I was wrong about that and the ref files are
>>>> used for both aac and aac_fixed. But a test like the one outlined above
>>>> would nevertheless obviate the need for a new ref file.
>>>
>>> Judging by
>>> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
>>>
>>> it seems at least for these samples the fixed decoder does not generate
>>> a decoded stream comparable to the float one, so I'll just upload a new
>>> raw pcm file.
>>
>> When I decode both of these streams with git master, the left channel is
>> pretty much identical, yet the right channel of the fixed-point decoder
>> is silent and the right channel of the floating point decoder is not.
>> With this patch applied, the result are two mono streams that are pretty
>> much identical: The test sample created by the floating-point decoder
>> works with the fixed-point decoder test (if one uncomments and modifies
>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
>> upload new samples.
> 
> Ok, can you suggest how to add a test that decodes with the fixed point
> decoder then compares that with the output of the float decoder? Is
> there a helper in fate.sh already for this?
> 

There is currently no helper in fate-run.sh for this.

>>
>> - Andreas
>>
>> PS: libfdk-aac produces a file that looks pretty much like the floating
>> point decoder from git master. Are you sure your patch is correct?
> 
> Yes, they duplicate the single channel in the stream and output it as
> stereo, something that should be done by a filter if that's what the
> user wants. Decoding a mono sample should generate a mono stream.

Not really. The channels are different.

- Andreas
James Almer July 22, 2022, 3:03 p.m. UTC | #8
On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> Should fix ticket #3361
>>>>>>>>
>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>> ---
>>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>>> before
>>>>>>>> pushing
>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which are now
>>>>>>>> decoded
>>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>>>
>>>>>>>
>>>>>>> We have both a fixed-point AAC as well as a floating point AAC
>>>>>>> decoder.
>>>>>>> Is there actually a test that tests that the output they produce is
>>>>>>> reasonably close? If not, could we make the test so that the same
>>>>>>> file
>>>>>>> is decoded once with the fixed-point and once with the floating-point
>>>>>>> decoder and then compared?
>>>>>>
>>>>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>>>>> files are going to affect both decoders, so a breakage would not be
>>>>>> detected if you compare their output with each other as they would
>>>>>> both
>>>>>> exhibit it.
>>>>>>
>>>>>
>>>>> I actually thought that the aac_fixed tests used checksums instead of
>>>>> ref files; then changes and breakages would be visible by changes to
>>>>> these files. Apparently I was wrong about that and the ref files are
>>>>> used for both aac and aac_fixed. But a test like the one outlined above
>>>>> would nevertheless obviate the need for a new ref file.
>>>>
>>>> Judging by
>>>> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
>>>>
>>>> it seems at least for these samples the fixed decoder does not generate
>>>> a decoded stream comparable to the float one, so I'll just upload a new
>>>> raw pcm file.
>>>
>>> When I decode both of these streams with git master, the left channel is
>>> pretty much identical, yet the right channel of the fixed-point decoder
>>> is silent and the right channel of the floating point decoder is not.
>>> With this patch applied, the result are two mono streams that are pretty
>>> much identical: The test sample created by the floating-point decoder
>>> works with the fixed-point decoder test (if one uncomments and modifies
>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
>>> upload new samples.
>>
>> Ok, can you suggest how to add a test that decodes with the fixed point
>> decoder then compares that with the output of the float decoder? Is
>> there a helper in fate.sh already for this?
>>
> 
> There is currently no helper in fate-run.sh for this.

Yeah, figures that's the case. Can you suggest how one would work for this?

> 
>>>
>>> - Andreas
>>>
>>> PS: libfdk-aac produces a file that looks pretty much like the floating
>>> point decoder from git master. Are you sure your patch is correct?
>>
>> Yes, they duplicate the single channel in the stream and output it as
>> stereo, something that should be done by a filter if that's what the
>> user wants. Decoding a mono sample should generate a mono stream.
> 
> Not really. The channels are different.

./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af 
channelmap=channel_layout=mono:map=0 -f md5 -

has the same result as

./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af 
channelmap=channel_layout=mono:map=1 -f md5 -

Same with the samples in the ticket.

> 
> - 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".
Andreas Rheinhardt July 22, 2022, 3:14 p.m. UTC | #9
James Almer:
> On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>>>> James Almer:
>>>>>>>>> Should fix ticket #3361
>>>>>>>>>
>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>> ---
>>>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>>>> before
>>>>>>>>> pushing
>>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which
>>>>>>>>> are now
>>>>>>>>> decoded
>>>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>>>>
>>>>>>>>
>>>>>>>> We have both a fixed-point AAC as well as a floating point AAC
>>>>>>>> decoder.
>>>>>>>> Is there actually a test that tests that the output they produce is
>>>>>>>> reasonably close? If not, could we make the test so that the same
>>>>>>>> file
>>>>>>>> is decoded once with the fixed-point and once with the
>>>>>>>> floating-point
>>>>>>>> decoder and then compared?
>>>>>>>
>>>>>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>>>>>> files are going to affect both decoders, so a breakage would not be
>>>>>>> detected if you compare their output with each other as they would
>>>>>>> both
>>>>>>> exhibit it.
>>>>>>>
>>>>>>
>>>>>> I actually thought that the aac_fixed tests used checksums instead of
>>>>>> ref files; then changes and breakages would be visible by changes to
>>>>>> these files. Apparently I was wrong about that and the ref files are
>>>>>> used for both aac and aac_fixed. But a test like the one outlined
>>>>>> above
>>>>>> would nevertheless obviate the need for a new ref file.
>>>>>
>>>>> Judging by
>>>>> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
>>>>>
>>>>>
>>>>> it seems at least for these samples the fixed decoder does not
>>>>> generate
>>>>> a decoded stream comparable to the float one, so I'll just upload a
>>>>> new
>>>>> raw pcm file.
>>>>
>>>> When I decode both of these streams with git master, the left
>>>> channel is
>>>> pretty much identical, yet the right channel of the fixed-point decoder
>>>> is silent and the right channel of the floating point decoder is not.
>>>> With this patch applied, the result are two mono streams that are
>>>> pretty
>>>> much identical: The test sample created by the floating-point decoder
>>>> works with the fixed-point decoder test (if one uncomments and modifies
>>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
>>>> upload new samples.
>>>
>>> Ok, can you suggest how to add a test that decodes with the fixed point
>>> decoder then compares that with the output of the float decoder? Is
>>> there a helper in fate.sh already for this?
>>>
>>
>> There is currently no helper in fate-run.sh for this.
> 
> Yeah, figures that's the case. Can you suggest how one would work for this?
> 

A new function in fate-run.sh seems to be necessary. Given that a
similar situation exists for AC-3 we should not hardcode aac; instead it
should have two parameters for the float and the fixed decoder. Then it
should decode the input file twice and do the same comparison that the
current tests use (they use the oneoff method, which results in
do_tiny_psnr with MAXDIFF being called).
I think the tests for the fixed-point decoder (with checksums) should be
separate, so that one can test this without the floating-point decoder
being present.

>>
>>>>
>>>> - Andreas
>>>>
>>>> PS: libfdk-aac produces a file that looks pretty much like the floating
>>>> point decoder from git master. Are you sure your patch is correct?
>>>
>>> Yes, they duplicate the single channel in the stream and output it as
>>> stereo, something that should be done by a filter if that's what the
>>> user wants. Decoding a mono sample should generate a mono stream.
>>
>> Not really. The channels are different.
> 
> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
> channelmap=channel_layout=mono:map=0 -f md5 -
> 
> has the same result as
> 
> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
> channelmap=channel_layout=mono:map=1 -f md5 -
> 
> Same with the samples in the ticket.
> 

This seems to be true for al_sbr_ps_04_new.mp4; but it is not true for
al_sbr_ps_06_new.mp4.

- Andreas
James Almer July 22, 2022, 3:37 p.m. UTC | #10
On 7/22/2022 12:14 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>>>>> James Almer:
>>>>>>>>>> Should fix ticket #3361
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>>>>> before
>>>>>>>>>> pushing
>>>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which
>>>>>>>>>> are now
>>>>>>>>>> decoded
>>>>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We have both a fixed-point AAC as well as a floating point AAC
>>>>>>>>> decoder.
>>>>>>>>> Is there actually a test that tests that the output they produce is
>>>>>>>>> reasonably close? If not, could we make the test so that the same
>>>>>>>>> file
>>>>>>>>> is decoded once with the fixed-point and once with the
>>>>>>>>> floating-point
>>>>>>>>> decoder and then compared?
>>>>>>>>
>>>>>>>> That wouldn't help much, i think. Almost all changes to *_template.c
>>>>>>>> files are going to affect both decoders, so a breakage would not be
>>>>>>>> detected if you compare their output with each other as they would
>>>>>>>> both
>>>>>>>> exhibit it.
>>>>>>>>
>>>>>>>
>>>>>>> I actually thought that the aac_fixed tests used checksums instead of
>>>>>>> ref files; then changes and breakages would be visible by changes to
>>>>>>> these files. Apparently I was wrong about that and the ref files are
>>>>>>> used for both aac and aac_fixed. But a test like the one outlined
>>>>>>> above
>>>>>>> would nevertheless obviate the need for a new ref file.
>>>>>>
>>>>>> Judging by
>>>>>> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=tests/fate/aac.mak;h=1743428f544fad8946dba11dd4ecec0630eb70a6;hb=HEAD#l117
>>>>>>
>>>>>>
>>>>>> it seems at least for these samples the fixed decoder does not
>>>>>> generate
>>>>>> a decoded stream comparable to the float one, so I'll just upload a
>>>>>> new
>>>>>> raw pcm file.
>>>>>
>>>>> When I decode both of these streams with git master, the left
>>>>> channel is
>>>>> pretty much identical, yet the right channel of the fixed-point decoder
>>>>> is silent and the right channel of the floating point decoder is not.
>>>>> With this patch applied, the result are two mono streams that are
>>>>> pretty
>>>>> much identical: The test sample created by the floating-point decoder
>>>>> works with the fixed-point decoder test (if one uncomments and modifies
>>>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
>>>>> upload new samples.
>>>>
>>>> Ok, can you suggest how to add a test that decodes with the fixed point
>>>> decoder then compares that with the output of the float decoder? Is
>>>> there a helper in fate.sh already for this?
>>>>
>>>
>>> There is currently no helper in fate-run.sh for this.
>>
>> Yeah, figures that's the case. Can you suggest how one would work for this?
>>
> 
> A new function in fate-run.sh seems to be necessary. Given that a
> similar situation exists for AC-3 we should not hardcode aac; instead it
> should have two parameters for the float and the fixed decoder. Then it
> should decode the input file twice and do the same comparison that the
> current tests use (they use the oneoff method, which results in
> do_tiny_psnr with MAXDIFF being called).
> I think the tests for the fixed-point decoder (with checksums) should be
> separate, so that one can test this without the floating-point decoder
> being present.
> 
>>>
>>>>>
>>>>> - Andreas
>>>>>
>>>>> PS: libfdk-aac produces a file that looks pretty much like the floating
>>>>> point decoder from git master. Are you sure your patch is correct?
>>>>
>>>> Yes, they duplicate the single channel in the stream and output it as
>>>> stereo, something that should be done by a filter if that's what the
>>>> user wants. Decoding a mono sample should generate a mono stream.
>>>
>>> Not really. The channels are different.
>>
>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>> channelmap=channel_layout=mono:map=0 -f md5 -
>>
>> has the same result as
>>
>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>> channelmap=channel_layout=mono:map=1 -f md5 -
>>
>> Same with the samples in the ticket.
>>
> 
> This seems to be true for al_sbr_ps_04_new.mp4; but it is not true for
> al_sbr_ps_06_new.mp4.

So looks like nearly a hundred samples into al_sbr_ps_06_new.mp4 frames 
start containing PS info. With this patch the decoder properly decodes 
the first hundred as mono, but since the decoder is locked, it will keep 
decoding the stereo samples as mono.

If i do

> diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
> index 4a0fa70ebc..9fd57043d6 100644
> --- a/libavcodec/aacdec_template.c
> +++ b/libavcodec/aacdec_template.c
> @@ -2576,8 +2576,8 @@ static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
>              ac->avctx->profile = FF_PROFILE_AAC_HE;
>          }
>          res = AAC_RENAME(ff_decode_sbr_extension)(ac, &che->sbr, gb, crc_flag, cnt, elem_type);
> -        if (ac->oc[1].m4ac.ps == 1 && ac->oc[1].status < OC_LOCKED &&
> -            ac->avctx->ch_layout.nb_channels == 1) {
> +        if (ac->oc[1].m4ac.ps == 1 && ac->avctx->ch_layout.nb_channels == 1) {
> +            push_output_configuration(ac);
>              output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
>                               ac->oc[1].status, 1);
>          }

Then it will properly start decoding the rest as stereo, but since 
ffmpeg.c already went with mono during configuration, it autoinserts a 
resample filter to keep outputting mono. Fun.

> 
> - 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".
Alex Converse July 22, 2022, 11 p.m. UTC | #11
On Fri, Jul 22, 2022 at 8:37 AM James Almer <jamrial@gmail.com> wrote:
> On 7/22/2022 12:14 PM, Andreas Rheinhardt wrote:
> > James Almer:
> >> On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
> >>> James Almer:
> >>>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
> >>>>> James Almer:
> >>>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
> >>>>>>> James Almer:
> >>>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
> >>>>>>>>> James Almer:
> >>>>>>>>>> Should fix ticket #3361
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>>>>>>>> ---
> >>>>>>>>>> This also needs an update to some fate ref samples i'll upload
> >>>>>>>>>> before
> >>>>>>>>>> pushing
> >>>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which
> >>>>>>>>>> are now
> >>>>>>>>>> decoded
> >>>>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
> >>>>>>>>>>
> >>>>>>>>>

[snip]

> >>>>>>
> >>>>>>
> >>>>>> it seems at least for these samples the fixed decoder does not
> >>>>>> generate
> >>>>>> a decoded stream comparable to the float one, so I'll just upload a
> >>>>>> new
> >>>>>> raw pcm file.
> >>>>>
> >>>>> When I decode both of these streams with git master, the left
> >>>>> channel is
> >>>>> pretty much identical, yet the right channel of the fixed-point decoder
> >>>>> is silent and the right channel of the floating point decoder is not.
> >>>>> With this patch applied, the result are two mono streams that are
> >>>>> pretty
> >>>>> much identical: The test sample created by the floating-point decoder
> >>>>> works with the fixed-point decoder test (if one uncomments and modifies
> >>>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
> >>>>> upload new samples.
> >>>>
> >>>> Ok, can you suggest how to add a test that decodes with the fixed point
> >>>> decoder then compares that with the output of the float decoder? Is
> >>>> there a helper in fate.sh already for this?
> >>>>
> >>>
> >>> There is currently no helper in fate-run.sh for this.
> >>
> >> Yeah, figures that's the case. Can you suggest how one would work for this?
> >>
> >
> > A new function in fate-run.sh seems to be necessary. Given that a
> > similar situation exists for AC-3 we should not hardcode aac; instead it
> > should have two parameters for the float and the fixed decoder. Then it
> > should decode the input file twice and do the same comparison that the
> > current tests use (they use the oneoff method, which results in
> > do_tiny_psnr with MAXDIFF being called).
> > I think the tests for the fixed-point decoder (with checksums) should be
> > separate, so that one can test this without the floating-point decoder
> > being present.
> >
> >>>
> >>>>>
> >>>>> - Andreas
> >>>>>
> >>>>> PS: libfdk-aac produces a file that looks pretty much like the floating
> >>>>> point decoder from git master. Are you sure your patch is correct?
> >>>>
> >>>> Yes, they duplicate the single channel in the stream and output it as
> >>>> stereo, something that should be done by a filter if that's what the
> >>>> user wants. Decoding a mono sample should generate a mono stream.
> >>>
> >>> Not really. The channels are different.
> >>
> >> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
> >> channelmap=channel_layout=mono:map=0 -f md5 -
> >>
> >> has the same result as
> >>
> >> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
> >> channelmap=channel_layout=mono:map=1 -f md5 -
> >>
> >> Same with the samples in the ticket.
> >>
> >
> > This seems to be true for al_sbr_ps_04_new.mp4; but it is not true for
> > al_sbr_ps_06_new.mp4.
>
> So looks like nearly a hundred samples into al_sbr_ps_06_new.mp4 frames
> start containing PS info. With this patch the decoder properly decodes
> the first hundred as mono, but since the decoder is locked, it will keep
> decoding the stereo samples as mono.
>

Hey all,

I thought I should share a little bit of context about this problem,
but I don't mean to come back from nowhere and try to overrule you
all. Do what you decide is best.

An HE-AACv2 decoder treating unsignaled mono as stereo is an
intentional design trade-off that the MPEG audio committee made. It is
a tradeoff that the FFmpeg decoder has mimicked for a number of years.
If you want to revisit the trade-off (and there may very well be good
reasons to do that) that's fine, but I think treating the current
behavior as a "bug" is the wrong approach.

In fact, those fate tests are based on a Coding Technologies test
suite designed to validate a decoder conformed to the MPEG behavior.

Parametric Stereo is nested inside the SBR extension after the main
SBR payload which itself is nested inside the AAC raw data block after
the main channel elements. It takes a full bitstream parse of both the
AAC and SBR layers and finding an SBR intra-frame to even see if PS is
present.


As for why I think MPEG made this trade-off (my opinion of why they did this) :
- It enables cutting (or joining a stream of) audio at arbitrary
frames without losing PS content or stereo detection.
- Most devices with mono output can support downmixing from stereo.
- Down mixing stereo to mono can be ugly with regard to phase but it
doesn't have nearly the complexity of the taste/environment/judgment
factor that surround sound to stereo downmix does.
- In transcode scenarios, most output codecs can support encoding
stereo formatted audio where both channels are identical quite
efficiently (even in AAC-LC with mid-side coding the overhead for
mono-coded as stereo is a single digit number of bits per frame).
- On playback on a stereo device it doesn't matter if the decoded
signal is one channel played out of both speakers or two channels that
are identical.
- This behavior can be overridden with more complicated signaling the
extradata (but this requires a transport that supports such signaling
and doesn't simply wrap ADTS).
- The folks working on iterating "MPEG-2 NBC" into the "MPEG-4 Audio"
monstrosity were primarily focused on getting their new features used.
James Almer July 22, 2022, 11:10 p.m. UTC | #12
On 7/22/2022 8:00 PM, Alex Converse wrote:
> On Fri, Jul 22, 2022 at 8:37 AM James Almer <jamrial@gmail.com> wrote:
>> On 7/22/2022 12:14 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>>>>>>> James Almer:
>>>>>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>>>>>>> James Almer:
>>>>>>>>>>>> Should fix ticket #3361
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>>>>>>> before
>>>>>>>>>>>> pushing
>>>>>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which
>>>>>>>>>>>> are now
>>>>>>>>>>>> decoded
>>>>>>>>>>>> properly as he_aac mono, so the .s16 files need to be replaced).
>>>>>>>>>>>>
>>>>>>>>>>>
> 
> [snip]
> 
>>>>>>>>
>>>>>>>>
>>>>>>>> it seems at least for these samples the fixed decoder does not
>>>>>>>> generate
>>>>>>>> a decoded stream comparable to the float one, so I'll just upload a
>>>>>>>> new
>>>>>>>> raw pcm file.
>>>>>>>
>>>>>>> When I decode both of these streams with git master, the left
>>>>>>> channel is
>>>>>>> pretty much identical, yet the right channel of the fixed-point decoder
>>>>>>> is silent and the right channel of the floating point decoder is not.
>>>>>>> With this patch applied, the result are two mono streams that are
>>>>>>> pretty
>>>>>>> much identical: The test sample created by the floating-point decoder
>>>>>>> works with the fixed-point decoder test (if one uncomments and modifies
>>>>>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a reason to
>>>>>>> upload new samples.
>>>>>>
>>>>>> Ok, can you suggest how to add a test that decodes with the fixed point
>>>>>> decoder then compares that with the output of the float decoder? Is
>>>>>> there a helper in fate.sh already for this?
>>>>>>
>>>>>
>>>>> There is currently no helper in fate-run.sh for this.
>>>>
>>>> Yeah, figures that's the case. Can you suggest how one would work for this?
>>>>
>>>
>>> A new function in fate-run.sh seems to be necessary. Given that a
>>> similar situation exists for AC-3 we should not hardcode aac; instead it
>>> should have two parameters for the float and the fixed decoder. Then it
>>> should decode the input file twice and do the same comparison that the
>>> current tests use (they use the oneoff method, which results in
>>> do_tiny_psnr with MAXDIFF being called).
>>> I think the tests for the fixed-point decoder (with checksums) should be
>>> separate, so that one can test this without the floating-point decoder
>>> being present.
>>>
>>>>>
>>>>>>>
>>>>>>> - Andreas
>>>>>>>
>>>>>>> PS: libfdk-aac produces a file that looks pretty much like the floating
>>>>>>> point decoder from git master. Are you sure your patch is correct?
>>>>>>
>>>>>> Yes, they duplicate the single channel in the stream and output it as
>>>>>> stereo, something that should be done by a filter if that's what the
>>>>>> user wants. Decoding a mono sample should generate a mono stream.
>>>>>
>>>>> Not really. The channels are different.
>>>>
>>>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>>>> channelmap=channel_layout=mono:map=0 -f md5 -
>>>>
>>>> has the same result as
>>>>
>>>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>>>> channelmap=channel_layout=mono:map=1 -f md5 -
>>>>
>>>> Same with the samples in the ticket.
>>>>
>>>
>>> This seems to be true for al_sbr_ps_04_new.mp4; but it is not true for
>>> al_sbr_ps_06_new.mp4.
>>
>> So looks like nearly a hundred samples into al_sbr_ps_06_new.mp4 frames
>> start containing PS info. With this patch the decoder properly decodes
>> the first hundred as mono, but since the decoder is locked, it will keep
>> decoding the stereo samples as mono.
>>
> 
> Hey all,
> 
> I thought I should share a little bit of context about this problem,
> but I don't mean to come back from nowhere and try to overrule you
> all. Do what you decide is best.
> 
> An HE-AACv2 decoder treating unsignaled mono as stereo is an
> intentional design trade-off that the MPEG audio committee made. It is
> a tradeoff that the FFmpeg decoder has mimicked for a number of years.
> If you want to revisit the trade-off (and there may very well be good
> reasons to do that) that's fine, but I think treating the current
> behavior as a "bug" is the wrong approach.
> 
> In fact, those fate tests are based on a Coding Technologies test
> suite designed to validate a decoder conformed to the MPEG behavior.
> 
> Parametric Stereo is nested inside the SBR extension after the main
> SBR payload which itself is nested inside the AAC raw data block after
> the main channel elements. It takes a full bitstream parse of both the
> AAC and SBR layers and finding an SBR intra-frame to even see if PS is
> present.
> 
> 
> As for why I think MPEG made this trade-off (my opinion of why they did this) :
> - It enables cutting (or joining a stream of) audio at arbitrary
> frames without losing PS content or stereo detection.
> - Most devices with mono output can support downmixing from stereo.
> - Down mixing stereo to mono can be ugly with regard to phase but it
> doesn't have nearly the complexity of the taste/environment/judgment
> factor that surround sound to stereo downmix does.
> - In transcode scenarios, most output codecs can support encoding
> stereo formatted audio where both channels are identical quite
> efficiently (even in AAC-LC with mid-side coding the overhead for
> mono-coded as stereo is a single digit number of bits per frame).
> - On playback on a stereo device it doesn't matter if the decoded
> signal is one channel played out of both speakers or two channels that
> are identical.
> - This behavior can be overridden with more complicated signaling the
> extradata (but this requires a transport that supports such signaling
> and doesn't simply wrap ADTS).
> - The folks working on iterating "MPEG-2 NBC" into the "MPEG-4 Audio"
> monstrosity were primarily focused on getting their new features used.

Thanks a lot for clarifying this. This "bug" has been pending for nearly 
a decade now...

So i withdraw this patch and I'll close the ticket as invalid. I'll then 
see if adding a downmix input option is feasible, but the user could 
just request to downmix to mono by a filter down the chain (which is 
what the audiomatch tests do), so maybe it's not that useful.
James Almer Aug. 2, 2022, 5:48 p.m. UTC | #13
On 7/22/2022 8:10 PM, James Almer wrote:
> On 7/22/2022 8:00 PM, Alex Converse wrote:
>> On Fri, Jul 22, 2022 at 8:37 AM James Almer <jamrial@gmail.com> wrote:
>>> On 7/22/2022 12:14 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 7/22/2022 11:56 AM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 7/22/2022 11:23 AM, Andreas Rheinhardt wrote:
>>>>>>>> James Almer:
>>>>>>>>> On 7/18/2022 10:57 AM, Andreas Rheinhardt wrote:
>>>>>>>>>> James Almer:
>>>>>>>>>>> On 7/14/2022 9:10 AM, Andreas Rheinhardt wrote:
>>>>>>>>>>>> James Almer:
>>>>>>>>>>>>> Should fix ticket #3361
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> This also needs an update to some fate ref samples i'll upload
>>>>>>>>>>>>> before
>>>>>>>>>>>>> pushing
>>>>>>>>>>>>> (fate-aac-al_sbr_ps_04_ur and fate-aac-al_sbr_ps_06_ur which
>>>>>>>>>>>>> are now
>>>>>>>>>>>>> decoded
>>>>>>>>>>>>> properly as he_aac mono, so the .s16 files need to be 
>>>>>>>>>>>>> replaced).
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>
>> [snip]
>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> it seems at least for these samples the fixed decoder does not
>>>>>>>>> generate
>>>>>>>>> a decoded stream comparable to the float one, so I'll just 
>>>>>>>>> upload a
>>>>>>>>> new
>>>>>>>>> raw pcm file.
>>>>>>>>
>>>>>>>> When I decode both of these streams with git master, the left
>>>>>>>> channel is
>>>>>>>> pretty much identical, yet the right channel of the fixed-point 
>>>>>>>> decoder
>>>>>>>> is silent and the right channel of the floating point decoder is 
>>>>>>>> not.
>>>>>>>> With this patch applied, the result are two mono streams that are
>>>>>>>> pretty
>>>>>>>> much identical: The test sample created by the floating-point 
>>>>>>>> decoder
>>>>>>>> works with the fixed-point decoder test (if one uncomments and 
>>>>>>>> modifies
>>>>>>>> the latter). So the issue with aac-al_sbr_ps_06_ur is not a 
>>>>>>>> reason to
>>>>>>>> upload new samples.
>>>>>>>
>>>>>>> Ok, can you suggest how to add a test that decodes with the fixed 
>>>>>>> point
>>>>>>> decoder then compares that with the output of the float decoder? Is
>>>>>>> there a helper in fate.sh already for this?
>>>>>>>
>>>>>>
>>>>>> There is currently no helper in fate-run.sh for this.
>>>>>
>>>>> Yeah, figures that's the case. Can you suggest how one would work 
>>>>> for this?
>>>>>
>>>>
>>>> A new function in fate-run.sh seems to be necessary. Given that a
>>>> similar situation exists for AC-3 we should not hardcode aac; 
>>>> instead it
>>>> should have two parameters for the float and the fixed decoder. Then it
>>>> should decode the input file twice and do the same comparison that the
>>>> current tests use (they use the oneoff method, which results in
>>>> do_tiny_psnr with MAXDIFF being called).
>>>> I think the tests for the fixed-point decoder (with checksums) 
>>>> should be
>>>> separate, so that one can test this without the floating-point decoder
>>>> being present.
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> - Andreas
>>>>>>>>
>>>>>>>> PS: libfdk-aac produces a file that looks pretty much like the 
>>>>>>>> floating
>>>>>>>> point decoder from git master. Are you sure your patch is correct?
>>>>>>>
>>>>>>> Yes, they duplicate the single channel in the stream and output 
>>>>>>> it as
>>>>>>> stereo, something that should be done by a filter if that's what the
>>>>>>> user wants. Decoding a mono sample should generate a mono stream.
>>>>>>
>>>>>> Not really. The channels are different.
>>>>>
>>>>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>>>>> channelmap=channel_layout=mono:map=0 -f md5 -
>>>>>
>>>>> has the same result as
>>>>>
>>>>> ./ffmpeg -c:a libfdk_aac -i ../samples/aac/al_sbr_ps_04_new.mp4 -af
>>>>> channelmap=channel_layout=mono:map=1 -f md5 -
>>>>>
>>>>> Same with the samples in the ticket.
>>>>>
>>>>
>>>> This seems to be true for al_sbr_ps_04_new.mp4; but it is not true for
>>>> al_sbr_ps_06_new.mp4.
>>>
>>> So looks like nearly a hundred samples into al_sbr_ps_06_new.mp4 frames
>>> start containing PS info. With this patch the decoder properly decodes
>>> the first hundred as mono, but since the decoder is locked, it will keep
>>> decoding the stereo samples as mono.
>>>
>>
>> Hey all,
>>
>> I thought I should share a little bit of context about this problem,
>> but I don't mean to come back from nowhere and try to overrule you
>> all. Do what you decide is best.
>>
>> An HE-AACv2 decoder treating unsignaled mono as stereo is an
>> intentional design trade-off that the MPEG audio committee made. It is
>> a tradeoff that the FFmpeg decoder has mimicked for a number of years.
>> If you want to revisit the trade-off (and there may very well be good
>> reasons to do that) that's fine, but I think treating the current
>> behavior as a "bug" is the wrong approach.
>>
>> In fact, those fate tests are based on a Coding Technologies test
>> suite designed to validate a decoder conformed to the MPEG behavior.
>>
>> Parametric Stereo is nested inside the SBR extension after the main
>> SBR payload which itself is nested inside the AAC raw data block after
>> the main channel elements. It takes a full bitstream parse of both the
>> AAC and SBR layers and finding an SBR intra-frame to even see if PS is
>> present.
>>
>>
>> As for why I think MPEG made this trade-off (my opinion of why they 
>> did this) :
>> - It enables cutting (or joining a stream of) audio at arbitrary
>> frames without losing PS content or stereo detection.
>> - Most devices with mono output can support downmixing from stereo.
>> - Down mixing stereo to mono can be ugly with regard to phase but it
>> doesn't have nearly the complexity of the taste/environment/judgment
>> factor that surround sound to stereo downmix does.
>> - In transcode scenarios, most output codecs can support encoding
>> stereo formatted audio where both channels are identical quite
>> efficiently (even in AAC-LC with mid-side coding the overhead for
>> mono-coded as stereo is a single digit number of bits per frame).
>> - On playback on a stereo device it doesn't matter if the decoded
>> signal is one channel played out of both speakers or two channels that
>> are identical.
>> - This behavior can be overridden with more complicated signaling the
>> extradata (but this requires a transport that supports such signaling
>> and doesn't simply wrap ADTS).
>> - The folks working on iterating "MPEG-2 NBC" into the "MPEG-4 Audio"
>> monstrosity were primarily focused on getting their new features used.
> 
> Thanks a lot for clarifying this. This "bug" has been pending for nearly 
> a decade now...
> 
> So i withdraw this patch and I'll close the ticket as invalid. I'll then 
> see if adding a downmix input option is feasible, but the user could 
> just request to downmix to mono by a filter down the chain (which is 
> what the audiomatch tests do), so maybe it's not that useful.

Do you know for that matter where in the spec is this defined? I see 
fdk-aac also does the same, so it's clearly intentional, but where is it 
specified?
diff mbox series

Patch

diff --git a/libavcodec/aacdec_template.c b/libavcodec/aacdec_template.c
index 10fba3d3b2..15c20c07d6 100644
--- a/libavcodec/aacdec_template.c
+++ b/libavcodec/aacdec_template.c
@@ -967,8 +967,7 @@  static int decode_ga_specific_config(AACContext *ac, AVCodecContext *avctx,
 
     if (count_channels(layout_map, tags) > 1) {
         m4ac->ps = 0;
-    } else if (m4ac->sbr == 1 && m4ac->ps == -1)
-        m4ac->ps = 1;
+    }
 
     if (ac && (ret = output_configure(ac, layout_map, tags, OC_GLOBAL_HDR, 0)))
         return ret;
@@ -2572,18 +2571,16 @@  static int decode_extension_payload(AACContext *ac, GetBitContext *gb, int cnt,
             av_log(ac->avctx, AV_LOG_ERROR, "Implicit SBR was found with a first occurrence after the first frame.\n");
             skip_bits_long(gb, 8 * cnt - 4);
             return res;
-        } else if (ac->oc[1].m4ac.ps == -1 && ac->oc[1].status < OC_LOCKED &&
-                   ac->avctx->ch_layout.nb_channels == 1) {
-            ac->oc[1].m4ac.sbr = 1;
-            ac->oc[1].m4ac.ps = 1;
-            ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
-            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
-                             ac->oc[1].status, 1);
         } else {
             ac->oc[1].m4ac.sbr = 1;
             ac->avctx->profile = FF_PROFILE_AAC_HE;
         }
         res = AAC_RENAME(ff_decode_sbr_extension)(ac, &che->sbr, gb, crc_flag, cnt, elem_type);
+        if (ac->oc[1].m4ac.ps == 1 && ac->oc[1].status < OC_LOCKED &&
+            ac->avctx->ch_layout.nb_channels == 1) {
+            output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
+                             ac->oc[1].status, 1);
+        }
         break;
     case EXT_DYNAMIC_RANGE:
         res = decode_dynamic_range(&ac->che_drc, gb);
diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
index b72c94b76d..f9925b40e5 100644
--- a/libavcodec/aacsbr_template.c
+++ b/libavcodec/aacsbr_template.c
@@ -954,6 +954,7 @@  static void read_sbr_extension(AACContext *ac, SpectralBandReplication *sbr,
             *num_bits_left = 0;
         } else {
             *num_bits_left -= ff_ps_read_data(ac->avctx, gb, &sbr->ps.common, *num_bits_left);
+            ac->oc[1].m4ac.ps = 1;
             ac->avctx->profile = FF_PROFILE_AAC_HE_V2;
         }
         break;
diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak
index 68a396e187..7dd8ceb142 100644
--- a/tests/fate/gapless.mak
+++ b/tests/fate/gapless.mak
@@ -47,27 +47,27 @@  fate-audiomatch-square-aac: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/squar
 
 fate-audiomatch-afconvert-16000-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav
 fate-audiomatch-afconvert-16000-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav
-fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
-fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ac 1 -ar 16000"
+fate-audiomatch-afconvert-16000-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
+fate-audiomatch-afconvert-16000-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_mono.wav "-ar 16000"
 fate-audiomatch-afconvert-16000-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav
 fate-audiomatch-afconvert-16000-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav
 fate-audiomatch-afconvert-16000-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
 fate-audiomatch-afconvert-16000-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
-fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
+fate-audiomatch-afconvert-16000-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ac 2 -ar 16000"
 fate-audiomatch-afconvert-16000-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_16000_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_16000_stereo.wav "-ar 16000"
 fate-audiomatch-afconvert-44100-mono-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
 fate-audiomatch-afconvert-44100-mono-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
-fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
-fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
+fate-audiomatch-afconvert-44100-mono-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_mono.wav
+fate-audiomatch-afconvert-44100-mono-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_mono_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_mono.wav
 fate-audiomatch-afconvert-44100-stereo-lc-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 fate-audiomatch-afconvert-44100-stereo-lc-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_lc.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 fate-audiomatch-afconvert-44100-stereo-he-adts: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.adts  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 fate-audiomatch-afconvert-44100-stereo-he-m4a:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he.m4a   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
-fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav
+fate-audiomatch-afconvert-44100-stereo-he2-adts:CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.adts $(SAMPLES)/audiomatch/tones_44100_stereo.wav "-ac 2"
 fate-audiomatch-afconvert-44100-stereo-he2-m4a: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_afconvert_44100_stereo_aac_he2.m4a  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 
 fate-audiomatch-dolby-44100-mono-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
-fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav "-ac 1"
+fate-audiomatch-dolby-44100-mono-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_mono_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_mono.wav
 fate-audiomatch-dolby-44100-stereo-lc-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_lc.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 fate-audiomatch-dolby-44100-stereo-he-mp4:  CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he.mp4   $(SAMPLES)/audiomatch/tones_44100_stereo.wav
 fate-audiomatch-dolby-44100-stereo-he2-mp4: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/tones_dolby_44100_stereo_aac_he2.mp4  $(SAMPLES)/audiomatch/tones_44100_stereo.wav
diff --git a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
index 32b2627946..527c9acdba 100644
--- a/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
+++ b/tests/ref/fate/audiomatch-afconvert-16000-stereo-he2-adts
@@ -1 +1 @@ 
-presig: 5186 postsig:446 c: 0.9839 lenerr:5632
+presig: 5154 postsig:446 c: 0.9839 lenerr:5600