How to make a GrayScale filter?

I am using SDL for simplicities sake, but I have found myself not being able to make a grayscale filter. I am curious if anyone here has a website or even code that would help me solve this problem. I am currently working on some test code to get it working. If I solve it I will post ASAP, but it isn't likely to happen. Thanks in advance.

Theres different ways you can calculate greyscale, which give different results.

One method would be to add the rgb components together then divide:

Code:

---

c =  (r + g + b)/3;

---

Or you could measure it by the brightest element:

Code:

---

c = r; if(g > c) c = g; if(b > c) c = b;

---

Or you coudl give bias to certain elements. This is from an example I found, thats meant to give a good grey:

Code:

---

c =  ((r * 61) + (g * 174) + (b * 21)) / 256;

---

Edit: oh, and you might find this tut helpful: http://student.kuleuven.be/~m0216922/CG/color.html

Does this look good?

Code:

---

SDL_LockSurface(SDL_GetVideoSurface());
                for ( int x = 0; x != SDL_GetVideoSurface()->w; ++x )
                {
                        for ( int y = 0; y != SDL_GetVideoSurface()->h; ++y )
                        {
                                Uint32 pixel = getpixel(SDL_GetVideoSurface(),x,y);
                                Uint8 r = 0;
                                Uint8 g = 0;
                                Uint8 b = 0;
                                SDL_GetRGB(pixel, SDL_GetVideoSurface()->format, &r, &g, &b);
                                r = g = b = (( r+g+b )/3);
                                putpixel(SDL_GetVideoSurface(),x,y, SDL_MapRGB(SDL_GetVideoSurface()->format, r,g,b));
                        }
                }
                SDL_UnlockSurface(SDL_GetVideoSurface());

---

EDIT:
I wont always be doing the entire screen, but more asking about my method of doing it.

Well I made my own read/write pixel functions, so I'm not sure exactly how the built in ones work, but it looks like you have more or less the right idea. A few pointers though:

You don't want to be declaring your variables inside a loop.

Theres no need to to initalize r, g, and b

You could save some cpu time by doing something like:

Code:

---

b = (( r+g+b )/3);
putpixel(SDL_GetVideoSurface(),x,y, SDL_MapRGB(SDL_GetVideoSurface()->format, b,b,b));

---

Also i think theres something wrong with your loops here:

Code:

---

for ( int y = 0; y != SDL_GetVideoSurface()->h; ++y )

---

Should instead be:

Code:

---

for (int y = 0; y < SDL_GetVideoSurface()->h; y++ )

---

The putpixel and getpixel are actually my functions that i adapted from the ones SDL provides. Also thanks for the assistance.

Quote:

---

You don't want to be declaring your variables inside a loop.

---

The compiler will most certainly optimize this and move the redundant init outside of the loop. Since this is really just a sub esp,1 (BYTE) the compiler will do this outside of the loop.

Quote:

---

Theres no need to to initalize r, g, and b

---

It is a good practice to initialize your variables prior to usage. If SDL_GetRGB () failed for some reason you could guarantee that your variables would at least be 0 thus causing a black output. Without this you could end up with any mixture of r,g,b on an error condition since you would be using un-init vars.

Quote:

---

The compiler will most certainly optimize this and move the redundant init outside of the loop. Since this is really just a sub esp,1 (BYTE) the compiler will do this outside of the loop.

---

Good point. I had not thought of that.

Quote:

---

It is a good practice to initialize your variables prior to usage. If SDL_GetRGB () failed for some reason you could guarantee that your variables would at least be 0 thus causing a black output. Without this you could end up with any mixture of r,g,b on an error condition since you would be using un-init vars.

---

Yes, but initializing 3 variables say maybe 500,000 times each loop is going to create a reasonable amount of overhead. But then I guess converting to greyscale is not something thats likely to be performed very often. Then again I doubt SDL_GetRBG can fail as long as you are reading inside the surface boundaries.

Also heres a greyscale filter I found that I coded a while back. Incase anyone ever needs one :) It assumes 32 bit colour tho.

Code:

---

int Greyscale(SDL_Surface *image)
{
    char* pixels = NULL;
    unsigned bytes = image->w * image->h * 4;
    if(! (pixels = (char*)malloc( bytes ))) return 1;

    // copy image data to ram
    char *p1 = pixels;
    char *p2 = (char*)image->pixels;
    for(int i=0; i<bytes; i++, p1++, p2++) *p1 = *p2;
   
    // convert to greyscale
    p1 = pixels;
    p2 = pixels;
    unsigned col, a, b, size=bytes/4;
    for(int a=0; a<size; a++)
    {
        col = 0;
        for(b=0; b<3; b++, p1++) col = *p1; //read colours
        p1++; //skip alpha byte
        col /= 3;
        for(b=0; b<3; b++, p2++) *p2 = col;  //write greyscale
        p2++;
    }
   
    // copy greyscale to image
    p1 = pixels;
    p2 = (char*)image->pixels;
    for(int i=0; i<bytes; i++, p1++, p2++) *p2 = *p1;   

    free(pixels);
    return 0;
}

---

Just to let you know, your algorithm for greyscale doesn't work. it goes extremely dark.

Also, another question. Would it be faster to reload the image if your through with greyscale, create a copy surface, or just run greyscale each frame. The latter seems the most inefficient, but the other two seem to be about the same.

Quote:

---

Just to let you know, your algorithm for greyscale doesn't work. it goes extremely dark.

---

Youre wrong, it does work :P Thats the effect you get by dividing the colour by 3. If you want something brighter you could change the greyscale conversion to:

Code:

---

    for(int a=0; a<size; a++)
    {
        col = 0;
        for(b=0; b<3; b++, p1++) if(*p1 > col) col = *p1; //read colours
        p1++; //skip alpha byte
        for(b=0; b<3; b++, p2++) *p2 = col;  //write greyscale
        p2++;
    }

---

;)

Quote:

---

Also, another question. Would it be faster to reload the image if your through with greyscale, create a copy surface, or just run greyscale each frame. The latter seems the most inefficient, but the other two seem to be about the same.

---

Replace the image or create a greyscale copy of it. Converting every loop is going to hammer the cpu.

Edit: Alternatively if you just want to brighten the image but keep the same gradient add this:

Code:

---

float brighten = 100.0 / col * inc_percent;
if( (col+=brighten) >255 ) col = 255;

---

I hate to argue, but if i use your algorithm it makes my picture almost black. and the one i showed above doesnt.

Yeah actually i see what you mean now. My version originally picked the brightest element instead of dividing by 3, which worked fine. Tbh I can't see whats wrong with the changes I made and I cant be bothered to find out.

Edit: its probably because I never declared the chars in the function as unsigned.
Edit2: yep it is.