The regression was introduced in https://hg.libsdl.org/SDL/rev/a8382e3d0b54

Seems it's not specific to downsampling;
"testresample SDL/test/sample.wav out.wav 44100 1" also causes ASan issues - reading outside of ResamplerFilter. (that case is upsampling from 22050Hz).

Has there been any patch posted for this?

Created attachment 2973 [details]
quick hack fix

Some more info - for my test case of "testresample SDL/test/sample.wav output.wav 11025 1", it fails at i=16920 of the outer "for (i = 0; i < outframes; i++)" loop in SDL_ResampleAudio.
outframes = 120428
interpolation1 = -0.00263154507
filterindex1 = -1
"filterindex1 + (j * RESAMPLER_SAMPLES_PER_ZERO_CROSSING)" = -1

This looks like a floating point precision thing.. I tried changing the position bookkeeping variables from float to double and this fixed the immediate problem.

Memory accesses before the buffer when downsampling are fixed in https://hg.libsdl.org/SDL/rev/b5e404b928ea, will be looking into the overflow when upsampling shortly.

--ryan.

Cool. Looks like the overflow when upsampling is from the repeated addition to calculate outtime:

(lldb) print outtime
(float) $5 = 10.9461832
(lldb) print outtimeincr * i
(float) $6 = 10.8856688

outtime is just outtimeincr added "i" times, so just replace it with this?

   const outtime = outtimeincr * i;

(In reply to Ryan C. Gordon from comment #5)
> Memory accesses before the buffer when downsampling are fixed in
> https://hg.libsdl.org/SDL/rev/b5e404b928ea

valgrind still complains when downsampling (x86_64-Linux, Fedora 20, gcc-4.8.4):

$ file m_moving.wav
m_moving.wav: RIFF (little-endian) data, WAVE audio, Microsoft PCM, 16 bit, stereo 22050 Hz

$ valgrind ./testresample m_moving.wav 1.wav 11025 1
==27121== Memcheck, a memory error detector
==27121== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==27121== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==27121== Command: ./testresample m_moving.wav 1.wav 11025 1
==27121== 
==27121== Invalid read of size 4
==27121==    at 0x4C2EC60: SDL_ResampleCVT_c1 (SDL_audiocvt.c:524)
==27121==    by 0x4C303C6: SDL_ConvertAudio_REAL (SDL_audiocvt.c:555)
==27121==    by 0x400BD0: main (testresample.c:69)
==27121==  Address 0x5252e30 is 0 bytes after a block of size 4,096 alloc'd
==27121==    at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==27121==    by 0x4C2EA43: SDL_ResampleCVT_c1 (SDL_audiocvt.c:724)
==27121==    by 0x4C303C6: SDL_ConvertAudio_REAL (SDL_audiocvt.c:555)
==27121==    by 0x400BD0: main (testresample.c:69)
==27121== 
==27121== 
==27121== HEAP SUMMARY:
==27121==     in use at exit: 75,458 bytes in 211 blocks
==27121==   total heap usage: 721 allocs, 510 frees, 14,010,016 bytes allocated
==27121== 
==27121== LEAK SUMMARY:
==27121==    definitely lost: 0 bytes in 0 blocks
==27121==    indirectly lost: 0 bytes in 0 blocks
==27121==      possibly lost: 0 bytes in 0 blocks
==27121==    still reachable: 75,458 bytes in 211 blocks
==27121==         suppressed: 0 bytes in 0 blocks
==27121== Rerun with --leak-check=full to see details of leaked memory
==27121== 
==27121== For counts of detected and suppressed errors, rerun with: -v
==27121== ERROR SUMMARY: 4374 errors from 1 contexts (suppressed: 2 from 2)

Better trace after building without any -O? flags:

==19100== Invalid read of size 4
==19100==    at 0x4C30958: SDL_ResampleAudio (SDL_audiocvt.c:524)
==19100==    by 0x4C31243: SDL_ResampleCVT (SDL_audiocvt.c:730)
==19100==    by 0x4C312F3: SDL_ResampleCVT_c1 (SDL_audiocvt.c:750)
==19100==    by 0x4C2EFDD: SDL_ConvertStereoToMono_SSE3 (SDL_audiocvt.c:71)
==19100==    by 0x4C33E6E: SDL_Convert_S16_to_F32_SSE2 (SDL_audiotypecvt.c:424)
==19100==    by 0x4C30ACA: SDL_ConvertAudio_REAL (SDL_audiocvt.c:555)
==19100==    by 0x4C3EA61: SDL_ConvertAudio (SDL_dynapi_procs.h:120)
==19100==    by 0x400E2D: main (testresample.c:69)
==19100==  Address 0x528ad30 is 0 bytes after a block of size 4,096 alloc'd
==19100==    at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==19100==    by 0x4C9D474: SDL_calloc_REAL (SDL_malloc.c:41)
==19100==    by 0x4C311F7: SDL_ResampleCVT (SDL_audiocvt.c:724)
==19100==    by 0x4C312F3: SDL_ResampleCVT_c1 (SDL_audiocvt.c:750)
==19100==    by 0x4C2EFDD: SDL_ConvertStereoToMono_SSE3 (SDL_audiocvt.c:71)
==19100==    by 0x4C33E6E: SDL_Convert_S16_to_F32_SSE2 (SDL_audiotypecvt.c:424)
==19100==    by 0x4C30ACA: SDL_ConvertAudio_REAL (SDL_audiocvt.c:555)
==19100==    by 0x4C3EA61: SDL_ConvertAudio (SDL_dynapi_procs.h:120)
==19100==    by 0x400E2D: main (testresample.c:69)

Just pushed some fixes, which I think should solve all the issues, knock on wood.

The "outtimeincr * i;" change seemed good, but it introduced clicks in the sine wave I was resampling. No idea why. Eventually, I resorted to using double instead of float, which also avoids the buffer overflows.

If everyone can give this a shot and see if it still fails for them, I'd appreciate it.

--ryan.

Created attachment 2976 [details]
Sine wave.


Here's the sine wave file I'm using for testing, in case anyone wants to play with it.

(for example, "hg update 61c151bcd76a" if you want to see what's up with that floating point accumulation issue.)

Built with Address Sanitizer, I'm using this shell script:

for w in 8000 11025 22050 44100 48000 96000 192000 ; do
    echo $w ...
    ./testresample sine.wav $w.wav $w 1
done

(one could stick Valgrind in there trivially, too.)

I also have this line stuck into the disk audio driver, right after the "SDL_zerop(this->hidden);" line:

{ const char *envr = SDL_getenv("FREQ"); if (envr) { this->spec.freq = SDL_atoi(envr); } }

So I can do this...

for feh in 8000 11025 22050 44100 48000 96000 192000 ; do 
    SDL_AUDIODRIVER=disk SDL_DISKAUDIOFILE=$w.raw FREQ=$w ./loopwave
done

(hit ctrl-c every few seconds until it finishes.)

Then I import the raw audio into Audacity and look at the waveforms and listen for problems. Note that Audacity (or maybe just my old build of it) fails to load 192000Hz audio correctly, but I believe this to be an Audacity issue and not SDL.

Note that there's a gap in the wave file itself, so a click every few seconds is expected, as loopwave repeats the data.

--ryan.

(In reply to Ryan C. Gordon from comment #10)
>     SDL_AUDIODRIVER=disk SDL_DISKAUDIOFILE=$w.raw FREQ=$w ./loopwave

(oh, that should be "./loopwave sine.wav" unless you want to rename it the default "sample.wav")

--ryan.

(In reply to Ryan C. Gordon from comment #10)

> for feh in 8000 11025 22050 44100 48000 96000 192000 ; do 
>     SDL_AUDIODRIVER=disk SDL_DISKAUDIOFILE=$w.raw FREQ=$w ./loopwave

that should be "for w in 8000" ...typing too quickly.  :)

r11596 seems to have fixed the issue valgrind reported for me.

Segfaults / asan issues seem fixed as of https://hg.libsdl.org/SDL/rev/9d8ea0382c52 for me as well.

Yeah - taking a closer look, I also get the severe distortion with "const double outtime = i * outtimeincr;"... that is puzzling!

Thanks for the debugging setup info, will give that a shot as I've just been launching one-off tests from Xcode. BTW - I'm using "sox" to make spectrograms like this:

  sox in.wav -n spectrogram -o out.png

They're higher-resolution than audacity's spectrogram option so it might be handy.



The last issue I'm seeing (this should be a new bug I guess) is when upsampling 11025Hz to 44100 (or 22050 to 44100), the resampler doesn't seem to be filtering out the frequencies above the nyquist frequency (0.5*samplerate) of the original file, as it should. You can hear this as high frequency noise in SDL's "test/sample.wav", compared to a reference resampler (e.g. "sox test/sample.wav out.wav rate 44100"). I'm going to take a look and see if I can see any obvious fix.

Eric, the original bug report should be fixed. Can you please enter a new bug for the high frequency noise?

Thanks!

Sure, I created https://bugzilla.libsdl.org/show_bug.cgi?id=3878 for that