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;

}