Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDL_ConvertSurface does not convert color keys consistently #1854

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

SDL_ConvertSurface does not convert color keys consistently #1854

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: 2.0.2
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2015-05-10 09:39:04 +0000, Edmund Horner wrote:

Created attachment 2153
Test case showing colour key conversion bug

When a 16-bit "565 format" surface has a colour key set, it will blit with correct transparency. If, however, it has its colour key set then is converted to a 32-bit ARGB format surface, the colour key in the converted image will not necessarily be the same pixel value as the transparent pixels. It may not blit correctly, because the colour key does not match the right pixels.

In my case, with an image using 0xB54A for transparency, the colour key was converted to 180,170,82; but the corresponding pixels (with the same original value) were converted to 180,169,82. Blitting the converted image did not use transparency where expected.

I have attached a test case. The bug has been replicated on both x86_64 Linux (SDL 2.0.2), and 32-bit MS C++ 2010 on Windows (SDL 2.0.0).

On 2015-05-10 09:44:53 +0000, Ellie wrote:

FWIW, I can also reproduce on Linux x86_64 (Fedora 21) with SDL 2.0.3.

On 2015-08-11 12:25:18 +0000, Pankaj wrote:

Hi Edmund Horner,

when RGB565 format surface is converted to 32 bit ARGB8888 format using API SDL_ConvertSurfaceFormat.It adds up alpha channel in the converted surface.
SDL_SetColorKey API sets the RGB color pixel to transparent.
But after converting surface to 32 bit ARGB8888 by blitting. Alpha channel with 0xff value is coming.

Example :

Pixel in RGB565 format = 0x0000b54a (180,170,82)

After blitting 16 bit SDL_Surface to 32 bit SDL_Surface
Pixel in ARGB8888 format = 0xffb4a952 (180,169,82)

In your example:
After blitting RGB has changed.(In above case it is coming different in case of Green value)
But it is blitted with correct values when I use pixel values other than 0xb54a. Like 0xBA54 in place of 0xb54a.There RGB value comes same.

If color key set is 0xb54a. And it does not contain alpha channel. So when it goes for matching pixels from converted surface with color key set. It fails to match and shows opaque pixels because of alpha channel in converted surface with the RGB value.

So this is expected behaviour

On 2016-06-23 02:55:45 +0000, Edmund Horner wrote:

The alpha channel is not the issue.

The issue is that colour key conversion maps a 6-bit green value 42 to 170 (which seems correct, as 42/63 = 170/255).

But 565 pixel conversion maps the same 6-bit value 42 to 169. So even though the newly converted surface has a colour key, it's now a different set of pixels that will appear transparent when it is blitted.

The pixel conversion happens in Blit_RGB565_32 (SDL_blit_N.c) using a lookup table.

The problem is the lookup table can't replicate the same rounding effects, since it maps the top 3 bits of green to one value, and the bottom bits to a different value, and then sums them. This isn't a problem for Red and Blue whose values come from only one lookup.

On 2016-10-08 00:39:53 +0000, Sam Lantinga wrote:

Any suggestions on a fix?

On 2016-11-08 17:12:08 +0000, Sylvain wrote:

Created attachment 2608
patch

Some idea of patch ...
It uses a temporary surface to have the color-key converted.

On 2016-11-08 17:53:10 +0000, Sylvain wrote:

Created attachment 2609
patch

some mistake, updated patch!

On 2016-11-10 01:26:45 +0000, Edmund Horner wrote:

Two ideas come to mind (and I haven't looked at Sylvain's patch yet):

  1. Perform the conversion of the colour key using the same mapping table. The inaccuracy caused by the table is extremely minor, being +/- 1 for all 256 possible green inputs, I believe.

  2. Allow a mode (perhaps controlled by a SDL hint) that uses a slower conversion of the pixel data, using the same formula as is currently used for the colour key.

PS. Thanks so much for your work on SDL!

On 2017-08-12 23:59:59 +0000, Sam Lantinga wrote:

This fix is so monstrous, I can't even tell you. However, I think it is the one way to be 100% correct, so I'm adding it, thanks!
https://hg.libsdl.org/SDL/rev/ecc1f6b82d95

On 2017-08-13 13:15:18 +0000, Sylvain wrote:

Agree this is not the best thing ... It would be better if the pixel conversion routine could be called directly.

On 2017-08-14 04:24:35 +0000, Sam Lantinga wrote:

That can be cleanup sometime in the future. :)

On 2017-09-13 17:09:57 +0000, Sylvain wrote:

I am reopening, because there might be an issue.
when converting a surface with palette and colorkey, to a rgb surface.
the "new" colorkey is wrong.

I think when do a "SDL_FillRect(tmp, NULL, surface->map->info.colorkey);" with the real color, instead of using the key color.
Any idea ?

On 2017-09-13 21:04:41 +0000, Sylvain wrote:

some more precisions.

if "surface" has a palette,

int ck = SDL_MapRGB(surface->format, 255,0,255);
SDL_SetColorKey(surface, SDL_TRUE, ck);
int ck2 = SDL_MapRGB(surface->format, 255,0,255);

ck != ck2

because internally, the palette has an alpha value, and SDL_SetColorKey toggles with opaque/transparency. I think this is an issue for the previous patch :(

On 2017-09-14 07:28:01 +0000, Sylvain wrote:

Created attachment 2936
patch for the case of the input surface has a palette.

Ok, this is more clear now:

When the input surface has a palette, we need to share it internally for the color-key conversion. That's the patch.

There is another modification in the patch (SDL_pixels.c): to be applied, the palette does not need to be exactly "palette->ncolors == (1 << format->BitsPerPixel)". Palette are not always power of two.

On 2017-09-14 12:33:58 +0000, Ryan C. Gordon wrote:

*** Bug 3826 has been marked as a duplicate of this bug. ***

On 2017-09-14 12:38:18 +0000, Ryan C. Gordon wrote:

(In reply to Sylvain from comment # 13)

Created attachment 2936 [details]
patch for the case of the input surface has a palette.

This patch is now https://hg.libsdl.org/SDL/rev/3d1698bc2747, thanks!

--ryan.

On 2020-05-17 12:24:53 +0000, Holger Schemel wrote:

I just found out that this (or a very similar) problem reappeared in the current stable SDL release 2.0.12 (and it is also in 2.0.13 in Mercurial as of today):

Summary: Using the current stable SDL2 2.0.12 release, there are apparently certain cases where SDL_ConvertSurface() corrupts a surface's color key that was explicitly set using SDL_SetColorKey().

How to reproduce the problem: Build and execute the minimal code example with PNG image file attached to the report for bug 3826 (by typing "make"). (Tested on macOS X 10.14.)

Expected behaviour: The black background of the image should be displayed transparently (as it is the case with SDL2 2.0.10).

Observed behaviour: The black pixels of the image are non-transparent.

This bug potentially breaks every SDL2 program that uses SDL_SetColorKey() followed by SDL_ConvertSurface() to set a certain pixel color in a surface as being transparent and convert the surface to a different surface format (in my case to the “native” format for blitting to the target background surface).

The code example mentioned above can be found in my bug report from 2017 (which was marked as a duplicate of this bug):

https://bugzilla.libsdl.org/show_bug.cgi?id=3826

I haven't debugged this problem any further so far.

On 2020-05-17 12:34:06 +0000, Holger Schemel wrote:

P.S.: To run the code example without failing on SDL_GetColorKey(), and to display the image with the transparency problem, just add a "return;" as the very first line of the function "PrintColorkey()" at the beginning of the example code.

On 2020-05-17 18:51:43 +0000, Sylvain wrote:

I think this is fix in https://hg.libsdl.org/SDL/rev/5c4ad9fc7b21

Fix issue with colorkey, palette and format conversion
Set the colorkey information on the converted surface.
Test-case in bug 3826/2979, conflicting with bug 4798

( With bug 4798, the colorkey is applied by setting the palette colour to alpha and blitting. see
https://hg.libsdl.org/SDL/rev/0c66be754e29 )

On 2020-05-17 19:25:59 +0000, Sylvain wrote:

Suite: https://hg.libsdl.org/SDL/rev/c0689749d4b4

Only need to set the colorkey on the converted surface if it has no alpha channel.

On 2020-05-19 11:11:56 +0000, Holger Schemel wrote:

Thanks a lot! I can confirm that using the current SDL2 from Mercurial containing your two patches, not only my minimal code example program from bug 3826 works correctly again, but also the images from my game "Rocks'n'Diamonds" now have correct transparency again! :-)

Is it possible to add the mentioned code example as an automated regression test somehow to check future SDL versions against this potential pitfall?

On 2020-05-19 15:59:22 +0000, Sylvain wrote:

ok great, so I mark this as fixed!

Yes it would be nice to have a complete test-suite ! Please submit a patch to improve what exists

sulix added a commit to sulix/sdl12-compat that referenced this issue Sep 14, 2023
The LGP port of Hyperspace Delivery Boy has broken colour keys if run in
32-bpp mode (see bug libsdl-org#317). This is because it relies heavily on the
imprecise RGB565->RGB888 conversion in earlier SDL 1.2 versions, when
running in 32-bpp mode.

The game's assets are all in 565 format, and the game converts these to
the screen's format on load. It then sets a colour key. This presents a
problem, because:
- The generic BlitNToN implementation in SDL 1.2 just shifted the
  values, so the resulting image was not at full range. Magenta became
  (F800F8).
- Early versions of SDL 1.2 fell back to the BlitNToN blitter very
  frequently:
  libsdl-org/SDL-1.2@6f4a75d
- So, Hyperspace Delivery Boy calls SDL_MapRGB(0xF8, 0, 0xF8) to get the
  colour key, then sets it on the converted surface.
- In SDL 2.0, the blitters now properly do a full-range conversion, so
  the magenta becomes (FF00FF), which now doesn't match the hardcoded
  (F800F8).
- That being said, in general, it's not guaranteed that SDL_MapRGB()
  will do the same format conversion as SDL_CovertSurface(), so the
  "correct" way of handling this is to set the colour key before
  converting, which works (albeit slowly) in SDL2:
  libsdl-org/SDL#1854
- Since the conversion behaviour is different even between SDL 1.2
  versions, it's not worth trying to imitate it here, so we just force
  the game to run in 16-bpp mode, which works fine.
- (And the game's README recommends it, too.)
slouken pushed a commit to libsdl-org/sdl12-compat that referenced this issue Sep 14, 2023
The LGP port of Hyperspace Delivery Boy has broken colour keys if run in
32-bpp mode (see bug #317). This is because it relies heavily on the
imprecise RGB565->RGB888 conversion in earlier SDL 1.2 versions, when
running in 32-bpp mode.

The game's assets are all in 565 format, and the game converts these to
the screen's format on load. It then sets a colour key. This presents a
problem, because:
- The generic BlitNToN implementation in SDL 1.2 just shifted the
  values, so the resulting image was not at full range. Magenta became
  (F800F8).
- Early versions of SDL 1.2 fell back to the BlitNToN blitter very
  frequently:
  libsdl-org/SDL-1.2@6f4a75d
- So, Hyperspace Delivery Boy calls SDL_MapRGB(0xF8, 0, 0xF8) to get the
  colour key, then sets it on the converted surface.
- In SDL 2.0, the blitters now properly do a full-range conversion, so
  the magenta becomes (FF00FF), which now doesn't match the hardcoded
  (F800F8).
- That being said, in general, it's not guaranteed that SDL_MapRGB()
  will do the same format conversion as SDL_CovertSurface(), so the
  "correct" way of handling this is to set the colour key before
  converting, which works (albeit slowly) in SDL2:
  libsdl-org/SDL#1854
- Since the conversion behaviour is different even between SDL 1.2
  versions, it's not worth trying to imitate it here, so we just force
  the game to run in 16-bpp mode, which works fine.
- (And the game's README recommends it, too.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant