Refactor _img_sdl2.pyx load_from_surface (cleanup)

* Fix loading of SDL_PIXELFORMAT_BGR24 surfaces (BMP2, BMP3)
* Fix loading of (some) PNG formats with binary alpha
* Fix byte order for big-endian arch (untested)

More info: #5274

kivy/core/image/_img_sdl2.pyx

@@ -58,57 +58,83 @@ def save(filename, w, h, fmt, pixels, flipped):
     IMG_SavePNG(image, c_filename)
     SDL_FreeSurface(image)
 
-
+# NOTE: This must be kept up to date with ImageData supported formats. If you
+# add support for converting/uploading (for example) ARGB, you must ensure
+# that it is returned unmodified below to avoid converting to RGB/RGBA.
+#
+# FIXME:
+# - Some PNG48 images with binary transparency load incorrectly
+# - (Some?) 8/16 bit grayscale PNGs load incorrectly (likely SDL_Image problem)
 cdef load_from_surface(SDL_Surface *image):
     cdef SDL_Surface *image2 = NULL
     cdef SDL_Surface *fimage = NULL
-    cdef SDL_PixelFormat pf
+    cdef int want_rgba = 0, want_bgra = 0, target_fmt = 0, n = 0
     cdef bytes pixels
 
-    try:
+    if image == NULL:
-        if image == NULL:
+        Logger.warn('ImageSDL2: load_from_surface() with NULL surface')
-            return None
+        return None
 
-        fmt = ''
+    # SDL 2.0.5 now has endian-agnostic 32-bit pixel formats like RGB24,
-        if image.format.BytesPerPixel == 3:
+    # but we can't count on that yet (RGB24 is available since 2.0)
-            fmt = 'rgb'
+    if SDL_BYTEORDER == SDL_BIG_ENDIAN:
-        elif image.format.BytesPerPixel == 4:
+        want_rgba = SDL_PIXELFORMAT_RGBA8888
-            fmt = 'rgba'
+        want_bgra = SDL_PIXELFORMAT_BGRA8888
+    else:
+        want_rgba = SDL_PIXELFORMAT_ABGR8888
+        want_bgra = SDL_PIXELFORMAT_ARGB8888
 
-        # FIXME the format might be 3 or 4, but it doesn't mean it's rgb/rgba.
+    # Output format (string) - supported by ImageData. If the surface is
-        # It could be argb, bgra etc. it needs to be detected correctly. I guess
+    # already in a supported pixel format, no conversion is done.
-        # we could even let the original pass, bgra / argb support exists in
+    fmt = ''
-        # some opengl card.
 
-        if fmt not in ('rgb', 'rgba'):
+    # 32-bit rgba and bgra can be used directly
-            if fmt == 'rgb':
+    if image.format.format == want_rgba:
-                pf.format = SDL_PIXELFORMAT_BGR888
+        fmt = 'rgba'
-                fmt = 'rgb'
+    elif image.format.format == want_bgra:
-            else:
+        fmt = 'bgra'
-                pf.format = SDL_PIXELFORMAT_ABGR8888
+
+    # Alpha mask or colorkey must be converted to rgba
+    elif image.format.Amask or SDL_GetColorKey(image, NULL) == 0:
+        fmt = 'rgba'
+        target_fmt = want_rgba
+
+    # Palette with alpha is converted to rgba
+    elif image.format.palette != NULL:
+        for n in xrange(0, image.format.palette.ncolors):
+            if image.format.palette.colors[n].a < 0xFF:
                 fmt = 'rgba'
+                target_fmt = want_rgba
+                break
 
-            image2 = SDL_ConvertSurfaceFormat(image, pf.format, 0)
+    # 24bpp RGB/BGR without colorkey can be used directly.
+    elif image.format.format == SDL_PIXELFORMAT_RGB24:
+        fmt = 'rgb'
+    elif image.format.format == SDL_PIXELFORMAT_BGR24:
+        fmt = 'bgr'
+
+    # Everything else is converted to RGB
+    if not fmt:
+        fmt = 'rgb'
+        target_fmt = SDL_PIXELFORMAT_RGB24
+
+    # Convert if needed, and return a copy of the raw pixel data
+    try:
+        fimage = image
+        if target_fmt != 0:
+            with nogil:
+                image2 = SDL_ConvertSurfaceFormat(image, target_fmt, 0)
             if image2 == NULL:
-                return
+                Logger.warn('ImageSDL2: error converting {} to {}: {}'.format(
-
+                        SDL_GetPixelFormatName(image.format.format),
-            fimage = image2
+                        SDL_GetPixelFormatName(target_fmt),
-        else:
+                        SDL_GetError()))
-            if (image.format.Rshift > image.format.Bshift):
+                return None
-                memset(&pf, 0, sizeof(pf))
-                pf.BitsPerPixel = 32
-                pf.Rmask = 0x000000FF
-                pf.Gmask = 0x0000FF00
-                pf.Bmask = 0x00FF0000
-                pf.Amask = 0xFF000000
-                image2 = SDL_ConvertSurface(image, &pf, 0)
-                fimage = image2
             else:
-                fimage = image
+                fimage = image2
 
         pixels = (<char *>fimage.pixels)[:fimage.pitch * fimage.h]
         return (fimage.w, fimage.h, fmt, pixels, fimage.pitch)
-
     finally:
         if image2:
             SDL_FreeSurface(image2)

kivy/lib/sdl2.pxi

@@ -28,11 +28,12 @@ cdef extern from "SDL.h":
     int SDL_WINDOWPOS_UNDEFINED
 
     ctypedef enum:
+        SDL_PIXELFORMAT_BGRA8888
         SDL_PIXELFORMAT_ARGB8888
         SDL_PIXELFORMAT_RGBA8888
-        SDL_PIXELFORMAT_RGB888
         SDL_PIXELFORMAT_ABGR8888
-        SDL_PIXELFORMAT_BGR888
+        SDL_PIXELFORMAT_RGB24
+        SDL_PIXELFORMAT_BGR24
 
     ctypedef enum SDL_GLattr:
         SDL_GL_RED_SIZE
@@ -481,10 +482,9 @@ cdef extern from "SDL.h":
     cdef int SDL_SetTextureBlendMode(SDL_Texture * texture, SDL_BlendMode blendMode)
     cdef int SDL_GetTextureBlendMode(SDL_Texture * texture, SDL_BlendMode *blendMode)
     cdef SDL_Surface * SDL_CreateRGBSurfaceFrom(void *pixels, int width, int height, int depth, int pitch, Uint32 Rmask, Uint32 Gmask, Uint32 Bmask, Uint32 Amask)
-    cdef SDL_Surface* SDL_ConvertSurface(SDL_Surface* src, SDL_PixelFormat* fmt, Uint32 flags)
+    cdef SDL_Surface* SDL_ConvertSurfaceFormat(SDL_Surface* src, Uint32 pixel_format, Uint32 flags) nogil
-    cdef SDL_Surface* SDL_ConvertSurfaceFormat(SDL_Surface* src, Uint32
-            pixel_format, Uint32 flags)
     cdef const char* SDL_GetPixelFormatName(Uint32 format)
+    cdef int SDL_GetColorKey(SDL_Surface *surface, Uint32 *key)
     cdef int SDL_Init(Uint32 flags)
     cdef void SDL_Quit()
     cdef int SDL_EnableUNICODE(int enable)
@@ -627,6 +627,11 @@ cdef extern from "SDL.h":
     Uint16 AUDIO_F32MSB #0x9120  /**< As above, but big-endian byte order */
     Uint16 AUDIO_F32    #AUDIO_F32LSB
 
+    # Endianness
+    Uint16 SDL_BYTEORDER
+    Uint16 SDL_LIL_ENDIAN
+    Uint16 SDL_BIG_ENDIAN
+
 cdef extern from "SDL_shape.h":
     cdef SDL_Window * SDL_CreateShapedWindow(
         char *title,