rockdodger: Immediately exits

[smcv]

Prerequisites:

• Debian testing (Debian 12 alpha)
• Video: GNOME 43 in Wayland mode (with Mesa 22.2.0 on AMD Vega, if it matters)
• Audio: Pipewire 0.3.59, with pipewire-pulse emulating PulseAudio
• apt install rockdodger (Debian package version 1.1.4-2)
• Some relevant libraries:
  • libsdl1.2-compat either 1.2.58-1 (packaged in Debian experimental) or commit 63e4393 (locally built)
  • libsdl2-2.0-0 version 2.24.1+dfsg-1
  • libsdl-image1.2 version 1.2.12-13+b1
  • libsdl-mixer1.2 version 1.2.12-17+b2
  • libsdl1.2debian (real SDL 1.2) version 1.2.15+dfsg2-8

To reproduce:

• ld.so /usr/games/rockdodger (to avoid setgid interfering)
• LD_LIBRARY_PATH=.../sdl12-compat/_build ld.so /usr/games/rockdodger
• SDL_VIDEODRIVER=wayland LD_LIBRARY_PATH=.../sdl12-compat/_build ld.so /usr/games/rockdodger

Expected result: it runs

Actual result: Real SDL 1.2 works. With sdl12-compat it loads some resources, creates a window and immediately exits.

[icculus]

During startup, it creates an SDL_Surface:

    /*
     * Just create a surface, no special needs, etc. Therefore we use
     * a simple 2-bit surface, give it a colorkey for transparency and
     * convert it to the display format.
     */
    if((surf = SDL_CreateRGBSurface(SDL_SWSURFACE, text_scroller_width, 36, 2, 0, 0, 0, 0)) != NULL) {

And this fails, because SDL2 refuses to make a 2-bit-per-pixel surface.

[icculus]

I put a hacky solution in there, but it gets it working.

[slouken]

Actually it's just 2bpp that's not supported.

Here's a potential patch to fix it?

diff --git a/include/SDL_pixels.h b/include/SDL_pixels.h
index 5d2c0c898..8a8489537 100644
--- a/include/SDL_pixels.h
+++ b/include/SDL_pixels.h
@@ -61,7 +61,8 @@ typedef enum
     SDL_PIXELTYPE_ARRAYU16,
     SDL_PIXELTYPE_ARRAYU32,
     SDL_PIXELTYPE_ARRAYF16,
-    SDL_PIXELTYPE_ARRAYF32
+    SDL_PIXELTYPE_ARRAYF32,
+    SDL_PIXELTYPE_INDEX2,
 } SDL_PixelType;

 /** Bitmap pixel order, high bit -> low bit. */
@@ -134,6 +135,7 @@ typedef enum
 #define SDL_ISPIXELFORMAT_INDEXED(format)   \
     (!SDL_ISPIXELFORMAT_FOURCC(format) && \
      ((SDL_PIXELTYPE(format) == SDL_PIXELTYPE_INDEX1) || \
+      (SDL_PIXELTYPE(format) == SDL_PIXELTYPE_INDEX2) || \
       (SDL_PIXELTYPE(format) == SDL_PIXELTYPE_INDEX4) || \
       (SDL_PIXELTYPE(format) == SDL_PIXELTYPE_INDEX8)))

@@ -177,6 +179,12 @@ typedef enum
     SDL_PIXELFORMAT_INDEX1MSB =
         SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX1, SDL_BITMAPORDER_1234, 0,
                                1, 0),
+    SDL_PIXELFORMAT_INDEX2LSB =
+        SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX2, SDL_BITMAPORDER_4321, 0,
+                               2, 0),
+    SDL_PIXELFORMAT_INDEX2MSB =
+        SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX2, SDL_BITMAPORDER_1234, 0,
+                               2, 0),
     SDL_PIXELFORMAT_INDEX4LSB =
         SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX4, SDL_BITMAPORDER_4321, 0,
                                4, 0),
diff --git a/src/video/SDL_pixels.c b/src/video/SDL_pixels.c
index 1646d844f..ccee3d8bc 100644
--- a/src/video/SDL_pixels.c
+++ b/src/video/SDL_pixels.c
@@ -90,6 +90,8 @@ SDL_GetPixelFormatName(Uint32 format)
 #define CASE(X) case X: return #X;
     CASE(SDL_PIXELFORMAT_INDEX1LSB)
     CASE(SDL_PIXELFORMAT_INDEX1MSB)
+    CASE(SDL_PIXELFORMAT_INDEX2LSB)
+    CASE(SDL_PIXELFORMAT_INDEX2MSB)
     CASE(SDL_PIXELFORMAT_INDEX4LSB)
     CASE(SDL_PIXELFORMAT_INDEX4MSB)
     CASE(SDL_PIXELFORMAT_INDEX8)
@@ -300,6 +302,9 @@ SDL_MasksToPixelFormatEnum(int bpp, Uint32 Rmask, Uint32 Gmask, Uint32 Bmask,
     case 1:
         /* SDL defaults to MSB ordering */
         return SDL_PIXELFORMAT_INDEX1MSB;
+    case 2:
+        /* SDL defaults to MSB ordering */
+        return SDL_PIXELFORMAT_INDEX2MSB;
     case 4:
         /* SDL defaults to MSB ordering */
         return SDL_PIXELFORMAT_INDEX4MSB;

Actually, since SDL was designed to understand new formats, this might just work:

const Uint32 SDL_PIXELFORMAT_INDEX2LSB = SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX1, SDL_BITMAPORDER_4321, 0, 2, 0);
const Uint32 SDL_PIXELFORMAT_INDEX2MSB = SDL_DEFINE_PIXELFORMAT(SDL_PIXELTYPE_INDEX1, SDL_BITMAPORDER_1234, 0, 2, 0);

SDL_CreateRGBSurfaceWithFormat(0, width, height, 2, SDL_PIXELFORMAT_INDEX2MSB);

[icculus]

It gets around the SDL_CreateRGBSurface issue, but then fails when the game tries to call SDL_DisplayFormat() on it.

Deep inside SDL_ConvertSurface we try to blit from the source format to the new one, which ends up failing in SDL_CalculateBlit0, here:

    if (surface->format->BitsPerPixel != 1) {
        /* We don't support sub 8-bit packed pixel modes */
        return (SDL_BlitFunc) NULL;
    }

(this happens even with the SDL2 patch, above.)

[slouken]

Hmm, I think we can probably add support for that back in. It's just two functions, it should be fine.

Edit: Oops, I misremembered the number of blit functions required for any given format. Never mind. :)

[slouken]

BTW, does your workaround of forcing 8 bits actually work? Do we get the right pixels on the screen?

[icculus]

We do, because it only creates it and immediately converts it to display format. In other cases this might fail. If all it wants to do is use SDL functions with it (blit, fill, etc), it won't.

[icculus]

Closing this again. If something else pops up that needs a more robust fix, we'll deal with it then.

[slouken]

Do we want to debug log something here? I feel like we're leaving a footgun for future us.

[icculus]

Do we want to debug log something here?

Your wish is my command. :)

[smcv]

Confirmed fixed with https://github.com/libsdl-org/sdl12-compat/commit/67f8b3a85b782eefb4db90f34d5b0742ef2cb5fc, 1.2.58 + 29 commits

[smcv]

The debug log message doesn't seem right though:

INFO: This app is creating an 806-bit SDL_Surface, but we are bumping it to 8-bits. ...

[icculus]

Your wish is my command. :)

The debug log message doesn't seem right though:

It's definitely on-brand for a genie to make things worse by granting your wish. :)

Thanks for catching that!

[sezero]

We are missing gcc printf format string attributes with our SDL20_syms.h macro machinery. That would require some work though, possibly not worth messing with.

[icculus]

Yeah, the surface area for bugs is pretty small (a few function calls), so I'd rather not add it if it adds any significant complexity, even though I totally just introduced a bug in one of those function calls. 😬