We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3827 - issue with MapRGB, palette and colorkey
Summary: issue with MapRGB, palette and colorkey
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: *don't know* (show other bugs)
Version: don't know
Hardware: All All
: P2 normal
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-14 08:07 UTC by Sylvain
Modified: 2020-03-17 14:45 UTC (History)
3 users (show)

See Also:


Attachments
testcase (2.39 KB, text/x-csrc)
2017-09-14 08:07 UTC, Sylvain
Details
palette image for the testcase (21.12 KB, image/bmp)
2017-09-14 08:07 UTC, Sylvain
Details
palette with same RGB and various alpha: testcase.c (5.95 KB, text/x-csrc)
2019-01-21 17:26 UTC, Sylvain
Details
palette with same RGB and various alpha: image (49.95 KB, image/bmp)
2019-01-21 17:27 UTC, Sylvain
Details
test_1.c (4.41 KB, text/x-csrc)
2019-01-21 22:06 UTC, Sylvain
Details
test-case set opaque (5.57 KB, text/x-csrc)
2020-02-23 22:00 UTC, Sylvain
Details
fix implicit declaration of SDL_DetectPalette (388 bytes, patch)
2020-03-17 12:55 UTC, Mathieu Eyraud
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sylvain 2017-09-14 08:07:32 UTC
Created attachment 2937 [details]
testcase

the MapRGB value of the colorkey changes whether the surface has a palette or not.

See the test case: 

INFO: before any colorkey, SDL_MapRGB(255,0,255) == 30
INFO: after setting the colorkey, SDL_MapRGB(255,0,255) == 79
INFO: after removing the colorkey, SDL_MapRGB(255,0,255) == 30
INFO: after setting the colorkey, SDL_MapRGB(255,0,255) == 79
INFO: using SDL_GetColorkey(), colorkey == 30
Comment 1 Sylvain 2017-09-14 08:07:55 UTC
Created attachment 2938 [details]
palette image for the testcase
Comment 2 Sylvain 2017-09-14 08:11:28 UTC
SDL_Surface.c, SDL_SetColorKey() is toggling the palette: 

surface->format->palette->colors[surface->map->info.colorkey].a = SDL_ALPHA_TRANSPARENT;
vs
surface->format->palette->colors[surface->map->info.colorkey].a = SDL_ALPHA_OPAQUE;

which may be even worse if the palette is shared between several surfaces...
Comment 3 Sylvain 2017-09-15 14:06:10 UTC
typo, in the bug description:

"the MapRGB value of the colorkey changes whether the surface has this colorkey or not."


It's introduced at:

https://hg.libsdl.org/SDL/rev/1d49dc7b5ce9 (SDL_surface.c, SDL_SetColorKey() part)
https://hg.libsdl.org/SDL/rev/4eb8ac58642c

Comment says it fixes bug 1746

(but this issue could also be solved by bug 2979)

patch would be :

--- a/src/video/SDL_surface.c	Thu Sep 14 19:33:32 2017 +0200
+++ b/src/video/SDL_surface.c	Fri Sep 15 16:05:34 2017 +0200
@@ -229,21 +229,7 @@
     if (flag) {
         surface->map->info.flags |= SDL_COPY_COLORKEY;
         surface->map->info.colorkey = key;
-        if (surface->format->palette) {
-            surface->format->palette->colors[surface->map->info.colorkey].a = SDL_ALPHA_TRANSPARENT;
-            ++surface->format->palette->version;
-            if (!surface->format->palette->version) {
-                surface->format->palette->version = 1;
-            }
-        }
     } else {
-        if (surface->format->palette) {
-            surface->format->palette->colors[surface->map->info.colorkey].a = SDL_ALPHA_OPAQUE;
-            ++surface->format->palette->version;
-            if (!surface->format->palette->version) {
-                surface->format->palette->version = 1;
-            }
-        }
         surface->map->info.flags &= ~SDL_COPY_COLORKEY;
     }
     if (surface->map->info.flags != flags) {
Comment 4 David Lönnhager 2019-01-16 11:44:46 UTC
Here's a workaround I used for pygame, though it requires that you use the exact color key value:

static Uint32
pg_map_rgb(SDL_Surface *surf, Uint8 r, Uint8 g, Uint8 b)
{
    /* SDL_MapRGB() returns wrong values for color keys
       for indexed formats since alpha = 0 */
    Uint32 key;
    if (!surf->format->palette)
        return SDL_MapRGB(surf->format, r, g, b);
    if (!SDL_GetColorKey(surf, &key)) {
        Uint8 keyr, keyg, keyb;
        SDL_GetRGB(key, surf->format, &keyr, &keyg, &keyb);
        if (r == keyr && g == keyg && b == keyb)
            return key;
    } else
        SDL_ClearError();
    return SDL_MapRGBA(surf->format, r, g, b, SDL_ALPHA_OPAQUE);
}

static Uint32
pg_map_rgba(SDL_Surface *surf, Uint8 r, Uint8 g, Uint8 b, Uint8 a)
{
    if (!surf->format->palette)
        return SDL_MapRGBA(surf->format, r, g, b, a);
    return pg_map_rgb(surf, r, g, b);
}
Comment 5 Sylvain 2019-01-20 14:15:44 UTC
My previous patch is incomplete, it needs also to remove thoses lines:

-            } else if (format->Amask) {
-                /* The alpha was set in the destination from the palette */

so that it goes into "set colorkey by color" path.
Comment 6 Sylvain 2019-01-20 14:19:07 UTC
Then, I see other flaws:

1) If you set colorkey successively to several colors. Only the last one should be the colorkey, but 
currently, all the previous colors remain in "transparent" state.
For instance, if you set many colorkeys, you can end up with a blank/transparent surface.

2) Even, if you clear those colorkeys before, it loses the Alpha information, because it's set back to opaque.
(alpha information isn't stored).

=> at this point we should go for the patch.


3) Find_Color() gives the same priority for R,G,B and A, but I think RGB should be higher priority.
 (and SDL_FindColor could be made 'static' )

for instance, 
  distance = (((rd * rd) + (gd * gd) + (bd * bd)) << 16) + (ad * ad);

but it requires 64 bits (which works bad on android arm v7a), so absolute values would be just fine.
Comment 7 Sylvain 2019-01-21 17:26:05 UTC
Created attachment 3580 [details]
palette with same RGB and various alpha:  testcase.c

quick testcase.c: 

It has a palette with same RGB triplet but various alpha values.
If you set a ColorKey to a specific RGB+A, only this value is made transparent.

This is should be for bug 1746
https://hg.libsdl.org/SDL/rev/1d49dc7b5ce9 
https://hg.libsdl.org/SDL/rev/4eb8ac58642c 

But just to make sure there is no regression, since this is about reverting something from bug 1746.
(current patch is incomplete)
Comment 8 Sylvain 2019-01-21 17:27:54 UTC
Created attachment 3581 [details]
palette with same RGB and various alpha: image

It needs a specific image because the testcase patches the palette ...
Comment 9 Sylvain 2019-01-21 17:37:37 UTC
For the record, impacted issues: 

Bug 1746: Transparency set with SDL_ColorKey fails to transfer over to SDL_Textures with SDL_CreateTextureFromSurface()

https://hg.libsdl.org/SDL/rev/1d49dc7b5ce9
https://hg.libsdl.org/SDL/rev/4eb8ac58642c
https://hg.libsdl.org/SDL/rev/72cb3e205571

Bug 2979: SDL_ConvertSurface does not convert color keys consistently
https://hg.libsdl.org/SDL/rev/ecc1f6b82d95
https://hg.libsdl.org/SDL/rev/3d1698bc2747

Bug 2009: Bug Alpha + ColorKey + 32 bpp
https://hg.libsdl.org/SDL/rev/fdf89883bbaa
https://hg.libsdl.org/SDL/rev/9829fb1aeac7


SDL_ConvertColorKeyToAlpha in SDL_CreateTextureFromSurface()
https://hg.libsdl.org/SDL/rev/fa1095d42a5b
SDL_CreateTextureFromSurface

Moved to SDL_ConvertSurface()
https://hg.libsdl.org/SDL/rev/93764fe8601a
Comment 10 Sylvain 2019-01-21 19:33:10 UTC
Fixed in https://hg.libsdl.org/SDL/rev/884f99b039f0 !
Comment 11 Sylvain 2019-01-21 19:33:24 UTC
marked as fixed
Comment 12 Sylvain 2019-01-21 22:06:11 UTC
Created attachment 3582 [details]
test_1.c

Another test where it shows a difference (conversion from palette to argb8888)

before: the alpha isn't used.
now: the alpha is used
(need the previous image: img_palette_bug_3827.bmp )
Comment 13 Sylvain 2019-09-10 15:14:36 UTC
Also, bug 4798:  PNG w/transparency breaks in SDL 2.0.10 but works in SDL 2.0.9
and fix: https://hg.libsdl.org/SDL/rev/0c66be754e29
Comment 14 Sylvain 2020-02-23 21:16:26 UTC
For the record: https://hg.libsdl.org/SDL/rev/c66d1c7700bd

Fixed bug 4999 - Palette surface always promoted to alpha (Thanks Cameron Gutman!)
MSVC Static analysis: Incorrect alpha_value check in SDL_render.c
Comment 15 Sylvain 2020-02-23 21:36:19 UTC
Fix regression: when a palette + colorkey is converted, it needs a blend mode.

- Regression of test_1.c of bug 3827, after fix from bug 4798.
- Blending is also needed when the palette contains alpha value, but not necessarily colorkey.
- Clean up SDL_ConvertColorkeyToAlpha which doesn't seem to need 'ignore_alpha' parameter any-more.
Comment 16 Sylvain 2020-02-23 21:36:51 UTC
https://hg.libsdl.org/SDL/rev/078a70793bd9
Comment 17 Sylvain 2020-02-23 22:00:20 UTC
Created attachment 4226 [details]
test-case set opaque

New test-case, for this situation:

- the surface has a palette with no alpha values (all alpha field are 0).
- When using the GLES 1 render, the only texture format is ARGB.

=> the texture that gets created is always fully transparent, whereas it should be opaque like with other renderers who would select a non alpha texture format.
Comment 18 Sylvain 2020-02-23 22:05:50 UTC
Fix: set to opaque when a palette surface is converted to an alpha format.

https://hg.libsdl.org/SDL/rev/96382c849dec
Comment 19 Sylvain 2020-02-24 20:59:23 UTC
Previous "test-case set opaque"
isn't robust when we also use the colorkey !
(eg adding SDL_SetColorKey(textbuf, SDL_TRUE, 200); )
This issue is that's not possible to set opaque pixels after the color-key has forced the transparency.


Better fix to set the palette opaque, when there is also a colorkey:
https://hg.libsdl.org/SDL/rev/4efb3eb7a3b3

The palette, if it has no alpha, is forced to opaque before conversion. Then restored.
Comment 20 Cameron Gutman 2020-02-25 03:04:23 UTC
https://hg.libsdl.org/SDL/rev/4efb3eb7a3b3 breaks the build for me using Autotools on Debian 10 (or rather breaks it more, since ./configure doesn't even work right now #5003) 

/opt/SDL2/src/video/SDL_surface.c: In function ‘SDL_ConvertSurface_REAL’:
/opt/SDL2/src/video/SDL_surface.c:1056:13: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
             int i;
Comment 21 Sylvain 2020-02-25 05:53:44 UTC
Thanks, fix in https://hg.libsdl.org/SDL/rev/2f75c19fd2f2 !
Comment 22 Sylvain 2020-03-17 08:38:14 UTC
Bug 5037 - [Regression 2.0.12] Alpha value of 0 on INDEX8 format surfaces is opaque (edit) 
https://hg.libsdl.org/SDL/rev/b68bc68d5cce

added SDL_DetectPalette(SDL_Palette *pal, SDL_bool *is_opaque, SDL_bool *has_alpha_channel)
to detect whether the palette is fully opaque or not, and it if has an alpha channel.
all alpha 0 ->    it's opaque without alpha channel
all alpha 255 ->  it's opaque with alpha channel
otherwise
it's not opaque, and it has an alpha channel
Comment 23 Mathieu Eyraud 2020-03-17 12:55:40 UTC
Created attachment 4267 [details]
fix implicit declaration of SDL_DetectPalette

Include "../video/SDL_pixels_c.h" in SDL_render.c to fix implicit declaration of SDL_DetectPalette.
Comment 24 Sylvain 2020-03-17 14:45:34 UTC
Thanks, fixed in https://hg.libsdl.org/SDL/rev/b1ebbd8cafef