xrick starts with incorrect colors and then freezes

Question

xrick starts with incorrect colors and then freezes

chewi opened this issue 2 years ago · 17 comments

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.

Answer 1 · 2023-01-09T21:22:51.000Z

This happens with 1.2.60 and the latest git.

Answer 2 · 2023-01-09T22:47:31.000Z

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

Answer 3 · 2023-01-09T22:54:04.000Z

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

Answer 4 · 2023-01-10T06:30:07.000Z

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)

Answer 5 · 2023-01-10T09:20:04.000Z

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.

Answer 6 · 2023-01-10T14:06:16.000Z

It sounds like xrick is doing something silly.

Not necessarily.

Answer 7 · 2023-01-11T01:03:49.000Z

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.

Answer 8 · 2023-01-11T01:13:43.000Z

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.

Answer 9 · 2023-01-11T01:46:06.000Z

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.

Answer 10 · 2023-01-11T16:23:51.000Z

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?

Answer 11 · 2023-01-11T17:34:03.000Z

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:

Lock the surface only if SDL_MUSTLOCK says so.
Do all its updates
Unlock only if it was locked.
Call SDL_Flip at the end (for this game, there's probably extremely little benefit to trying to track dirty rectangles and call SDL_UpdateRects, even on legit ancient hardware).

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!

Answer 12 · 2023-01-11T17:42:15.000Z

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

Answer 13 · 2023-01-11T20:28:14.000Z

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

Answer 14 · 2023-01-11T20:33:33.000Z

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?

Answer 15 · 2023-01-11T20:55:04.000Z

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.

Answer 16 · 2023-01-11T21:04:54.000Z

Yep, good point.

Answer 17 · 2023-01-11T22:00:10.000Z

Very nice, thank you!