Breadth-first search for clusters of pixels in a given color range

I am a beginner in programming languages, so I apologise if my code is badly formatted or doesn't make any sense.

My program gets an image and a RGB color range as input and it counts how many pixels are in each group. A group only contains pixels connected directly, not diagonally, so for each pixel, it searches for valid pixels (in the RGB range) in all 4 directions, and if it finds one it adds it to a queue.

My code has the output I want, but I need it to be about 10% faster. Do you have any ideas of improvements I could make?

```
#include
#include
#include "bmp_header.h"
typedef struct
{
unsigned char B;
unsigned char G;
unsigned char R;
} Pixel;
int main()
{
int i,j,k;
int *vpLoc; //Array of location of the valid pixels
int Rr,Gr,Br,Ro,Go,Bo,Rv,Gv,Bv;
int count;
float P;
int *queue=0;//I will use this for my breadth-first search algorithm
int temp[4]={0};

FILE *input=fopen("input.txt", "rt");
fscanf(input,"%d %d %d %d %d %d %f",&Rr,&Gr,&Br,&Ro,&Go,&Bo,&P);

FILE *in=fopen("input.bmp","rb");
if(in==NULL)
{
printf("\nCannot open file\n");
exit(1);
}

struct bmp_fileheader Header;
struct bmp_infoheader InfoHeader;

fread(&Header, sizeof(Header), 1, in);
fread(&InfoHeader, sizeof(InfoHeader), 1, in);

Pixel *pArray;
pArray=(Pixel)calloc(InfoHeader.heightInfoHeader.width,sizeof(Pixel*));

//Read the pixel's info
int nrvp=0; //This will be the number of valid pixels
vpLoc=(int)calloc(InfoHeader.heightInfoHeader.width,sizeof(int));
for(i=0;i=Rv&&Gr-Go=Gv&&Br-Bo=Bv)
vpLoc[nrvp++]=i;

}
fclose(in);
free(pArray);
//To count the clusters, I will use a breadth-first search algorithm(which I am proud of)
queue=(int*)calloc(nrvp,sizeof(int));
int carry;
int buffer=0;
int tmp;
int keeper=0; //This will help reduce the range of for functions
int OK=1;
int cluster[100];//Here I will write the number of pixels in each cluster

for(i=0;iInfoHeader.width)
for(j=i;j>=0;j--)
if(vpLoc[i]-InfoHeader.width==vpLoc[j])
{
//If

Just some generic remarks as I don't have the time at the moment to look at the performance:

-
You said that you are a beginner and apologize for your bad formatting. However there is no real excuse to write badly formatted code. Not sure what editor or IDE you use but if you use something else but notepad most editors and especially IDEs support auto-indenting and can re-format your code. Make use of it. It significantly increases readability which has a direct connection to how easy it is to spot bugs.

Good formatting means: Proper indenting and to have spaces between operators and things like commas and semicolons. For example this:

for(i=0;i<abs(InfoHeader.height)*abs(InfoHeader.width);i++)

will read better when formatted like this:

for (i = 0; i < abs(InfoHeader.height) * abs(InfoHeader.width); i++)

-
You don't need to cast the return value of calloc in C. In fact it's considered bad practice as it can hide problems (like forgotten to include the appropriate header).

-
You should check the return value of calloc - in case it fails it will be NULL and you are usually left with a somewhat meaningless segfault when trying to access the memory.

-
You do calloc(InfoHeader.height*InfoHeader.width,sizeof(int)) but then immediately after in your for loop you do

for(i=0;i<abs(InfoHeader.height)*abs(InfoHeader.width);i++)

Why do you use abs in the for loop? If you think that one of them could be negative then you are already in trouble with calloc.

-
You only check the return value of the second fopen and not the first one - you should check both. You should also close the first file descriptor if the second fopen fails. Even though the open file descriptor will be closed when the process exits it's still a lot nicer to do it properly.

-
Unless I missed it you close in but never input.

-
Your comparison for valid pixel is duplicating code and could use refactoring into a single method like this:

int ColorInRange(int r, int o, int value)
{
    return r - o = value;
}

Not sure what the r and o stand for but they should be given proper names reflecting their meaning.

-
When you read the bitmap you allocate an array to hold each pixel. Yet you only read one pixel at a time and you also only need it to determine whether or not to add it to vploc. This is a waste of memory You only need to have a single Pixel structure to store the temporary data in (and you should check the return value of fread):

for (i = 0; i < abs(InfoHeader.height) * abs(InfoHeader.width); i++)
{   
    Pixel p;
    if (fread(&p, sizeof(p), 1, in) < 1)
    {
        // read failed
        fprintf(stderr, "Bitmap read error at position %d\n", i);
        exit(1);
    }

    //Check if the pixel is valid. If it is, add it's location to vpLoc
    if (ColorInRange(Rr, Ro, p.R) && ColorInRange(Gr, Go, p.G) && ColorInRange(Br, Bo, Bv))
    {
        vpLoc[nrvp++] = i; 
    }
}

If you want to make it faster instead you could try reading the entire file into memory in one go and then calculate the vpLoc. Something along these lines:

int nrvp = 0; //This will be the number of valid pixels
int totalPixels = InfoHeader.height * InfoHeader.width;
vpLoc = calloc(totalPixels, sizeof(int));
if (fread(pArray, sizeof(Pixel), totalPixels, in) = Rv && Gr - Go = Gv && Br - Bo = Bv)
        vpLoc[nrvp++]=i; 
}

-
You should in general try to find better names for variables - vpLoc and and nrvp are not very descriptive.

-
You should check the return value of fopen before writing the output.

StackExchange Code Review Q#77906, answer score: 4