[FFmpeg-devel] [PATCH] avfilter/mpegaudiodec_template: Fix wrong decision of illegal

[Michael Niedermayer]

On Wed, Jan 08, 2020 at 04:32:39PM +0900, Ted Lee wrote:
> Dear FFmpeg developers,
> 
> I'm glad to have a chance to contribute to FFmpeg.
> 
> Since this is the first time for me, please give feedback if I missed
> something. I will reflect on that.
> 
> *Summary:*
> 
>    - Test signal: 790_pomnyun_taebaek2.mp3
>       - MP3 file with the same left channel and right channel except for
>       the first 7 seconds and the last 10 seconds.
>       (download:
>       <https://www.dropbox.com/s/sivz18sixvhoo66/790_pomnyun_taebaek2.mp3?dl=0>
>       https://www.dropbox.com/s/sivz18sixvhoo66/790_pomnyun_taebaek2.mp3?dl=0
>       )
>    - When decoding the test signal using FFmpeg as below, a difference
>    occurs in the same part of L and R.
>       - $ffmpeg -i 790_pomnyun_taebaek2.mp3 -acodec pcm_s16le
>       790_pomnyun_taebaek2.wav
> 
> 
> Expected result (Adobe Audition or Core Audio):
> 
> Decoded PCM
> [image: image.png]
> (L-R)
> [image: image.png]
> (You can reproduce it using Adobe Audition or CoreAudio of MacOS)
> 
> Actual result (FFmpeg):
> 
> Decoded PCM
> [image: image.png]
> 
> (L-R)
> [image: image.png]
> 
> *Why it happens?:*
> 
>    - In MPEG-1 Layer 3 (MP3), the illegal position of intensity stereo is
>    defined as the maximum value of scale factor. Since intensity stereo
>    positions (scale factor of the right channel) are always allocated to 3
>    bits, intensity stereo positions are limited from 0 to 6 and 7 is treated
>    as the illegal position.
>    - In MPEG-2 LSF, the illegal position is also defined as the maximum
>    value of scale factor, but the number of bits of the scale factor is not
>    fixed. So the number of bits needs to be calculated (slen[]) and the
>    illegal stereo position should be changed as the maximum value. However, in
>    the current implementation, the illegal position is defined as fixed value
>    16.
> 
> *How to fix this?:*
> 
>    - The maximum value of scale factor (the illegal position) should be
>    store to sf_max_table[] and an illegal position is determined when the
>    scale factor is sf_max_table[](sf_max).
> 
> 
> From 754545f6c675aede07abd912992f9ed4a17d4ddc Mon Sep 17 00:00:00 2001
> From: Ted <t at gaudiolab.com>
> Date: Wed, 8 Jan 2020 11:53:56 +0900
> Subject: [PATCH] avfilter/mpegaudiodec_template: Fix wrong decision of
> illegal
>  intensity stereo position for MPEG-2 LSF
> 
> Signed-off-by: Ted <t at gaudiolab.com>
> ---
>  libavcodec/mpegaudiodec_template.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/mpegaudiodec_template.c
> b/libavcodec/mpegaudiodec_template.c
> index 3f1674e827..d6d7cd5ad0 100644
> --- a/libavcodec/mpegaudiodec_template.c
> +++ b/libavcodec/mpegaudiodec_template.c
> @@ -115,6 +115,7 @@ static uint16_t band_index_long[9][23];
>  static INTFLOAT is_table[2][16];
>  static INTFLOAT is_table_lsf[2][2][16];
>  static INTFLOAT csa_table[8][4];
> +static uint8_t sf_max_table[40];

[...]


>                  v1 = is_tab[0][sf];
> @@ -1523,11 +1527,15 @@ static int mp_decode_layer3(MPADecodeContext *s)
>                      n  = lsf_nsf_table[tindex2][tindex][k];
>                      sl = slen[k];
>                      if (sl) {
> -                        for (i = 0; i < n; i++)
> +                        for (i = 0; i < n; i++) {

> +                            sf_max_table[j] = (uint8_t) exp2(sl) - 1;

non constant statics do not work because there can be more than one mp3
decoder instance
also the exp2() very likely should be a simple shift

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200108/45f2a987/attachment.sig>