Scaling a raw 24 bit pixelmap in C

Question

I am trying to scale down the size of an X display that I have captured as to display it in a new Window. I tried averaging the individual RGB components but the image doesn't look too nice. Here is my algorithm:

  img = XGetImage(d_remote,RootWindow(d_remote,0),0,0,attr.width,attr.height,XAllPlanes(),ZPixmap);

   int i;
   int j;
   for(i=0;i<attr.height;i=i+2){
        for(j=0;j<attr.width;j=j+2) {
                unsigned long p1 = XGetPixel(img, j, i);
                unsigned long p1R = p1 & 0x00ff0000;
                unsigned long p1G = p1 & 0x0000ff00;
                unsigned long p1B = p1 & 0x000000ff;

                unsigned long p2 = XGetPixel(img, j+1, i);
                unsigned long p2R = p2 & 0x00ff0000;
                unsigned long p2G = p2 & 0x0000ff00;
                unsigned long p2B = p2 & 0x000000ff;

                unsigned long p3 = XGetPixel(img, j, i+1);
                unsigned long p3R = p3 & 0x00ff0000;
                unsigned long p3G = p3 & 0x0000ff00;
                unsigned long p3B = p3 & 0x000000ff;

                unsigned long p4 = XGetPixel(img, j+1, i+1);
                unsigned long p4R = p4 & 0x00ff0000;
                unsigned long p4G = p4 & 0x0000ff00;
                unsigned long p4B = p4 & 0x000000ff;

                unsigned long averageR = (p1R+p2R+p3R+p4R)/4;
                unsigned long averageG = (p1G+p2G+p3G+p4G)/4;
                unsigned long averageB = (p1B+p2B+p3B+p4B)/4;

                int average = averageR | averageG | averageB | 0x00000000;

                XPutPixel(newImg, j/2, i/2, average);

        }
   }

Is averaging pixels a bad way to do this? I'm basically trying to scale img by 50%. My assumption is that I move a 2x2 square across the pixelmap.

Answer 1

In doing so:

unsigned long averageR = (p1R+p2R+p3R+p4R)/4;
unsigned long averageG = (p1G+p2G+p3G+p4G)/4;
unsigned long averageB = (p1B+p2B+p3B+p4B)/4;

int average = averageR | averageG | averageB | 0x00000000;

You are "polluting" your colors: if the sum of (p1R+p2R+p3R+p4R) is not divisible by 4 you will have bits entering the G field (in the most significant bit!). Same for (p1G+p2G+p3G+p4G) and the B field. You should do:

unsigned long averageR = (p1R+p2R+p3R+p4R)/4  & 0x00ff0000;
unsigned long averageG = (p1G+p2G+p3G+p4G)/4  & 0x0000ff00;
unsigned long averageB = (p1B+p2B+p3B+p4B)/4  & 0x000000ff;

int average = averageR | averageG | averageB;

The last `0x00000000 is not necessary, it does absolutely nothing :)

(I also double checked operator precedence, it should be ok)