Created attachment 2938 [details]
palette image for the testcase

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...

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) {

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);
}

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.

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.

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)

Created attachment 3581 [details]
palette with same RGB and various alpha: image

It needs a specific image because the testcase patches the palette ...

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

Fixed in https://hg.libsdl.org/SDL/rev/884f99b039f0 !

marked as fixed

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 )

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

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

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.

https://hg.libsdl.org/SDL/rev/078a70793bd9

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.

Fix: set to opaque when a palette surface is converted to an alpha format.

https://hg.libsdl.org/SDL/rev/96382c849dec

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.

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;

Thanks, fix in https://hg.libsdl.org/SDL/rev/2f75c19fd2f2 !

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

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.

Thanks, fixed in https://hg.libsdl.org/SDL/rev/b1ebbd8cafef