Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New resampler: ASan error / segfault during downsampling #2634

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

New resampler: ASan error / segfault during downsampling #2634

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

@SDLBugzilla SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.1
Reported for operating system, platform: Other, x86

Comments on the original bug report:

On 2017-09-26 04:32:29 +0000, Eric Wasylishen wrote:

I ran the "testresample" test program with the arguments "SDL/test/sample.wav output.wav 11025 1", in Xcode with Address Sanitizer enabled. (the sample.wav is 22050Hz so it's downsampling with a factor of 0.5) Tested on https://hg.libsdl.org/SDL/rev/8ce1c541181d

It aborted with an Address Sanitizer error when trying to read ResamplerFilter[-1] at SDL_audiocvt.c:511.

I also got a segfault without Address Sanitizer when resampling a 44100Hz file to 22050Hz.

Is the code handling the "When (ratio) < 1" section of https://ccrma.stanford.edu/~jos/resample/Implementation.html (after eq. 5)?

On 2017-10-04 09:30:06 +0000, janisozaur wrote:

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

On 2017-10-04 17:37:24 +0000, Eric Wasylishen wrote:

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).

On 2017-10-09 20:10:03 +0000, Ozkan Sezer wrote:

Has there been any patch posted for this?

On 2017-10-11 05:35:58 +0000, Eric Wasylishen wrote:

Created attachment 2973
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.

On 2017-10-11 06:37:17 +0000, Ryan C. Gordon wrote:

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.

On 2017-10-11 06:58:43 +0000, Eric Wasylishen wrote:

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;

On 2017-10-11 07:55:36 +0000, Ozkan Sezer wrote:

(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)

On 2017-10-11 12:14:58 +0000, Ozkan Sezer wrote:

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)

On 2017-10-11 16:11:10 +0000, Ryan C. Gordon wrote:

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.

On 2017-10-11 16:20:32 +0000, Ryan C. Gordon wrote:

Created attachment 2976
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.

On 2017-10-11 16:21:55 +0000, Ryan C. Gordon wrote:

(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.

On 2017-10-11 16:44:43 +0000, Ryan C. Gordon wrote:

(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. :)

On 2017-10-11 17:25:59 +0000, Ozkan Sezer wrote:

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

On 2017-10-11 18:12:52 +0000, Eric Wasylishen wrote:

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.

On 2017-10-12 15:50:19 +0000, Sam Lantinga wrote:

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

Thanks!

On 2017-10-12 19:28:31 +0000, Eric Wasylishen wrote:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant