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

freedroid: Segfault after intro, etc #244

Closed smcv closed 1 year ago

smcv commented 1 year ago

Prerequisites:

To reproduce:

Each time, click once to pass the Briefing screen, then a second time to pass the description of your robot.

Expected result: all work

Actual result: with sdl12-compat, it crashes.

Video info summary from SDL:
----------------------------------------------------------------------
Is it possible to create hardware surfaces: no
Is there a window manager available: yes
Are hardware to hardware blits accelerated: no
Are hardware to hardware colorkey blits accelerated: no
Are hardware to hardware alpha blits accelerated: no
Are software to hardware blits accelerated: no
Are software to hardware colorkey blits accelerated: no
Are software to hardware alpha blits accelerated: no
Are color fills accelerated: no
Total amount of video memory in Kilobytes: 262144
Pixel format of the video device: bpp = 32, bytes/pixel = 4
Video Driver Name: wayland
----------------------------------------------------------------------
Found new graphics-theme: classic 
Found new graphics-theme: lanzz 
Found new graphics-theme: para90 
Game starts using theme: classic
Segmentation fault (core dumped)
#0  SDL_ListRemove (head=0x55b900e608a8, ent=ent@entry=0x55b9a5f4dce0) at ./src/SDL_list.c:70
#1  0x00007f49e83801eb in SDL_InvalidateMap (map=0x55b9a5f4dce0) at ./src/video/SDL_pixels.c:1052
#2  SDL_MapSurface (src=src@entry=0x55b9a5f4d7c0, dst=dst@entry=0x55b9a5e60870) at ./src/video/SDL_pixels.c:1075
#3  0x00007f49e83847ca in SDL_LowerBlit_REAL (src=0x55b9a5f4d7c0, srcrect=0x7ffcaf3e5d80, dst=0x55b9a5e60870, dstrect=0x7ffcaf3e5e00)
    at ./src/video/SDL_surface.c:701
#4  0x00007f49e8384a59 in SDL_UpperBlit_REAL (src=0x55b9a5f4d7c0, srcrect=<optimized out>, dst=0x55b9a5e60870, dstrect=0x7ffcaf3e5e00)
    at ./src/video/SDL_surface.c:807
#5  0x00007f49e8aef62c in SDL_UpperBlit
    (src12=0x55b9a5f4dd70, srcrect12=<optimized out>, dst12=0x55b9a5e60970, dstrect12=0x7ffcaf3e5e48)
    at /home/desktop/tmp/sdl12-compat/src/SDL12_compat.c:4772
#6  0x000055b9a3b26e07 in PutInfluence (x=x@entry=-1, y=y@entry=-1) at view.c:290
#7  0x000055b9a3b27ee4 in Assemble_Combat_Picture (mask=mask@entry=2) at view.c:228
#8  0x000055b9a3b0c9ea in main (argc=<optimized out>, argv=<optimized out>) at main.c:140
icculus commented 1 year ago

SaveDestAlpha strikes again. This is from valgrind:

==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x5428BDC: SDL_malloc_REAL (SDL_malloc.c:5405)
==3004919==    by 0x539D1C2: SDL_malloc (SDL_dynapi_procs.h:408)
==3004919==    by 0x490B9E6: SaveDestAlpha (SDL12_compat.c:6198)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB83: SaveDestAlpha (SDL12_compat.c:6217)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BE46: RestoreDestAlpha (SDL12_compat.c:6266)
==3004919==    by 0x490BFB0: SDL_UpperBlit (SDL12_compat.c:6297)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x4848842: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3004919==    by 0x542896E: real_malloc (SDL_malloc.c:5310)
==3004919==    by 0x5428BF5: SDL_malloc_REAL (SDL_malloc.c:5409)
==3004919==    by 0x539D1C2: SDL_malloc (SDL_dynapi_procs.h:408)
==3004919==    by 0x490B9E6: SaveDestAlpha (SDL12_compat.c:6198)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Warning: set address range perms: large range [0x59c87040, 0xc4b3d4bc) (undefined)
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB83: SaveDestAlpha (SDL12_compat.c:6217)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB6F: SaveDestAlpha (SDL12_compat.c:6218)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Invalid read of size 4
==3004919==    at 0x490BB45: SaveDestAlpha (SDL12_compat.c:6219)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919==  Address 0xa7a63f8 is 0 bytes after a block of size 16,424 alloc'd
==3004919==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3004919==    by 0x542896E: real_malloc (SDL_malloc.c:5310)
==3004919==    by 0x5428BF5: SDL_malloc_REAL (SDL_malloc.c:5409)
==3004919==    by 0x5390068: SDL_SIMDAlloc_REAL (SDL_cpuinfo.c:1125)
==3004919==    by 0x549B833: SDL_CreateRGBSurfaceWithFormat_REAL (SDL_surface.c:152)
==3004919==    by 0x549B97A: SDL_CreateRGBSurface_REAL (SDL_surface.c:198)
==3004919==    by 0x549DC81: SDL_ConvertSurface_REAL (SDL_surface.c:1196)
==3004919==    by 0x539E1D7: SDL_ConvertSurface (SDL_dynapi_procs.h:495)
==3004919==    by 0x490C480: SDL_ConvertSurface (SDL12_compat.c:6404)
==3004919==    by 0x490C623: SDL_DisplayFormatAlpha (SDL12_compat.c:6450)
==3004919==    by 0x11634D: ??? (in /usr/games/freedroid)
==3004919==    by 0x11AD3F: ??? (in /usr/games/freedroid)
==3004919== 
==3004919== 
==3004919== More than 10000000 total errors detected.  I'm not reporting any more.
==3004919== Final error counts will be inaccurate.  Go fix your program!
==3004919== Rerun with --error-limit=no to disable this cutoff.  Note
==3004919== that errors may occur in your program without prior warning from
==3004919== Valgrind, because errors are no longer being displayed.
sezero commented 1 year ago

This shouldn't be fixed by 35488c6, it should be #248 instead.

icculus commented 1 year ago

Ah, yes, my bad.

icculus commented 1 year ago

ok, the valgrind issues are resolved on sdl12-compat's end (but note that freedroid doesn't initialize the destination rectangle correctly in any case, it doesn't initialize the stack-allocated variable before doing dst.x += whatever;), but we're still rendering incorrectly, so this bug is still open.

icculus commented 1 year ago

This is trying to blit from a surface that has its flags set to SDL_RLEACCELOK in SDL-1.2, where sdl12-compat has it set to zero; this is probably okay, it was an internal flag we probably don't use.

But it's also trying to blit to a surface that has its flags set to SDL_SRCALPHA in SDL-1.2...also zero in sdl12-compat.

It creates this destination surface with SDL_DisplayFormatAlpha(), and while the SDL_Surface::format details match, my guess is this flag should be set somewhere along the way, and will fix this issue. I need to dig a little further.

icculus commented 1 year ago

We appear to be checking for this flag in sdl12-compat's SDL_DisplayFormatAlpha, so the surface-to-be-converted doesn't have it for some reason.

icculus commented 1 year ago

Ok, we set the SDL_SRCALPHA flag the same way 1.2 does now, I think, and this partially fixes the droid rendering, as they now aren't backed by a black square but correctly blend with the background.

However: the droid's ID number doesn't render...this appears to be further drama with SaveDestAlpha in sdl12-compat; if I disable alpha saving/restoring, the droid rendering is 100% correct. Don't know what's up there, yet.

Also, shots from your weapon and burn marks where destroyed droids lay are still rendered with black backgrounds, regardless of any of these changes, so there's still some more to do here.

icculus commented 1 year ago

As far as the droid numbers go: the surfaces are flagged SDL_RLEACCEL when run with SDL-1.2, and not with sdl12-compat...but more notably, SDL 1.2 doesn't appear to preserve destination alpha for RLE-encoded surfaces, but only in the run-lengths. The blit lands in here:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L1194-L1259

And either memcpy's pixels for run-lengths:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L111-L118

Or blends them with this, which does appear to preserve dest alpha:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L955-L967

This is getting messy now.

icculus commented 1 year ago

Although, just turning off sdl12-compat's efforts to preserve destination alpha fixes all the rendering issues completely, so I'm just going to add a quirk and leave it at that for now.

icculus commented 1 year ago

(I must have had something else broken, it's only the droid numbers that need the fix I just pushed. Everything else is rendering correctly with or without this patch.)