sdl12-compat icon indicating copy to clipboard operation
sdl12-compat copied to clipboard

OpenXcom issues with warp mouse, mouse wheel and sfx sound

Open SupSuper opened this issue 3 years ago • 49 comments

Now that Linux distros are replacing SDL1.2 with sdl12-compat, we're getting new bug reports for features that worked fine in SDL1.2: https://openxcom.org/forum/index.php/topic,10776.0.html

The key regressions seem to be:

  • Scrolling to the sides is very erratic with the mouse. (this depends on SDL_WarpMouse)
  • Mousewheel either moves cursor around or doesn't work (do wheel events no longer send mouse coordinates?).
  • Sfx begin to deteriorate after a while (no idea about this, OpenXcom uses SDL_Mixer for audio).

SupSuper avatar Sep 21 '22 21:09 SupSuper

Knowing the SDL2 version in use in each case might help understand things.

sezero avatar Sep 21 '22 22:09 sezero

@sezero I've asked for more info. I assume they're using the latest Fedora packages, which are SDL2 2.24 and sdl12-compat 1.2.56

I looked at the source code and the mouse wheel bug seems to be a behavior difference, as SDL12-compat doesn't add the mouse coordinates to wheel events (which SDL1.2 did): https://github.com/libsdl-org/sdl12-compat/blob/main/src/SDL12_compat.c#L4587

SupSuper avatar Sep 22 '22 04:09 SupSuper

Hi. So I get these issues with all the builds I made with sdl12-compat. I don't get them with Wine or with the Appimage from the Download section. I get the same behavior on my desktop and laptop (both Fedora Linux 36)

Linux a485 5.19.9-200.fc36.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Sep 15 09:49:52 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

cmake .. -DCMAKE_BUILD_TYPE=Release -DDEV_BUILD=OFF -DBUILD_PACKAGE=OFF
-- The C compiler identification is GNU 12.2.1
-- The CXX compiler identification is GNU 12.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/lib64/ccache/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.8.0") 
found SDL 1.2.56 (/usr/lib64:/usr/include/SDL)
found SDL_mixer 1.2.12 (/usr/lib64:/usr/include/SDL)
found SDL_gfx 2.0.26 (/usr/lib64:/usr/include/SDL)
found SDL_image 1.2.12 (/usr/lib64:/usr/include/SDL)
found yaml-cpp(/usr/lib64:/usr/include;/usr/include/..)
-- Found OpenGL: /usr/lib64/libOpenGL.so   
found openGL (/usr/lib64/libGL.so;/usr/lib64/libGLU.so)
-- Found Git: /usr/bin/git (found version "2.37.3") 
git found: /usr/bin/git
Found doxygen:/usr/bin/doxygen
-- Configuring done
-- Generating done
-- Build files have been written to:
Installed Packages
Name         : yaml-cpp-devel
Version      : 0.6.3
Release      : 6.fc36
Architecture : x86_64
Size         : 112 k
Source       : yaml-cpp-0.6.3-6.fc36.src.rpm`

rebsol avatar Sep 23 '22 16:09 rebsol

sdl12-compat-devel.x86_64 1.2.56-1.fc36 @updates

rebsol avatar Sep 23 '22 16:09 rebsol

If you need any more info, just shoot.

Edit: Thanks for fixing the tags madebr, I try to remember using "```" in the future.

rebsol avatar Sep 23 '22 16:09 rebsol

$ dnf info sdl* --installed 
Installed Packages
Name         : SDL2
Version      : 2.24.0
Release      : 1.fc36
Architecture : i686
Size         : 1.9 M
Source       : SDL2-2.24.0-1.fc36.src.rpm
Repository   : @System
From repo    : updates
Summary      : Cross-platform multimedia library
URL          : http://www.libsdl.org/
License      : zlib and MIT
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library designed
             : to provide fast access to the graphics frame buffer and audio device.

Name         : SDL2
Version      : 2.24.0
Release      : 1.fc36
Architecture : x86_64
Size         : 1.8 M
Source       : SDL2-2.24.0-1.fc36.src.rpm
Repository   : @System
From repo    : updates
Summary      : Cross-platform multimedia library
URL          : http://www.libsdl.org/
License      : zlib and MIT
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library designed
             : to provide fast access to the graphics frame buffer and audio device.

Name         : SDL2_image
Version      : 2.0.5
Release      : 8.fc36
Architecture : x86_64
Size         : 165 k
Source       : SDL2_image-2.0.5-8.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Image loading library for SDL
URL          : http://www.libsdl.org/projects/SDL_image/
License      : LGPLv2+ and zlib
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library
             : designed to provide fast access to the graphics frame buffer and audio
             : device.  This package contains a simple library for loading images of
             : various formats (BMP, PPM, PCX, GIF, JPEG, PNG) as SDL surfaces.

Name         : SDL2_net
Version      : 2.0.1
Release      : 15.fc36
Architecture : x86_64
Size         : 27 k
Source       : SDL2_net-2.0.1-15.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : SDL portable network library
URL          : http://www.libsdl.org/projects/SDL_net/
License      : zlib
Description  : This is a portable network library for use with SDL.

Name         : SDL_gfx
Version      : 2.0.26
Release      : 6.fc36
Architecture : x86_64
Size         : 112 k
Source       : SDL_gfx-2.0.26-6.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : SDL graphics drawing primitives and other support functions
URL          : http://www.ferzkopp.net/Software/SDL_gfx-2.0/
License      : LGPLv2
Description  : Library providing SDL graphics drawing primitives and other support functions
             : wrapped up in an addon library for the Simple Direct Media (SDL) cross-platform
             : API layer.

Name         : SDL_gfx-devel
Version      : 2.0.26
Release      : 6.fc36
Architecture : x86_64
Size         : 85 k
Source       : SDL_gfx-2.0.26-6.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Development files for SDL_gfx
URL          : http://www.ferzkopp.net/Software/SDL_gfx-2.0/
License      : LGPLv2
Description  : This package contains the files required to develop programs which use SDL_gfx.

Name         : SDL_image
Version      : 1.2.12
Release      : 30.fc36
Architecture : x86_64
Size         : 86 k
Source       : SDL_image-1.2.12-30.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Image loading library for SDL
URL          : http://www.libsdl.org/projects/SDL_image/
License      : LGPLv2+
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library
             : designed to provide fast access to the graphics frame buffer and audio
             : device.  This package contains a simple library for loading images of
             : various formats (BMP, PPM, PCX, GIF, JPEG, PNG) as SDL surfaces.

Name         : SDL_image-devel
Version      : 1.2.12
Release      : 30.fc36
Architecture : x86_64
Size         : 5.7 k
Source       : SDL_image-1.2.12-30.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Development files for SDL_image
URL          : http://www.libsdl.org/projects/SDL_image/
License      : LGPLv2+
Description  : The SDL_image-devel package contains libraries and header files for
             : developing applications that use SDL_image.

Name         : SDL_mixer
Version      : 1.2.12
Release      : 24.fc36
Architecture : x86_64
Size         : 205 k
Source       : SDL_mixer-1.2.12-24.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Simple DirectMedia Layer - Sample Mixer Library
URL          : http://www.libsdl.org/projects/SDL_mixer/
License      : LGPLv2
Description  : A simple multi-channel audio mixer for SDL. It supports 4 channels of
             : 16 bit stereo audio, plus a single channel of music, mixed by the popular
             : MikMod MOD, Timidity MIDI and Ogg Vorbis libraries.

Name         : SDL_mixer-devel
Version      : 1.2.12
Release      : 24.fc36
Architecture : x86_64
Size         : 27 k
Source       : SDL_mixer-1.2.12-24.fc36.src.rpm
Repository   : @System
From repo    : fedora
Summary      : Development files for SDL_mixer
URL          : http://www.libsdl.org/projects/SDL_mixer/
License      : LGPLv2
Description  : The SDL_mixer-devel package contains libraries and header files for
             : developing applications that use SDL_mixer.

Name         : sdl12-compat
Version      : 1.2.56
Release      : 1.fc36
Architecture : x86_64
Size         : 186 k
Source       : sdl12-compat-1.2.56-1.fc36.src.rpm
Repository   : @System
From repo    : updates
Summary      : SDL 1.2 runtime compatibility library using SDL 2.0
URL          : https://github.com/libsdl-org/sdl12-compat
License      : zlib and (Public Domain or MIT-0) and MIT
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library
             : designed to provide fast access to the graphics frame buffer and audio device.
             : 
             : This code is a compatibility layer; it provides a binary-compatible API for
             : programs written against SDL 1.2, but it uses SDL 2.0 behind the scenes.
             : 
             : If you are writing new code, please target SDL 2.0 directly and do not use
             : this layer.

Name         : sdl12-compat-devel
Version      : 1.2.56
Release      : 1.fc36
Architecture : x86_64
Size         : 482 k
Source       : sdl12-compat-1.2.56-1.fc36.src.rpm
Repository   : @System
From repo    : updates
Summary      : Files to develop SDL 1.2 applications using SDL 2.0
URL          : https://github.com/libsdl-org/sdl12-compat
License      : zlib and (Public Domain or MIT-0) and MIT
Description  : Simple DirectMedia Layer (SDL) is a cross-platform multimedia library
             : designed to provide fast access to the graphics frame buffer and audio device.
             : 
             : This code is a compatibility layer; it provides a source-compatible API for
             : programs written against SDL 1.2, but it uses SDL 2.0 behind the scenes.
             : 
             : If you are writing new code, please target SDL 2.0 directly and do not use
             : this layer.

rebsol avatar Sep 23 '22 18:09 rebsol

Got a build of OpenXcom running here now, finally saw an app print this error:

INFO: WARNING: this app used SDL_KillThread(), an unforgivable curse.
This program should be fixed. No thread was actually harmed.

...and I'm wondering if maybe we shouldn't be so cavalier about it and actually add some platform-specific code to kill threads.

(And maybe remove the Harry Potter reference from the error message.)

The game runs despite this, I haven't looked further into the thread in question yet. Still looking at the other issues.

icculus avatar Sep 28 '22 17:09 icculus

The thread in question loads the game data, and terminates on its own normally, and OpenXcode always tries to kill it when it's ready to move on (during the StartState object's destructor), but in modern times this thread is almost always going to finish and terminate itself before it can get there, making the SDL_KillThread call precautionary unless the user quit the game before the Microprose logo popped up. Maybe this gets used again later?

In this case, it appears to be mostly harmless.

icculus avatar Sep 28 '22 17:09 icculus

Not seeing mouse warping issues, on X11 or Wayland, with SDL 1.2 or sdl12-compat, but maybe I'm misunderstanding the issue...moving the mouse to the edge of the window seems to scroll the map okay, and it doesn't look like the game is trying to grab the mouse input to the window.

@rebsol, can I get more info on how to reproduce that specific issue?

icculus avatar Sep 28 '22 18:09 icculus

I get the same Info about the thread. I didn't think it was really connected to the issue. There is no difference for me if I'm in X11 or Wayland. The easiest reproduction of the issue for me is:

Open game, (either use default settings or custom), open New Battle -ok-ok-ESC- in Battlescape use the mouse wheel one time. The screen then moves to the north east.

I see this on my laptop, on my desktop with different mouses as well as trackpoint and touchpad.

rebsol avatar Sep 29 '22 06:09 rebsol

When I'm back at home I can test it with other distros in VMs.

rebsol avatar Sep 29 '22 06:09 rebsol

Open game, (either use default settings or custom), open New Battle -ok-ok-ESC- in Battlescape use the mouse wheel one time. The screen then moves to the north east.

Oh, yeah, that problem was fixed yesterday. It was the "Scrolling to the sides is very erratic with the mouse. (this depends on SDL_WarpMouse)" comment...scrolling seems okay to me (with the mousewheel fix or not using the mousewheel), but I might not understand what the issue is.

icculus avatar Sep 29 '22 14:09 icculus

Ah, good to know that is fixed. The other problems I had I have to investigate further. I try to look for something reproducible (I switched to Wine for my game, so this might take a bit).

rebsol avatar Sep 29 '22 14:09 rebsol

I got the latest sdl12_compat source running on Windows (don't have a Linux env set up at the moment) and here's what I found out:

  • Commit 50bde9f51d5e1714fdd87d380a030b28ce6a2cf1 does seem to fix the mouse issues. Couldn't reproduce any issues with mouse warping or scrolling.
  • Commit cc23a07dc4c7a720d192285cc38d1bd90508f72d can cause a crash, as there can be MOUSEMOTION events before VideoSurface12 is initialized.
  • There seems to be some bad interaction between sdl12_compat and SDL_Mixer, or I did something wrong. The following lines keep triggering a heap corruption error:
	SDL_RWops *rw = SDL_RWFromConstMem(data, size);
	_sound = Mix_LoadWAV_RW(rw, 1);

SupSuper avatar Sep 29 '22 21:09 SupSuper

The thread in question loads the game data, and terminates on its own normally, and OpenXcode always tries to kill it when it's ready to move on (during the StartState object's destructor), but in modern times this thread is almost always going to finish and terminate itself before it can get there, making the SDL_KillThread call precautionary unless the user quit the game before the Microprose logo popped up. Maybe this gets used again later?

In this case, it appears to be mostly harmless.

@icculus Your analysis is correct, the thread is only used at the start and the force kill is just in case the app is closed early (the loading can be significantly longer with debug / modded builds). So SDL_KillThread being a no-op should be harmless.

SupSuper avatar Sep 29 '22 21:09 SupSuper

* There seems to be some bad interaction between sdl12_compat
  and SDL_Mixer, or I did something wrong. The following lines keep
  triggering a heap corruption error:
	SDL_RWops *rw = SDL_RWFromConstMem(data, size);
	_sound = Mix_LoadWAV_RW(rw, 1);

That can be a bug in SDL_mixer itself? Can you try building SDL_mixer from git SDL-1.2 branch (where multiple bugs are fixed since SDL_mixer-1.2.12) and test with it?

EDIT: Since you said you are running on windows, here are windows x86 and x64 builds of SDL_mixer-1.2 from git: ~SDL_mixer-1.2-git-win32.zip~ ~SDL_mixer-1.2-git-win64.zip~

sezero avatar Sep 29 '22 21:09 sezero

Commit https://github.com/libsdl-org/sdl12-compat/commit/cc23a07dc4c7a720d192285cc38d1bd90508f72d can cause a crash, as there can be MOUSEMOTION events before VideoSurface12 is initialized.

So: something like the following? (CC: @sulix)

diff --git a/src/SDL12_compat.c b/src/SDL12_compat.c
index 3c9272f..84bc020 100644
--- a/src/SDL12_compat.c
+++ b/src/SDL12_compat.c
@@ -4538,8 +4538,10 @@ EventFilter20to12(void *data, SDL_Event *event20)
             event12.motion.state = event20->motion.state;
             AdjustOpenGLLogicalScalingPoint(&event20->motion.x, &event20->motion.y);
             /* Clamp the absolute position to the window dimensions. */
-            event20->motion.x = SDL_max(SDL_min(event20->motion.x, VideoSurface12->w), 0);
-            event20->motion.y = SDL_max(SDL_min(event20->motion.y, VideoSurface12->h), 0);
+            if (VideoSurface12) {
+                event20->motion.x = SDL_max(SDL_min(event20->motion.x, VideoSurface12->w), 0);
+                event20->motion.y = SDL_max(SDL_min(event20->motion.y, VideoSurface12->h), 0);
+            }
             event12.motion.x = (Uint16) event20->motion.x;
             event12.motion.y = (Uint16) event20->motion.y;
             if (UseMouseRelativeScaling) {

sezero avatar Sep 29 '22 21:09 sezero

So: something like the following?

Should we just drop the event if there's no VideoSurface12 yet?

icculus avatar Sep 30 '22 03:09 icculus

Should we just drop the event if there's no VideoSurface12 yet?

Well, why not.

sezero avatar Sep 30 '22 03:09 sezero

the thread is only used at the start and the force kill is just in case the app is closed early (the loading can be significantly longer with debug / modded builds)

I suspect this is unnecessary on mainstream OSs, as long as when "the app is closed" (what does this mean specifically, the main window being Alt+F4'd?), the result is either exit() or a return from main().

On POSIX systems (in particular Linux and macOS), if the main thread returns from main() or any thread calls exit(), POSIX says all other threads in the process get terminated automatically.

I don't develop on Windows, but MSDN documents ExitProcess() as terminating all threads (similar to exit()), and documents a return from main() as causing a call to ExitProcess(), so the same is probably true there too.

smcv avatar Sep 30 '22 10:09 smcv

Just as an FYI: we're at Release Candidate status for 1.2.60, so if there's something we need to change in sdl12-compat for SDL_mixer support, we should try to do it in the next few days.

(or if it's just "I built it with (Visual Studio 2019/MingW/something else) and it crashes trying to load a .wav" then let me know and I'll try to reproduce it over here.)

It's also worth noting that, if this is Windows-specific issue, we're a little safer in that no one will change the SDL 1.2 build out from under you, unlike a Linux distribution.

icculus avatar Oct 03 '22 15:10 icculus

@sezero

EDIT: Since you said you are running on windows, here are windows x86 and x64 builds of SDL_mixer-1.2 from git: ~SDL_mixer-1.2-git-win32.zip~ ~SDL_mixer-1.2-git-win64.zip~

Tried them, same issue.

@icculus

(or if it's just "I built it with (Visual Studio 2019/MingW/something else) and it crashes trying to load a .wav" then let me know and I'll try to reproduce it over here.)

I made a minimal program in Visual Studio just loading a WAV in SDL_Mixer and I keep getting the heap corruption assert (it doesn't crash though, curiously). However it comes from inside SDL_Mixer so I don't have debug symbols for it. Here's the whole project, or just the code:

#include <SDL/SDL.h>
#include <SDL/SDL_mixer.h>

int main(int argc, char* argv[])
{
    SDL_Init(SDL_INIT_EVERYTHING);
    Mix_OpenAudio(MIX_DEFAULT_FREQUENCY, MIX_DEFAULT_FORMAT, MIX_DEFAULT_CHANNELS, 2048);
    SDL_Surface *video = SDL_SetVideoMode(640, 480, 24, 0);

    Mix_Chunk *chunk = Mix_LoadWAV("amazing.wav");
    Mix_PlayChannel(-1, chunk, 0);

    SDL_Event ev;
    bool quit = false;
    while (!quit) {
        while (SDL_PollEvent(&ev)) {
            if (ev.type == SDL_QUIT) {
                quit = true;
            }
        }
    }
    SDL_Quit();
    return 0;
}

SupSuper avatar Oct 06 '22 11:10 SupSuper

Built SDL_mixer with debug symbols, here's where it's failing: image

SupSuper avatar Oct 06 '22 22:10 SupSuper

OK, SDL_mixer-1.2 is possibly using free() for SDL_free here. Which compiler are SDL2 (and your SDL_mixer-1.2) built with? Specifically: is SDL2 built with Visual Studio?

Problem is, the audio file loaded is most possibly a wav file, and SDL_mixer-1.2 calls SDL_LoadWAV_RW, sdl12-compat forwards that to SDL2 version of SDL_LoadWAV_RW which allocates the buffer with SDL_malloc. If SDL2 is built using MSVC, then it does not use the libc version of malloc for SDL_malloc but its embedded one, same for SDL_free: the result is libc free() called with SDL_malloc'ed memory rightfully crashing...

If my assumptions are correct, what is the solution here?

sezero avatar Oct 06 '22 22:10 sezero

If my assumptions are correct, what is the solution here?

One thing I can think of: rebuilding SDL_mixer-1.2 specifically against sdl12-compat: In sdl12-compat, SDL_free is a function call and not a macro expanding to libc free.

sezero avatar Oct 06 '22 23:10 sezero

If my assumptions are correct, what is the solution here?

OR: a better solution would be calling SDL_FreeWAV for loaded wav files in SDL_mixer-1.2: Does the following patch fix your issue? (It is crude but it hopefully should do the job.)

[this one is incomplete - better patch in the next reply]
diff --git a/mixer.c b/mixer.c
index 431f6e6..6ad6409 100644
--- a/mixer.c
+++ b/mixer.c
@@ -561,4 +561,5 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 	SDL_AudioCVT wavecvt;
 	int samplesize;
+	int wavfree=0;
 
 	/* rcg06012001 Make sure src is valid */
@@ -595,4 +596,5 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 		case WAVE:
 		case RIFF:
+			wavfree = 1;
 			loaded = SDL_LoadWAV_RW(src, freesrc, &wavespec,
 					(Uint8 **)&chunk->abuf, &chunk->alen);
@@ -644,5 +646,7 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 				wavespec.format, wavespec.channels, wavespec.freq,
 				mixer.format, mixer.channels, mixer.freq) < 0 ) {
-			SDL_free(chunk->abuf);
+			if (wavfree)
+			  SDL_FreeWAV(chunk->abuf);
+			else SDL_free(chunk->abuf);
 			SDL_free(chunk);
 			return(NULL);
@@ -653,10 +657,14 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 		if ( wavecvt.buf == NULL ) {
 			SDL_SetError("Out of memory");
-			SDL_free(chunk->abuf);
+			if (wavfree)
+			  SDL_FreeWAV(chunk->abuf);
+			else SDL_free(chunk->abuf);
 			SDL_free(chunk);
 			return(NULL);
 		}
 		memcpy(wavecvt.buf, chunk->abuf, wavecvt.len);
-		SDL_free(chunk->abuf);
+		if (wavfree)
+		  SDL_FreeWAV(chunk->abuf);
+		else SDL_free(chunk->abuf);
 
 		/* Run the audio converter */

sezero avatar Oct 06 '22 23:10 sezero

Better, more complete patch for SDL_mixer-1.2:

diff --git a/mixer.c b/mixer.c
index 431f6e6..bae3271 100644
--- a/mixer.c
+++ b/mixer.c
@@ -560,6 +560,7 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 	SDL_AudioSpec wavespec, *loaded;
 	SDL_AudioCVT wavecvt;
 	int samplesize;
+	int wavfree;			/* to decide how to free chunk->abuf */
 
 	/* rcg06012001 Make sure src is valid */
 	if ( ! src ) {
@@ -591,9 +592,11 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 	/* Seek backwards for compatibility with older loaders */
 	SDL_RWseek(src, -(int)sizeof(Uint32), RW_SEEK_CUR);
 
+	wavfree = 0;
 	switch (magic) {
 		case WAVE:
 		case RIFF:
+			wavfree = 1;
 			loaded = SDL_LoadWAV_RW(src, freesrc, &wavespec,
 					(Uint8 **)&chunk->abuf, &chunk->alen);
 			break;
@@ -643,7 +646,8 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 		if ( SDL_BuildAudioCVT(&wavecvt,
 				wavespec.format, wavespec.channels, wavespec.freq,
 				mixer.format, mixer.channels, mixer.freq) < 0 ) {
-			SDL_free(chunk->abuf);
+			if (!wavfree)SDL_free(chunk->abuf);
+			else	SDL_FreeWAV(chunk->abuf);
 			SDL_free(chunk);
 			return(NULL);
 		}
@@ -652,12 +656,14 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 		wavecvt.buf = (Uint8 *)SDL_calloc(1, wavecvt.len*wavecvt.len_mult);
 		if ( wavecvt.buf == NULL ) {
 			SDL_SetError("Out of memory");
-			SDL_free(chunk->abuf);
+			if (!wavfree) SDL_free(chunk->abuf);
+			else	SDL_FreeWAV(chunk->abuf);
 			SDL_free(chunk);
 			return(NULL);
 		}
 		memcpy(wavecvt.buf, chunk->abuf, wavecvt.len);
-		SDL_free(chunk->abuf);
+		if (!wavfree) SDL_free(chunk->abuf);
+		else	SDL_FreeWAV(chunk->abuf);
 
 		/* Run the audio converter */
 		if ( SDL_ConvertAudio(&wavecvt) < 0 ) {
@@ -668,9 +674,10 @@ Mix_Chunk *Mix_LoadWAV_RW(SDL_RWops *src, int freesrc)
 
 		chunk->abuf = wavecvt.buf;
 		chunk->alen = wavecvt.len_cvt;
+		wavfree = 0;
 	}
 
-	chunk->allocated = 1;
+	chunk->allocated = (wavfree == 0) ? 1 : 2; /* see Mix_FreeChunk() */
 	chunk->volume = MIX_MAX_VOLUME;
 
 	return(chunk);
@@ -757,8 +764,13 @@ void Mix_FreeChunk(Mix_Chunk *chunk)
 		}
 		SDL_UnlockAudio();
 		/* Actually free the chunk */
-		if ( chunk->allocated ) {
+		switch ( chunk->allocated ) {
+		case 1:
 			SDL_free(chunk->abuf);
+			break;
+		case 2:
+			SDL_FreeWAV(chunk->abuf);
+			break;
 		}
 		SDL_free(chunk);
 	}

sezero avatar Oct 07 '22 00:10 sezero

@icculus, @slouken: Please review the SDL_mixer-1.2 patch above until @SupSuper has a chance to test it.

sezero avatar Oct 07 '22 02:10 sezero

@sezero I've rebuilt everything from source, SDL12-compat, SDL12-mixer, SDL2, all linked together and using the same compiler (Visual Studio 2019 on Windows). So it doesn't seem like a compiler issue.

However, your patch that replaces SDL_free with SDL_FreeWAV fixes the problem, so I guess there's still some subtle difference between the two.

SupSuper avatar Oct 07 '22 09:10 SupSuper

However, your patch that replaces SDL_free with SDL_FreeWAV fixes the problem, so I guess there's still some subtle difference between the two.

That's great, I'll apply the patch to SDL_mixer SDL-1.2 branch shortly. To re-confirm: The audio file you load with Mix_LoadWAV_RW is a wav file yes?

@icculus, @slouken: any better solution for SDL_mixer-1.2? It looks like it has been actually technically a bug in there but it used to work only by chance.

sezero avatar Oct 07 '22 10:10 sezero