Fixed bug 3023 - setting a white and then non-white texture color mod breaks the texture with software renderer

Adam M.

Okay, here is the problem, I think.

During the first blit, RLEAlphaSurface is called to do RLE conversion of the RGBA source into a format allowing it "to be quickly alpha-blittable onto dest". Since the destination is the screen, it has no alpha channel. RLEAlphaSurface calls copy_opaque(dst, src + runstart, len, sf, df) (where copy_opaque is copy_32), which has this code:

SDL_RLEaccel.c:984:
  RGBA_FROM_8888(*src, sfmt, r, g, b, a);
  PIXEL_FROM_RGBA(*d, dfmt, r, g, b, a);

On the first line, it reads the source pixel 0xFFFFFFFF. The second line drops the alpha value (because dfmt for the screen has no alpha channel) and writes 0x00FFFFFF. Later, when the RLE conversion is being undone, uncopy_32 is called, which has the following code:

SDL_RLEaccel.c:1001:
  Uint32 pixel = *s++;
  RGB_FROM_PIXEL(pixel, sfmt, r, g, b);
  a = pixel >> 24;
  PIXEL_FROM_RGBA(*dst, dfmt, r, g, b, a);

However, the the alpha channel has already been dropped by copy_opaque (= copy_32), so pixel = 0x00FFFFFF and 'a' becomes 0. Thus, all opaque pixels lose their alpha channel when being unRLE'd. (I don't know what happens to pixels with alpha from 1-254, but they should be checked too.)

So, that seems to be the problem, but I'm not sure what the solution should be. Since opaque pixels have alpha == 255, I'm thinking to create another uncopy function for opaque pixels that simply uses 255 for alpha.

However, there may be other problems here. For translucent pixels, uncopy_32 assumes the alpha channel is stored in the upper 8 bits, but copy_32 doesn't store it there. Instead, it stores it in whatever location is appropriate for the destination surface. Isn't one of their behaviors incorrect, given the other? I'm not sure which to change, however.

For translucent pixels, it seems that the blit function uses do_blend, which is the BLIT_TRANSL_888 macro, which also assumes alpha is in top 8 bits. It has the comment "we have made sure the alpha is stored in the top 8 bits...", but it seems that's not true (copy_32 doesn't make sure the alpha goes there).

Perhaps the correct fix is to make copy_32 put the alpha there, but then that seems to require that RLE conversion be limited to destination surfaces that don't use the upper 8 bits. However, looking further, it seems that has already been done: if (masksum != 0x00ffffff) return -1; /* requires unused high byte */

src/video/SDL_RLEaccel.c

@@ -373,6 +373,18 @@
         }                                                       \
     } while(0)
 
+/*
+ * Set a pixel value using the given format, except that the alpha value is
+ * placed in the top byte. This is the format used for RLE with alpha.
+ */
+#define RLEPIXEL_FROM_RGBA(Pixel, fmt, r, g, b, a)                      \
+{                                                                       \
+    Pixel = ((r>>fmt->Rloss)<<fmt->Rshift)|                             \
+        ((g>>fmt->Gloss)<<fmt->Gshift)|                                 \
+        ((b>>fmt->Bloss)<<fmt->Bshift)|                                 \
+        (a<<24);                                                        \
+}
+
 /*
  * This takes care of the case when the surface is clipped on the left and/or
  * right. Top clipping has already been taken care of.
@@ -982,7 +994,7 @@ copy_32(void *dst, Uint32 * src, int n,
     for (i = 0; i < n; i++) {
         unsigned r, g, b, a;
         RGBA_FROM_8888(*src, sfmt, r, g, b, a);
-        PIXEL_FROM_RGBA(*d, dfmt, r, g, b, a);
+        RLEPIXEL_FROM_RGBA(*d, dfmt, r, g, b, a);
         d++;
         src++;
     }