search

libsdl-org / sdl12-compat

An SDL-1.2 compatibility layer that uses SDL 2.0 behind the scenes.
Other
193 stars 40 forks source link

xrick starts with incorrect colors and then freezes #283

Closed chewi closed 1 year ago

chewi commented 1 year ago

I've finally added sdl12-compat to Gentoo, where it will eventually replace SDL1 entirely. I was saddened to find that xrick doesn't work though. It briefly shows some graphical corruption, then the title screen with incorrect colors, and freezes there. The freeze doesn't appear to be sound-related as it happens with the -nosound option.

chewi commented 1 year ago

This happens with 1.2.60 and the latest git.

hartwork commented 1 year ago

Note that this was observed with xrick 021212 from http://www.bigorno.net/xrick/download.html download http://www.bigorno.net/xrick/xrick-021212.tgz. (I'm on Gentoo too.)

hartwork commented 1 year ago

Here's an easy way to compile xrick:

cd "$(mktemp -d)"
wget http://www.bigorno.net/xrick/xrick-021212.tgz
tar xf xrick-021212.tgz 
cd xrick-021212/
wget -O- https://gitweb.gentoo.org/repo/gentoo.git/plain/games-arcade/xrick/files/xrick-021212-zlib.patch | patch -p1
wget -O- https://gitweb.gentoo.org/repo/gentoo.git/plain/games-arcade/xrick/files/xrick-021212-fno-common.patch | patch -p1
make
./xrick
sezero commented 1 year ago

valgrind outputs this:

==12612== Use of uninitialised value of size 4
==12612==    at 0x450288C: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x450289A: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x45028A8: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x450283F: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x450284D: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x450285B: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x4502869: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
==12612== 
==12612== Use of uninitialised value of size 4
==12612==    at 0x450287A: Blit1to4 (SDL_blit_1.c:263)
==12612==    by 0x450086A: SDL_SoftBlit (SDL_blit.c:88)
==12612==    by 0x45437B2: SDL_LowerBlit_REAL (SDL_surface.c:636)
==12612==    by 0x4543D34: SDL_UpperBlit_REAL (SDL_surface.c:733)
==12612==    by 0x44828B2: SDL_UpperBlit (SDL_dynapi_procs.h:497)
==12612==    by 0x404825A: SDL_UpdateRects (SDL12_compat.c:6848)
==12612==    by 0x4048544: SDL_UpdateRect (SDL12_compat.c:6899)
==12612==    by 0x404918D: SDL_SetPalette (SDL12_compat.c:7265)
==12612==    by 0x404925C: SDL_SetColors (SDL12_compat.c:7275)
==12612==    by 0x8050CF6: sysvid_setPalette (sysvid.c:100)
==12612==    by 0x8050579: draw_img (draw.c:677)
==12612==    by 0x804A8B1: screen_xrick (scr_xrick.c:37)
chewi commented 1 year ago

It sounds like xrick is doing something silly. If it makes more sense to fix that than make sdl12-compat work around it, I'd be fine with that.

sezero commented 1 year ago

It sounds like xrick is doing something silly.

Not necessarily.

icculus commented 1 year ago

I'm looking at it; those valgrind errors don't happen with classic SDL 1.2, so it very well might be an sdl12-compat bug.

icculus commented 1 year ago

The valgrind errors are because they set a palette before they draw anything, and that causes sdl12-compat to touch the screen surface's pixels. That part is an easy fix, we'll just calloc the pixels instead of malloc them.

It doesn't actually solve any of the problems, but the valgrind issues are gone. I'll dig further.

icculus commented 1 year ago

Okay, the problem is that xrick is locking the screen surface, and then calling SDL_UpdateRects on it while still locked, which is arguably a game bug but SDL 1.2 allows it. SDL2, however, sees the lock and refuses to use it for a blit, so we end up just returning out without drawing anything.

If I comment out the locks, it works fine.

There's never a time in sdl12-compat where you need to lock the screen surface, afaik, so I think we'll just check for that and pretend to lock it.

I still don't get audio in sdl12-compat, though, so I still need to check that, too.

hartwork commented 1 year ago

Okay, the problem is that xrick is locking the screen surface, and then calling SDL_UpdateRects on it while still locked, which is arguably a game bug but SDL 1.2 allows it. SDL2, however, sees the lock and refuses to use it for a blit, so we end up just returning out without drawing anything.

@icculus good to know, I can confirm that this quick fix indeed unblocks xrick start-up (but not the sound issue):

diff --git a/src/sysvid.c b/src/sysvid.c
index c8f847c..b00820b 100644
--- a/src/sysvid.c
+++ b/src/sysvid.c
@@ -315,12 +315,13 @@ sysvid_update(rect_t *rects)
     area.y = rects->y * zoom;
     area.h = rects->height * zoom;
     area.w = rects->width * zoom;
-    SDL_UpdateRects(screen, 1, &area);

     rects = rects->next;
   }

   SDL_UnlockSurface(screen);
+
+  SDL_Flip(screen);
 }

The alternative would probably creating a linked list of rectangles and then calling SDL_UnlockSurface on each of them after the call to SDL_UnlockSurface.

There's never a time in sdl12-compat where you need to lock the screen surface, afaik, so I think we'll just check for that and pretend to lock it.

Could you elaborate on why there is no need for locking? The way I read https://wiki.libsdl.org/SDL2/SDL_LockSurface and the existence of SDL_MUSTLOCK in SDL makes me think, there may be something to learn for me here.

I still don't get audio in sdl12-compat, though, so I still need to check that, too.

I found that SDL_LoadWAV_RW is failing with error Could not seek in file because the custom seek function data_file_seek returns -1 (for when path.zip is non-NULL with files contained within a zip container). My current guess is that SDL 1 is okay with unseekable files but SDL 2 isn't any more?

icculus commented 1 year ago

There's never a time in sdl12-compat where you need to lock the screen surface, afaik, so I think we'll just check for that and pretend to lock it.

Could you elaborate on why there is no need for locking? The way I read https://wiki.libsdl.org/SDL2/SDL_LockSurface and the existence of SDL_MUSTLOCK in SDL makes me think, there may be something to learn for me here.

Well, for one, xrick is calling SDL_LockSurface on a surface that SDL_MUSTLOCK reports false for. :)

The idea is that a surface might have been in video memory in some situations, or RLE-encoded in others, and to get access to raw pixels where the CPU can touch them in those cases, you had to lock the surface. Unlocking would re-encode and/or send them back to video RAM.

But in sdl12-compat, the screen surface is always in software, and we move its contents to a texture on the GPU as necessary.

In a perfect world, xrick would:

To be clear, the fix I made yesterday makes all of this unnecessary, so xrick doesn't need to be patched.

I found that SDL_LoadWAV_RW is failing with error Could not seek in file because the custom seek function data_file_seek returns -1 (for when path.zip is non-NULL with files contained within a zip container). My current guess is that SDL 1 is okay with unseekable files but SDL 2 isn't any more?

Nice work, I'll take it from here!

hartwork commented 1 year ago

@icculus thanks for the elaborations. I hackishly patched seeking support into xrick locally now to be sure, and that indeed made the audio work. So I re-confirm that it's SDL_RWops.seek.

icculus commented 1 year ago

Okay, I've pushed an sdl12-compat fix to deal with this, and now unmodifed xrick sources are working great here!

slouken commented 1 year ago

Okay, I've pushed an sdl12-compat fix to deal with this, and now unmodifed xrick sources are working great here!

Maybe we should do this inside of SDL3 LoadWAV?

icculus commented 1 year ago

Maybe we should do this inside of SDL3 LoadWAV?

My concern is that this explodes when someone hands it a multi-gigabyte file, which is less of a concern in sdl12-compat than in new code.

I did take a shot at having the SDL3 code fall back to reading and throwing away chunks to seek forward, and I might return to that.

slouken commented 1 year ago

Yep, good point.

hartwork commented 1 year ago

Very nice, thank you!