I am doing a project that involves writing functions to read/write and encode/decode PGM files. Im using a structure with a function to read in the PGM file. I am quite new to structures and how they are syntaxed so I am just wondering if this bit of code would correctly read in the scanned data into my structure.
Here is my code (C):
#include <stdio.h>
#include "image.h"
int **allocatePGM(int numCols, int numRows){
int ** = malloc(sizeof(int *) * numRows);
for (int i=0; i<numRows; i++)
pixels[i] = malloc(sizeof(int) * numCols);
return pixels;
}
ImagePGM *readPGM(char *filename, ImagePGM *pImagePGM){
FILE *inFile = NULL
char PGMcheck[5];
int max_value = 0;
unsigned int width = 0, height = 0;
unsigned int i = 0;
int pixeldata = 0;
inFile = fopen(filename, "r");
if (inFile == NULL)
printf("File could not be opened\n");
exit(1);
fgets(PGMcheck, sizeof(PGMcheck), inFile);
if (strcmp(version, "P5")) {
fprintf(stderr, "Wrong file type!\n");
exit(1);
}
printf("This file does not contain the PGM indicator \"P2\"");
exit(1);
}
fscanf(inFile, "%d", &width);
fscanf(inFile, "%d", &height);
fscanf(inFile, "%d", max_value);
struct ImagePGM.pImagePGM
pImagePGM.magic = PGMcheck;
pImagePGM.width = width;
pImagePGM.height = height;
pImagePGM.max_value = max_value;
pImagePGM->pixels = allocatePGM(pImagePGM->width, pImagePGM->height);
if (pImagePGM->max_value > 255) {
for (i = 0; i < height; ++i) {
for (j = 0; j < width; ++j) {
pImagePGM->pixels[i][j];
}
}
}
return pImagePGM;
}
My header file contains the structure as follows...
typedef struct _imagePGM {
char magic[3]; // magic identifier, "P2" for PGM
int width; // number of columns
int height; // number of rows
int max_value; // maximum grayscale intensity
int **pixels; // the actual grayscale pixel data, a 2D array
} ImagePGM;
Seem okay to you guys?
Continuing from my earlier comment, you have several problems that have to do with handling the Plain PGM File Format that will prevent your successful read of the file.
First, fgets(PGMcheck, sizeof(PGMcheck), inFile); is not guaranteed to read PGMcheck correctly. The magic-number may be followed by "(blanks, TABs, CRs, LFs)" so fgets will read more than just the magic-number unless it is followed by a single '\n' -- not guaranteed by the format. While fgets() is generally the correct way to do line-oriented input, the PGM format isn't guaranteed to be formatted in lines, so you are left using a formatted-input function, or a character-by-character approach.
(you can use fgets(), but that would require parsing the resulting buffer and saving whatever part of the buffer extended beyond the magic-number to include as the beginning part of your next read)
You have corrected your attempt at the string comparison using != instead of strcmp, but you still must compare the magic-number with "P2" for reading a Plain-PGM format file (as your question included originally) Go ahead and read the magic-number into a string, but use a formatted-input function (fscanf) to read only up until the first whitespace is encountered, regardless of what that whitespace is.
Lastly, there is no need to store the magic-number as part of the plain_pgm struct. That is something your validate before you attempt to fill the struct. It either is "P2" or it isn't -- no need to store it.
For portability, it is a good idea to use exact-width types for storage when reading image files. There are a number of benefits, but the primary being, your program will behave correctly whether run on your x86_64 or on your TI-MSP432 chip. The exact width types are defined in stdint.h and the macros from printing and reading the exact width types are provided in inttypes.h. Instead of char you have int8_t, instead of unsigned char you have uint8_t, and so on, where the numeric value specifies the exact number of bytes for the type.
With that your pgm struct could look like:
typedef struct { /* struct for plain pgm image */
uint32_t w, h; /* use exact width types for portable code */
uint16_t max; /* 16-bit max */
uint16_t **pixels; /* pointer-to-pointer for pixel values */
} plain_pgm;
Your allocation is largely correct, but rearranging to return a pointer-to-pointer-to uint16_t (adequate for the maximum gray value pixel values), you could do:
uint16_t **alloc_pgm_pixels (uint32_t w, uint32_t h)
{
uint16_t **pixels = NULL;
/* allocate/validate height number of pointers */
if (!(pixels = malloc (h * sizeof *pixels))) {
perror ("malloc-pixels");
return NULL;
}
/* allocate/validate width number of values per-pointer */
for (uint32_t i = 0; i < h; i++)
if (!(pixels[i] = malloc (w * sizeof *pixels[i]))) {
perror ("malloc-pixels[i]");
return NULL;
}
return pixels; /* return allocated pointers & storage */
}
Your read function needs lots of help. First, you generally want to open and validate the file is open for reading in the calling function and pass the open FILE * pointer as a parameter to your read function rather than the filename. (if the file cannot be opened in the caller, there is no need to make the function call to begin with). With that change and passing a pointer to your struct, your read function could look like:
int read_pgm (FILE *fp, plain_pgm *pgm)
{
char buf[RDBUF]; /* buffer for magic number */
uint32_t h = 0, w = 0; /* height/width counters */
if (fscanf (fp, "%s", buf) != 1) { /* read magic number */
fputs ("error: invalid format - magic\n", stderr);
return 0;
}
if (strcmp (buf, MAGIC_PLN) != 0) { /* validate magic number */
fprintf (stderr, "error: invalid magic number '%s'.\n", buf);
return 0;
}
/* read pgm width, height, max gray value */
if (fscanf (fp, "%" SCNu32 " %" SCNu32 " %" SCNu16,
&pgm->w, &pgm->h, &pgm->max) != 3) {
fputs ("error: invalid format, h, w, max or included comments.\n",
stderr);
return 0;
}
/* validate allocation of pointers and storage for pixel values */
if (!(pgm->pixels = alloc_pgm_pixels (pgm->w, pgm->h)))
return 0;
for (;;) { /* loop continually until image read */
if (fscanf (fp, "%" SCNu16, &pgm->pixels[h][w]) != 1) {
fputs ("error: stream error or short-read.\n", stderr);
return 0;
}
if (++w == pgm->w)
w = 0, h++;
if (h == pgm->h)
break;
}
return 1;
}
(note: this read function does NOT consider comment lines, implementing ignoring of comment lines is left to you. You could either make an additional call to fscanf before and between reading each part of the magic-number, width, height, and max gray value with something similar to " # %[^\n']" to skip any number of whitespace and read up to and including the next '#' character and to the end of line, or just use fgetc in a loop searching for the next non-whitespace character and checking if it is a '#' and if not use ungetc, if so, clear to the end of line.)
Putting it altogether in an example, you could do:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>
#define RDBUF 32 /* if you need a constant, #define one (or more) */
#define MAGIC_PLN "P2"
typedef struct { /* struct for plain pgm image */
uint32_t w, h; /* use exact width types for portable code */
uint16_t max; /* 16-bit max */
uint16_t **pixels; /* pointer-to-pointer for pixel values */
} plain_pgm;
uint16_t **alloc_pgm_pixels (uint32_t w, uint32_t h)
{
uint16_t **pixels = NULL;
/* allocate/validate height number of pointers */
if (!(pixels = malloc (h * sizeof *pixels))) {
perror ("malloc-pixels");
return NULL;
}
/* allocate/validate width number of values per-pointer */
for (uint32_t i = 0; i < h; i++)
if (!(pixels[i] = malloc (w * sizeof *pixels[i]))) {
perror ("malloc-pixels[i]");
return NULL;
}
return pixels; /* return allocated pointers & storage */
}
int read_pgm (FILE *fp, plain_pgm *pgm)
{
char buf[RDBUF]; /* buffer for magic number */
uint32_t h = 0, w = 0; /* height/width counters */
if (fscanf (fp, "%s", buf) != 1) { /* read magic number */
fputs ("error: invalid format - magic\n", stderr);
return 0;
}
if (strcmp (buf, MAGIC_PLN) != 0) { /* validate magic number */
fprintf (stderr, "error: invalid magic number '%s'.\n", buf);
return 0;
}
/* read pgm width, height, max gray value */
if (fscanf (fp, "%" SCNu32 " %" SCNu32 " %" SCNu16,
&pgm->w, &pgm->h, &pgm->max) != 3) {
fputs ("error: invalid format, h, w, max or included comments.\n",
stderr);
return 0;
}
/* validate allocation of pointers and storage for pixel values */
if (!(pgm->pixels = alloc_pgm_pixels (pgm->w, pgm->h)))
return 0;
for (;;) { /* loop continually until image read */
if (fscanf (fp, "%" SCNu16, &pgm->pixels[h][w]) != 1) {
fputs ("error: stream error or short-read.\n", stderr);
return 0;
}
if (++w == pgm->w)
w = 0, h++;
if (h == pgm->h)
break;
}
return 1;
}
int main (int argc, char **argv) {
plain_pgm pgm = { .w = 0 }; /* plain_pgm struct instance */
/* use filename provided as 1st argument (stdin by default) */
FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;
if (!fp) { /* validate file open for reading */
perror ("file open failed");
return 1;
}
if (!read_pgm (fp, &pgm)) { /* validate/allocate/read pgm file */
fputs ("error: read_pgm failed.\n", stderr);
return 1;
}
if (fp != stdin) /* close file if not stdin */
fclose (fp);
/* output success */
printf ("successful read of '%s'\n%" PRIu32 "x%" PRIu32 " pixel values.\n",
argc > 1 ? argv[1] : "stdin", pgm.w, pgm.h);
for (uint32_t i = 0; i < pgm.h; i++) /* free pixel storage */
free (pgm.pixels[i]);
free (pgm.pixels); /* free pointers */
}
Example Use/Output
Using an example apollonian_gasket.ascii.pgm, a 600 wide by 600 high image of an Apollonian gasket file as a test file, you would get:
$ ./bin/read_pgm_plain dat/apollonian_gasket.ascii.pgm
successful read of 'dat/apollonian_gasket.ascii.pgm'
600x600 pixel values.
Memory Use/Error Check
In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.
It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.
For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.
$ valgrind ./bin/read_pgm_plain dat/apollonian_gasket.ascii.pgm
==8086== Memcheck, a memory error detector
==8086== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8086== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==8086== Command: ./bin/read_pgm_plain dat/apollonian_gasket.ascii.pgm
==8086==
successful read of 'dat/apollonian_gasket.ascii.pgm'
600x600 pixel values.
==8086==
==8086== HEAP SUMMARY:
==8086== in use at exit: 0 bytes in 0 blocks
==8086== total heap usage: 604 allocs, 604 frees, 730,472 bytes allocated
==8086==
==8086== All heap blocks were freed -- no leaks are possible
==8086==
==8086== For counts of detected and suppressed errors, rerun with: -v
==8086== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Always confirm that you have freed all memory you have allocated and that there are no memory errors.
Look over the changes made and if you don't understand why something wast done, drop a comment asking and I'm happy to help further.