Reading a binary number from a file and saving it as an integer

Question

I am trying to write a converter from P6 to P3, which means from data saved in binary to data saved in ASCII.

This is the function I wrote to do the job but red,green and blue integers get garbage values instead of their decimal value.

int convertP6toP3(char* fileName)
{
    FILE *src, *dest;
    char *outputFilename;
    char magicNumber[3];
    int height, width, depth;
    int red = 0, green = 0, blue = 0;
    int i, j, widthCounter = 1;

    if (checkFileExists(fileName) == FALSE)
    {
        printf("- Given file does not exists!\n");
        return ERROR;
    }

    else
        src = fopen(fileName, "rb");

    outputFilename = getOutputFilename(fileName, ".p3.ppm");
    dest = fopen(outputFilename, "w+");

    // check that the input file is actually in P6 format
    fscanf(src, "%s", magicNumber);
    if (strcmp(magicNumber, "P6") != 0)
        return ERROR;
    fscanf(src, "%d %d %d", &height, &width, &depth);

    fprintf(dest, "P3\n");
    fprintf(dest, "#P3 converted from P6\n");
    fprintf(dest, "%d %d\n%d\n", height, width, depth);

    for (i = 0; i < width*height; i++)
    {
        for (j = 0; j < 3; j++)
        {
            fread(&red, sizeof(int), 1, src);
            fread(&green, sizeof(int), 1, src);
            fread(&blue, sizeof(int), 1, src);
        }

        for (j = 0;  j < 3; j++)
            fprintf(dest, "%d %d %d ", red, green, blue);

        if (widthCounter == width)
        {
            fprintf(dest, "\n");
            widthCounter = 1;
        }

        else
            widthCounter++;
    }

    free(outputFilename);
    fclose(src);
    fclose(dest);
    return TRUE;
}

What am I doing wrong with reading the binary data? The height, width and depth are read flawlessly.

EDIT: After trying to play with the read data size I get some crazy results which I can't explain:

Answer 1

Following on from our discussion in the comments thread for the question, I think the problem is in your reading of the binary data.

If each sample is one byte, you will want to use unsigned char for the red/blue/green variables and do fread(&var, 1, 1, src) I would think. I reckon sizeof(int) is likely 4 (system dependent) so for each of red, green, and blue, you've been reading in 4 unsigned bytes and storing them in a signed integer... this will lead to trouble.

Also, I don't think you need your inner loop for(j = ...

I would rewrite your inner loop as:

unsigned char red, green blue;
...
for (i = 0; i < width*height; i++)
{
    fread(&red, 1, 1, src); //<-- Note inner for(j... has been removed
    fread(&green, 1, 1, src);
    fread(&blue, 1, 1, src);

    fprintf(dest, "%u %u %u ", 
                  (unsigned int)red, 
                  (unsigned int)green, 
                  (unsigned int)blue);
    ...

Also, the spec I read at http://netpbm.sourceforge.net/doc/ppm.html indicates that the samples need not be one byte depending on the value of "... maximum color value (Maxval)...", which you seem to store in depth.

EDIT #1: The reason I took out your inner j loop...

for (i = 0; i < width*height; i++)
{
    for (j = 0; j < 3; j++)
    {
        fread(&red, sizeof(int), 1, src);
        fread(&green, sizeof(int), 1, src);
        fread(&blue, sizeof(int), 1, src);
    }

...was because this reads 9 bytes but only stores the value of the last 3 bytes read. This is because in the first loop iteration you read 3 bytes and store them, respectively, in the variables red, green and blue. Then the loop repeats and reads new data into these variables. Hence the last set of data is lost and so on...

Why putting this back in helped I don't know?! It implies that there are more bytes of pixel data available than I thought reading the standard. It says width*height pixels, at least that's how I read it, but with the for(j... loop you read 3*width*height.

Given what we've seen (see comments under question) it looks like without this loop you're not reading enough samples, but from the standard, I don't understand why (yet) :)

EDIT 2: What if you try....

unsigned char red, green blue;    
...
for (i = 0; i < width*height; i++)
{
    for (j = 0; j < 3; j++)
    {
        fread(&red, 1, 1, src);
        fread(&green, 1, 1, src);
        fread(&blue, 1, 1, src);

        fprintf(dest, "%u %u %u ", 
                      (unsigned int)red, 
                      (unsigned int)green, 
                      (unsigned int)blue);
        if (widthCounter == width)
        {
            fprintf(dest, "\n");
            widthCounter = 1;
        }
        else
            widthCounter++;

    }
}

Answer 2

OK, the problem is in fread. Youre reading from the file

sizeof(int) 

Depending on your compiler that value can be different (usually four bytes). In P6 format each color is stored in one byte (0 - 255). so when you're reading your values you're going out of bounds for each pixel.

try:

        fread(&red, sizeof(char), 1, src);
        fread(&green, sizeof(char), 1, src);
        fread(&blue, sizeof(char), 1, src);

and cast them later if you need. That should work.

EDIT: You should also delcare your rgb variables as unsigned ints

Heres some info about the P3/P6 file formats: http://paulbourke.net/dataformats/ppm/