[Mplayer-dev-eng] clicks/noise from AC3 audio decoder using 3dnowex instructions (AMD K7)

[Nick Kurshev]

Hi, Juergen Keil!

On 2001-06-13 12:05:56 you wrote:

>Hi,
>
>there's a problem in libac3/imdct.c and libac3/mmx/imdct_3dnow.c, function
>imdct_do_512_nol().  In both files the routine ends with:
>
>	...
>	
>        data_ptr = data;
>        delay_ptr = delay;
>        window_ptr = window;
>
>        /* Window and convert to real valued signal, no overlap here*/
>[1]     for(i=0; i< 64; i++) {
>               *data_ptr++   = -buf[64+i].im   * *window_ptr++;
>               *data_ptr++   =  buf[64-i-1].re * *window_ptr++;
>        }
>[2]
>	/* The trailing edge of the window goes into the delay line */
>
>   	delay_ptr = delay;
>
>[3]     for(i=0; i< 64; i++) {
>               *delay_ptr++  = -buf[64+i].re   * *--window_ptr;
>               *delay_ptr++  =  buf[64-i-1].im * *--window_ptr;
>        }
>
>[4]     for(i=0; i<64; i++) {
>               *delay_ptr++  =  buf[i].im       * *--window_ptr;
>               *delay_ptr++  = -buf[128-i-1].re * *--window_ptr;
>        }
>}
>
>
>'window' is an array with 256 float elements.  The loop at [1] uses the
>elements 0 .. 127 (via window_ptr).  The loop at [3] use the same elements
>in reverse order, 127 .. 0.
>
>And finally the loop at [4] continues looping backwards, using elements 
>-1 .. -128 .  Oops.  It access out-of-bound array elements,  which obviouly
>is not ok.
>
>
>Looking at the imdct_do_256*() variant of the code, I have the impression
>that a second loop accessing the array elements 128 .. 255 is missing here [2].
>With such a second loop added, [3] would access the elements 255 .. 128 and
>[4] 127 .. 0.  The appended patch adds the missing second loop.  DCT experts
>out there,  does that fix look OK?
>
>
>
>The patch fixes some stange audio clicks I get on dvd video playback;
>these clicks 'appeared' a few days ago with the mplayer cvs version due
>to an unrelated change in mp3lib/decode_k7.s which triggered this noise
>problem for me.
>
FIRST: IT'S NOT PROBLEM OF mp3lib/decode_k7.s
IT'S PROBLEM LIBAC3.
SECOND:
After building a diff with ignoring space changing I've got:
@@ -254,6 +254,11 @@
                *data_ptr++   = buf[64-i-1].re * *window_ptr++;
         }
 
+	for(i=0; i< 64; i++) {
+		*data_ptr++  = -buf[i].re       * *window_ptr++;
+		*data_ptr++  =  buf[128-i-1].im * *window_ptr++;
+	}
+
         /* The trailing edge of the window goes into the delay line */
 
    delay_ptr = delay;

My congratulations!
In current CVS three at www.linuxvideo.org this function contain the same changes.
IMHO it would be better to sync with them, Arpi?
Simply I never have samples which contain calls to XXX_nol functions, but your samples
has 2 calls - for imdct_do_512_nol and imdct_do_256_nol (except very overloaded imdct_do_512).
Would you like to find out in your samples something from untested parts:
It's very easy! Currently 3dnow optimization is applied only to
imdct_do_512
fft_asmb(16) from srfft.c sources
and was tested with 
downmix_3f_2r_to_2ch as CASE 7
and stream_sample_2ch_to_s16 from downmix_3dnow.S
Now I can apply 3dnow optimizations to:
imdct_do_512_nol and 
imdct_do_256_nol
**********************
But for libac3 are still untested:
***********************
imdct_do_256 from imdtc.c source.
downmix_2f_2r_to_2ch
downmix_3f_1r_to_2ch
downmix_2f_1r_to_2ch
downmix_3f_0r_to_2ch
stream_sample_1ch_to_s16
from downmix_3dnow.S
Simply you can put printf to imdct.c to body of imdct function to see
what subroutines are called.

Or maybe someone want to do that? (LGB for instance)

Best regards! Nick (2001-06-13 20:36:35)


_______________________________________________
Mplayer-dev-eng mailing list
Mplayer-dev-eng at lists.sourceforge.net
http://lists.sourceforge.net/lists/listinfo/mplayer-dev-eng