Drawing regression in latest SDL3

[andreasgrabher]

Drawing regression in latest SDL3

[icculus]

Are these 8-bit surfaces?

[andreasgrabher]

Format of the surface is SDL_PIXELFORMAT_ARGB8888.

[AntTheAlchemist]

Same thing happened to me, and I thought it was because I've been using the wrong format all this time. I changed it to SDL_PIXELFORMAT_RGBA32 and it works now.

[andreasgrabher]

Format is obtained from

SDL_Texture* uiTexture = SDL_CreateTexture(sdlRenderer, SDL_PIXELFORMAT_UNKNOWN, SDL_TEXTUREACCESS_STREAMING, width, height);
format = SDL_GetNumberProperty(SDL_GetTextureProperties(uiTexture), SDL_PROP_TEXTURE_FORMAT_NUMBER, NULL);
SDL_Surface* sdlscrn = SDL_CreateSurface(width, height, format);

Forcing format to SDL_PIXELFORMAT_RGBA32 does not fix the problem and causes wrong colours.

[slouken]

@0x1F9F1, is this related to the blitter changes?

[andreasgrabher]

After some testing it seems this commit caused the regression: 875c4f0

[slouken]

Can you see where indexed format surfaces are being used in your code?

[andreasgrabher]

It might be this:

/*-----------------------------------------------------------------------*/
/**
 * Load an 1 plane XBM into a 8 planes SDL_Surface.
 */
static SDL_Surface *SDLGui_LoadXBM(int w, int h, const Uint8 *pXbmBits)
{
	SDL_Surface *bitmap;
	Uint8 *dstbits;
	const Uint8 *srcbits;
	int x, y, srcpitch;
	int mask;

	srcbits = pXbmBits;

	/* Allocate the bitmap */
	bitmap = SDL_CreateSurface(w, h, SDL_GetPixelFormatForMasks(8, 0, 0, 0, 0));
	if (bitmap == NULL)
	{
		Log_Printf(LOG_ERROR, "SDLGui: failed to allocate bitmap: %s", SDL_GetError());
		return NULL;
	}

	srcpitch = ((w + 7) / 8);
	dstbits = (Uint8 *)bitmap->pixels;
	mask = 1;

	/* Copy the pixels */
	for (y = 0 ; y < h ; y++)
	{
		for (x = 0 ; x < w ; x++)
		{
			dstbits[x] = (srcbits[x / 8] & mask) ? 1 : 0;
			mask <<= 1;
			mask |= (mask >> 8);
			mask &= 0xFF;
		}
		dstbits += bitmap->pitch;
		srcbits += srcpitch;
	}

	return bitmap;
}

[slouken]

That's very likely. Are you assuming a palette on there?

The other possibility, since I noticed the issue is with text, is that you're using SDL_ttf and haven't updated to the latest code.

[icculus]

Okay, so a change we just made is that 8-bit surfaces do not get a palette by default now; the idea is that trying to blit from an indexed surface without a palette will just memcpy() the indices instead of try to map the colors, which can be very useful when you instead to have a destination surface where you're adjusting the palette and just want to fill in the new data from another surface without mapping colors during the blit.

(This is apparently how DirectDraw does it, too.)

The quickest fix: apply a palette to the new thing!

SDL_Palette *palette = SDL_CreatePalette(2);
if (palette) {
    /* Create a black and white bitmap palette */
    palette->colors[0].r = 0xFF;
    palette->colors[0].g = 0xFF;
    palette->colors[0].b = 0xFF;
    palette->colors[1].r = 0x00;
    palette->colors[1].g = 0x00;
    palette->colors[1].b = 0x00;
    SDL_SetSurfacePalette(surface, palette);
    SDL_DestroyPalette(palette);
}

@slouken will have to say if I screwed up that code completely, it's untested.

(Also, @slouken, README-migration really needs a code example for this.)

But my suspicion is that SDL_ttf just needs to be updated and it'll solve this problem.

[slouken]

This is the biggest change likely to bite people unexpectedly in SDL3. I'm really tempted to roll it back.

[icculus]

I'm okay with a rollback, but the one-time fix for things bit by this is dirt-simple and extremely localized.

Maybe a separate function that explicitly creates a surface without a palette would be better though, so you don't get a surprising result from the usual path, and only use it when you definitely want it? I think the new behavior is good, but it could be that the default isn't.

[slouken]

That's true, maybe documentation is enough. I'll add a note to README-migration.md and see how that goes.

[slouken]

FWIW, having the new behavior is as simple as SDL_SetSurfacePalette(surface, NULL); after you create it.

[slouken]

But, having a palette on a surface isn't very useful until you set the colors, so you might as well set the palette explicitly. If people are doing that, they're much more likely to either not need to set one, or set a palette already shared with other surfaces, so creating one initially is just wasteful.

[slouken]

Maybe it would be helpful to add SDL_CreatePaletteForSurface(surface), which automatically creates it with the right number of entries for the bit depth?

[andreasgrabher]

@icculus: Thank you for the code. This fixes my issue. But I think it would be great if there was a simpler solution.

	/* Create a black and white bitmap palette */
	palette = SDL_CreatePalette(2);
	if (palette == NULL)
	{
		Log_Printf(LOG_ERROR, "SDLGui: failed to allocate palette: %s", SDL_GetError());
		return NULL;
	}
	palette->colors[0].r = palette->colors[0].g = palette->colors[0].b = 255;
	palette->colors[1].r = palette->colors[1].g = palette->colors[1].b = 0;
	SDL_SetSurfacePalette(bitmap, palette);
	SDL_DestroyPalette(palette);

In case it still matters: SDL_ttf is not used.

[andreasgrabher]

Update: Seems this one works too:

	/* Create an all white bitmap palette */
	palette = SDL_CreatePalette(1<<8);
	if (palette == NULL)
	{
		Log_Printf(LOG_ERROR, "SDLGui: failed to allocate palette: %s", SDL_GetError());
		return NULL;
	}
	SDL_SetSurfacePalette(bitmap, palette);
	SDL_DestroyPalette(palette);

[slouken]

I've made it even simpler, adding SDL_CreateSurfacePalette():

	/* Create an all white bitmap palette */
	if (!SDL_CreateSurfacePalette(bitmap))
	{
		Log_Printf(LOG_ERROR, "SDLGui: failed to allocate palette: %s", SDL_GetError());
		SDL_DestroySurface(bitmap);
		return NULL;
	}

[slouken]

In any case, with the new utility function and documentation in README-migration.md, I think we're in a good place now.

Thanks!

[andreasgrabher]

Thank you very much for the support! Just a little suggestion: Maybe it would be even simpler to have something like SDL_CreateSurfaceWithPalette instead.

[slouken]

You'd have to have two calls anyway, one to create a surface with a palette, and one to get the palette. This way the order of operations is simple and obvious. You create the surface, you create the palette, you take that palette and set colors, and you move on.