Issues writing PNM P6

Question

I'm writing a program that takes in two duplicate PNM P6 files, puts the memory of the first file into a buffer, creates a yellow diagonal line over that, and writes the result to the second file. When I run it, the output file is corrupted and can't be displayed. I noticed when looking at the output that it's missing the three lines that should be at the top:

P6
1786 1344
255

I don't know how to programmatically ensure that those lines stay in the code -- I can't figure out why they're even being overwritten in the first place.

When I manually add those lines to the output, the file is no longer corrupted. However, no yellow diagonal line appears. Is this part of the same issue, or is there something else I should look into fixing?

My code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct
{
    unsigned char red, green, blue;
} iPixel;

typedef struct
{
    int x, y;
    iPixel *data;

} Image;

Image *
ReadImage(char *filename)
{
    FILE *f = fopen(filename, "rb");
    Image *img = NULL;
    char magicNum[128];
    int  width, height, maxval;

    if (f == NULL)
    {
        fprintf(stderr, "Unable to open file %s\n", filename);
        return NULL;
    }

    fscanf(f, "%s\n%d %d\n%d\n", magicNum, &width, &height, &maxval);
/*
    printf("Magic num = %s width = %d, height = %d, maxval = %d\n",
            magicNum, width, height, maxval)
; 
*/
    if (strcmp(magicNum, "P6") != 0)
    {
        fprintf(stderr, "Unable to read from file %s, because it is not a PNM file of type P6\n", filename);
        return NULL;
    }

    img = (Image *) malloc(sizeof(Image));
    img -> x = width;
    img -> y = height;
    img -> data = (iPixel*) malloc(img -> x * img -> y * sizeof(iPixel));

    fread(img -> data, sizeof(iPixel), width*height, f);

    fclose(f);
    return img;

}

void WriteImage(Image *img, char *filename)
{
    FILE *f = fopen(filename, "wb");

    fwrite(img->data, sizeof(iPixel), img-> x * img-> y, f);
    fclose(f);
}

Image *
YellowDiagonal(Image *input)
{

    int i, j;

    for (i = 0; i < input->x; i++)
    {
        for (j=0; j < input->y; j++)
        {
            if (i==j)
            {
                input->data[i].red=255;
                input->data[i].green=255;
                input->data[i].blue=0;
            }
        }
    }

    return input;   
}

int main(int argc, char *argv[])
{
    if (argc != 3)
    {
        fprintf(stderr, "Usage: ./3A_c.c <input image file> <output image file>\n");
        exit(EXIT_FAILURE);
    }

        Image *img;

        img = ReadImage(argv[1]);
        fprintf(stderr, "Read.\n");
        YellowDiagonal(img);
        fprintf(stderr, "Diagonal line.\n");
        WriteImage(img, argv[2]);
        fprintf(stderr, "Write.\n");
}

Answer 1

Be careful to write exactly the same format you are reading. The PNM format is well defined, and you are reading it correctly. However, in the writing routine there were a couple of mistakes:

1. opening a file with "w" or "wb" truncates it to 0 bytes;
2. best practice is always to check if fopen succeeds;
3. reading actual ASCII data can be done with fscanf, binary data with fread. Similarly, writing ASCII should be done with fprintf and only binary data again with fwrite.
4. If you want to make sure you write the same data as you read in before, you need to save it somewhere. The maxval variable is read, but not saved, and so I cannot write it back. However, it is not a huge problem because the rest of your code assumes the image is R8G8B8 anyway, and so maxval should always be 255.

Here is an adjusted WriteImage that works.

void WriteImage(Image *img, char *filename)
{
    FILE *f = fopen(filename, "wb");

    if (f == NULL)
    {
        printf ("Unable to open '%s' for writing!\n", filename);
     /* better would be: "return -1" to indicate an error, 0 otherwise */
        return;
    }

    fprintf (f, "P6\n");
    fprintf (f, "%d %d\n", img->x, img->y);
/*  actually you need to write 'maxval' here */
    fprintf (f, "%d\n", 255);

    fwrite(img->data, sizeof(iPixel), img->x * img->y, f);
    fclose(f);
}

---

With the above out of the way, you can now see your 'diagonal line' is not correct! I'm not going to fix that (I suppose not being able to see what happened stopped you in your tracks), but here are a few pointers just to guide you:

• no need to return an Image * if you are changing data in situ anyway
• no need to check every single pixel
• check the coordinates of what pixels are changed ...