Possible bug in FillRect?

Anyone else see this? Am I just going a bit mad - entirely possible :slight_smile:

If you are doing this:
SDL_FillRect(surface,&(surface->clip_rect),
SDL_MapRGB(new_surface->format,r,g,b)); // (I know the second parameter
could be NULL here)

On a 24 bit pixel surface (e.g. SDL_PIXELFORMAT_BGR24 or
SDL_PIXELFORMAT_RGB24)

You get down to this routine (v1.3.0 build 5557 I think):

static void
SDL_FillRect3(Uint8 * pixels, int pitch, Uint32 color, int w, int h)
{
Uint8 r = (Uint8) ((color >> 16) & 0xFF);
Uint8 g = (Uint8) ((color >> 8) & 0xFF);
Uint8 b = (Uint8) (color & 0xFF);

while (h--) {
int n = w;
Uint8 *p = pixels;

while (n--) {
*p++ = r;
*p++ = g;
*p++ = b;
}
pixels += pitch;
}
}

A Note from the comments for SDL_FillRect in the header ā€¦

  • The color should be a pixel of the format used by the surface, and
  • can be generated by the SDL_MapRGB() function.

Ironically, due to the SDL_MapRGB, the decode of r g b in SDL_FillRect3
is wrong for both BGR24 and RGB24!!!

Regards,
Rob

P.S. I was using the just as a temporary working surface in order to
modify one colour of the image to another. The work around Iā€™ve used is
to move to SDL_PIXELFORMAT_BGR888. However, I think a SDL_LoadBMP() will
return one of these surfaces so itā€™s not impossible to hit this by
accident (which is how I ended up thereā€¦).

The alternative - swap b and r in the SDL_MapRGB() - looks like it would
break once this (possible) bug is fixed.

P.P.S. [Apologies if this is a duplicate post ā€¦ different email
accounts, and all that]

If you are doing this:
SDL_FillRect(surface,&(surface->clip_rect),
SDL_MapRGB(new_surface->format,r,g,b));

Youā€™re asking it to MapRGB from one format, and then then filling it
into a different format surface.

Try this:

SDL_FillRect(surface,&(surface->clip_rect),
             SDL_MapRGB(surface->format,r,g,b));

ā€“ryan.

**
Anyone else see this? Am I just going a bit mad - entirely possible :slight_smile:

If you are doing this:
SDL_FillRect(surface,&(surface->clip_rect),
SDL_MapRGB(new_surface->format,r,g,b)); // (I know the second parameter
could be NULL here)

On a 24 bit pixel surface (e.g. SDL_PIXELFORMAT_BGR24 or
SDL_PIXELFORMAT_RGB24)

You get down to this routine (v1.3.0 build 5557 I think):

static void
SDL_FillRect3(Uint8 * pixels, int pitch, Uint32 color, int w, int h)
{
Uint8 r = (Uint8) ((color >> 16) & 0xFF);
Uint8 g = (Uint8) ((color >> 8) & 0xFF);
Uint8 b = (Uint8) (color & 0xFF);

If we abstract the idea of RGB and just treat them as 3 variables, say XYZ,
then:
X = bits 23:16
Y = bits 15:8
Z = bits 7:0

The upper 8 bits are then ignored. Then for each row of pixels, you find the
XYZ-XYZ-XYZ byte pattern. To make this work for RGB or BGR images, one would
need only ensure that XYZ are mapped to the correct components ahead of
time, which I assumed was the job of SDL_MapRGB(), i.e. X <- R, Y <- G, Z
<- B for RGB, X <- B, Y <- G, Z <- R.

Is this not what happens?On Sun, Jul 17, 2011 at 2:28 PM, Rob Probin wrote:

while (h--) {
    int n = w;
    Uint8 *p = pixels;

    while (n--) {
        *p++ = r;
        *p++ = g;
        *p++ = b;
    }
    pixels += pitch;
}

}

A Note from the comments for SDL_FillRect in the header ā€¦

  • The color should be a pixel of the format used by the surface, and
  • can be generated by the SDL_MapRGB() function.

Ironically, due to the SDL_MapRGB, the decode of r g b in SDL_FillRect3 is
wrong for both BGR24 and RGB24!!!

Regards,
Rob

P.S. I was using the just as a temporary working surface in order to modify
one colour of the image to another. The work around Iā€™ve used is to move to
SDL_PIXELFORMAT_BGR888. However, I think a SDL_LoadBMP() will return one of
these surfaces so itā€™s not impossible to hit this by accident (which is how
I ended up thereā€¦).

The alternative - swap b and r in the SDL_MapRGB() - looks like it would
break once this (possible) bug is fixed.

P.P.S. [Apologies if this is a duplicate post ā€¦ different email
accounts, and all that]


SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org

Hi Ryan,

If you are doing this:

SDL_FillRect(surface,&(surface->clip_rect),
SDL_MapRGB(new_surface->format,r,g,b));

Youā€™re asking it to MapRGB from one format, and then then filling it
into a different format surface.

Try this:

 SDL_FillRect(surface,&(surface->clip_rect),
              SDL_MapRGB(surface->format,r,g,b));

I think that was a copy-and-paste typo - I manually changed (nearly)
all the new_surfaces into surface in order to make the code clearer. DOH!

Anyway, Iā€™ll retest and get back to you.

Regards,
Rob

Hi Patrick,

If we abstract the idea of RGB and just treat them as 3 variables, say XYZ then:
X = bits 23:16
Y = bits 15:8
Z = bits 7:0

The upper 8 bits are then ignored. Then for each row of pixels, you find the
XYZ-XYZ-XYZ byte pattern. To make this work for RGB or BGR images, one would
need only ensure that XYZ are mapped to the correct components ahead of
time, which I assumed was the job of SDL_MapRGB(), i.e. X<- R, Y<- G, Z
<- B for RGB, X<- B, Y<- G, Z<- R.

Is this not what happens?

It was a couple of days ago ā€¦ but I got the impression that wasnā€™t
happening. Looks like Iā€™m retesting anyway - Iā€™ll supply more detailsā€¦

Regards,
Rob

Following up from my previous email about my problems with FillRect with
24-bit surfacesā€¦

See the program below. If you run this instead of getting a ā€˜redā€™ image
with no visible changes, you get ā€˜Redā€™, ā€˜Blueā€™, ā€˜Blueā€™ (I have a 0.5
second pause between subsequent renders in the program below).

Pixel (out from SDL_MapRGB) for the 3 values of ā€˜iā€™:
i=0 pixel=0xff0000ff SDL_PIXELFORMAT_ABGR8888
i=1 pixel=0xff0000 SDL_PIXELFORMAT_BGR24
i=2 pixel=0xff SDL_PIXELFORMAT_RGB24

The code also can be made to load a BMP image by uncommenting #define
LOAD_TEST and creating a, say, 32x32 pixel image in pure RED (255,0,0)
and save as Windows Bitmap called ā€˜red.bmpā€™. This prints a square of
red, as expected.

The first 16 bytes of pixel data in the load red case is:

i=0 data = ff 00 00 ff ff 00 00 ff ff 00 00 ff ff 00 00 ff
SDL_PIXELFORMAT_ABGR8888
i=1 data = 00 00 ff 00 00 ff 00 00 ff 00 00 ff 00 00 ff 00
SDL_PIXELFORMAT_BGR24
i=2 data = ff 00 00 ff 00 00 ff 00 00 ff 00 00 ff 00 00 ff
SDL_PIXELFORMAT_RGB24

In answer something that Patrick Baggett got me thinking about: Does
this mean SDL_MapRGB is not functioning correctly for some reason
instead? Except itā€™s a very simple routine just using the
Rmask/Gmask/Bmask/Amask.

For for the three values of iā€¦ (both from SDL_PixelFormatEnumToMasks
and out of ā€˜surface->formatā€™)

i=0 Rmask=0xff Gmask=0xff00 Bmask=0xff0000 Amask=0xff000000
SDL_PIXELFORMAT_ABGR8888
i=1 Rmask=0xff0000 Gmask=0xff00 Bmask=0xff Amask=0
SDL_PIXELFORMAT_BGR24
i=2 Rmask=0xff Gmask=0xff00 Bmask=0xff0000 Amask=0
SDL_PIXELFORMAT_RGB24

This doesnā€™t look right to me, based on the names - does anyone know
what they should be? And why blit from a load surface should work correctly?

Any thoughts?

Regards,
Rob

//
// Test program for FillRect
//
#include ā€œSDL.hā€

//#define LOAD_TEST

#define SCREEN_WIDTH 320
#define SCREEN_HEIGHT 480

#define max_types 3
SDL_Texture* texture[max_types];
int formats[max_types] = { SDL_PIXELFORMAT_ABGR8888,
SDL_PIXELFORMAT_BGR24, SDL_PIXELFORMAT_RGB24 }; /* desired texture
format */

void render(SDL_Renderer *renderer)
{
static int i = 0;
if(i >= max_types) { i = 0; }
SDL_RenderCopy(renderer, texture[i], 0, 0);
SDL_RenderPresent(renderer);
i++;
}

#ifndef LOAD_TEST
void create_textures(SDL_Renderer renderer)
{
for(int i = 0; i < max_types; i++)
{
Uint32 Rmask, Gmask, Bmask, Amask; /
masks for desired
format /
int bpp; /
bits per pixel for desired format */
int success = SDL_PixelFormatEnumToMasks(formats[i], &bpp,
&Rmask, &Gmask, &Bmask,
&Amask);
if(!success) { printf(ā€œBrokenā€); exit(1); }

     SDL_Surface *surface = SDL_CreateRGBSurface(formats[i], 

SCREEN_WIDTH, SCREEN_HEIGHT,
bpp, Rmask,
Gmask,Bmask, Amask);
if(!surface) { printf(ā€œOopsā€); exit(1); }
Uint32 pixel = SDL_MapRGB(surface->format,255,0,0); // all red
SDL_FillRect(surface, 0, pixel);

     texture[i] = SDL_CreateTextureFromSurface(renderer, surface);
     if (texture[i] == 0) {
         printf("texture creation failed: %s\n", SDL_GetError());
         exit(1);
     } else {
         /* set blend mode for our texture */
         SDL_SetTextureBlendMode(texture[i], SDL_BLENDMODE_BLEND);
     }
     SDL_FreeSurface(surface);
 }

}
#endif

#ifdef LOAD_TEST
void load_test(SDL_Renderer *renderer)
{
for(int i = 0; i < max_types; i++)
{
SDL_Surface *load_surface = SDL_LoadBMP(ā€œred.bmpā€);
if (!load_surface) {
printf(ā€œError loading bitmap: %s\nā€, SDL_GetError());
exit(1);
}

     Uint32 Rmask, Gmask, Bmask, Amask;      /* masks for desired 

format /
int bpp; /
bits per pixel for desired format */
int success = SDL_PixelFormatEnumToMasks(formats[i], &bpp,
&Rmask, &Gmask, &Bmask,
&Amask);
if(!success) { printf(ā€œBrokenā€); exit(1); }

     SDL_Surface *surface = SDL_CreateRGBSurface(formats[i], 

SCREEN_WIDTH, SCREEN_HEIGHT,
bpp, Rmask,
Gmask,Bmask, Amask);
if(!surface) { printf(ā€œOopsā€); exit(1); }

     int error = SDL_BlitSurface(load_surface, NULL, surface, NULL);
     if(error)
     {
         printf("SDL_BlitSurface error: %s\n", SDL_GetError());
         exit(1);
     }

     texture[i] = SDL_CreateTextureFromSurface(renderer, surface);
     if (texture[i] == 0) {
         printf("texture creation failed: %s\n", SDL_GetError());
         exit(1);
     } else {
         /* set blend mode for our texture */
         SDL_SetTextureBlendMode(texture[i], SDL_BLENDMODE_BLEND);
     }
     SDL_FreeSurface(surface);
 }

}
#endif

int main(int argc, char *argv[])
{

 SDL_Window *window;
 SDL_Renderer *renderer;
 int done;
 SDL_Event event;

 /* initialize SDL */
 if (SDL_Init(SDL_INIT_VIDEO) < 0) {
     printf("Could not initialize SDL\n");
     return 1;
 }

 /* create window and renderer */
 window =
     SDL_CreateWindow(NULL, 0, 0, SCREEN_WIDTH, SCREEN_HEIGHT,
                      SDL_WINDOW_OPENGL | SDL_WINDOW_SHOWN);
 if (!window) {
     printf("Could not initialize Window\n");
     return 1;
 }

 renderer = SDL_CreateRenderer(window, -1, 0);
 if (!renderer) {
     printf("Could not create renderer\n");
     return 1;
 }

#ifndef LOAD_TEST
create_textures(renderer);
#else
load_test(renderer);
#endif

 /* Enter render loop, waiting for user to quit */
 done = 0;
 while (!done) {
     while (SDL_PollEvent(&event)) {
         if (event.type == SDL_QUIT) {
             done = 1;
         }
     }
     render(renderer);
     SDL_Delay(500);
 }

 /* shutdown SDL */
 SDL_Quit();

 return 0;

}