C not entering For Loop which merge 2 PPM images

Question

I have gone absolutely mind numb trying merge two PPM images of the same type and size together. My code can read, display and save PPM images but won't enter the for loop which combine two images. Here's the combining function:

    struct MFI AddImages (struct MFI AddImgOne, struct MFI AddImgTwo)
    
    {
        struct MFI AddImgThree = {'P6', AddImgOne.ImgHeight, AddImgOne.ImgWidth, AddImgOne.MaxInfo, AddImgOne.ImageSize};
        int CombineCounter;
        
        if(AddImgOne.ImgWidth != AddImgTwo.ImgWidth || AddImgOne.ImgHeight != AddImgTwo.ImgHeight)
        {
            printf("They're not the same size you dummy!");
            exit(0);
        }

        AddImgThree.ImageSize = AddImgOne.ImgHeight * AddImgTwo.ImgWidth;
    
        AddImgThree.ImageMemory = malloc(AddImgThree.ImageSize * sizeof(unsigned long long));

        if (!AddImgThree.ImageMemory)
            {printf("theres no space!\n");
            exit(0);}     
    
        if (AddImgThree.ImageMemory) 
        {
            for (CombineCounter = 1; CombineCounter < AddImgOne.ImageSize; CombineCounter++)
                {
                AddImgThree.RGB[CombineCounter].red = AddImgOne.RGB[CombineCounter].red + AddImgTwo.RGB[CombineCounter].red;
                AddImgThree.RGB[CombineCounter].blue = AddImgOne.RGB[CombineCounter].blue + AddImgTwo.RGB[CombineCounter].blue;
                AddImgThree.RGB[CombineCounter].green = AddImgOne.RGB[CombineCounter].green + AddImgTwo.RGB[CombineCounter].green;

                if (AddImgThree.RGB[CombineCounter].red > 255)
                    {AddImgThree.RGB[CombineCounter].red = 255;}
            
                if (AddImgThree.RGB[CombineCounter].blue > 255)
                    {AddImgThree.RGB[CombineCounter].blue = 255;}
    
                if (AddImgThree.RGB[CombineCounter].green > 255)
                    {AddImgThree.RGB[CombineCounter].green = 255;}
                }
        }
        else
            printf ("it's not right\n");

    
        free(AddImgThree.ImageMemory);
        return AddImgThree;
    }

For context heres my main function with the structures too:

#include <stdio.h> /*printf, scanf, seekf*/
#include <string.h> /*strstr, strcmp, strcat*/
#include <stdlib.h> /*malloc free command*/


struct MFI {
        char Format[3];
        int ImgHeight, ImgWidth, MaxInfo, ImageSize;
        int *ImageMemory;
        struct RGBPixel *RGB;
        };

struct RGBPixel {
    int red, green, blue;
};

struct MFI Reading (char filename[128]);
struct MFI AddImages (struct MFI AddImgOne, struct MFI AddImgTwo);
void Display (struct MFI D);
void Save(struct MFI S);

int main()
{
    struct MFI ImgOne;
    struct MFI ImgTwo;
    struct MFI ImgThree;
    char Image1[128];
    char Image2[128];


    printf("please enter the name of image 1:");
    scanf(" %s", Image1);
    fseek(stdin,0,SEEK_END);

    printf("please enter the name of image 2:");
    scanf(" %s", Image2);
    fseek(stdin,0,SEEK_END);

    ImgOne = Reading(Image1);
    ImgTwo = Reading(Image2);

    Display(ImgOne);
    Display(ImgTwo);

    ImgThree = AddImages(ImgOne, ImgTwo);

    Save(ImgThree);

    printf("\nfile is closed\n");

    free(ImgOne.ImageMemory);
    free(ImgTwo.ImageMemory);
    free(ImgThree.ImageMemory);
    printf("\nimage freed\n");

    return 0;
}

And my reading function:

struct MFI Reading (char filename[128])
{

    struct MFI R;
    FILE *ImageRead; // file pointer or handle
    int p; 

    if(strstr(filename, ".ppm") == NULL)
        {strcat(filename, ".ppm");}
  
    ImageRead = fopen(filename, "rb");
  
    //check if it exists
    if (ImageRead == NULL) {
    printf("Nope doesn't exist\n");
    exit(0);}
    
    //get all the image information
    fscanf(ImageRead, "%s %d %d %d", R.Format, &R.ImgWidth, &R.ImgHeight, &R.MaxInfo);

    //allocate memory for image//
    R.ImageMemory = malloc(R.ImgHeight * R.ImgWidth * sizeof(int*));
    if (!R.ImageMemory)
        {printf("theres no space!\n");
        exit(0);}

    //check if its a PPM
    if (!strcmp(R.Format, "P3"))
        printf("This is a ppm\n");
    else
        {
        if (!strcmp (R.Format, "P2"))
        printf("This is a pgm\n");
        else
            {
            if (!strcmp (R.Format, "P5"))
            printf("This is a pgm\n");
            else 
                {printf("this isn't a ppm file");}
            }
        }

    p = getc(ImageRead);
    while (p == '#')
    {
        while (getc(ImageRead) != '\n')
        {
        p = getc(ImageRead);
        }
    }
    ungetc(p, ImageRead);

    //check image size
    if(R.ImgHeight > 1080 || R.ImgWidth > 1920)
        {printf ("This is too big!!");}

    R.ImageSize = R.ImgWidth*R.ImgHeight;

    while (fgetc(ImageRead) != '\n');
    R.RGB = malloc(3*R.ImageSize * sizeof(R.RGB));

    if (fread(R.RGB, 3*R.ImgHeight, 4*R.ImgWidth-3, ImageRead) != EOF)
        printf("error with loading image");

    //reading image width and height
    printf("The height of the image is %d and the width is %d\n", R.ImgWidth, R.ImgHeight);
    printf("The maximum value of each pixel is %d\n", R.MaxInfo);

    

    return R;
}

And my saving function:

void Save(struct MFI S)

{   FILE *SavePointer;
    char InputFileName[128];
    
    printf("\nPlease Enter the Name of the file you want to Store this Array In: \n");
    scanf("%s", InputFileName);

    SavePointer = fopen(InputFileName, "r");


    if(SavePointer != NULL)
    {
        printf("File exits, try another name");
        fclose(SavePointer);
        exit(0);
    }
    else   
    {
    fclose(SavePointer);
    printf("File is good to go!");
    }
    
    if(strstr(InputFileName, ".ppm") == NULL)
        {strcat(InputFileName, ".ppm");}

    SavePointer = fopen(InputFileName, "wb");
    if (!SavePointer)
    {
        printf ("Lost your File pointer!");
        exit (0);
    }

    fprintf(SavePointer, "%s\n%d %d\n%d\n", S.Format, S.ImgWidth, S.ImgHeight, S.MaxInfo);
    fwrite(S.RGB, 3*S.ImgHeight, 4*S.ImgWidth-3, SavePointer);

    fclose(SavePointer);
    return;
}

EDIT

I've rewrote the final part of the function so now the program will enter the for loop but will stop when assigning the array element value...

for (CombineCounter = 0; CombineCounter < AddImgOne.ImageSize; CombineCounter++)
        {
        unsigned int red = AddImgOne.RGB[CombineCounter].red + AddImgTwo.RGB[CombineCounter].red;
        
        if (red > 255)
            {red = 255;}
        AddImgThree.RGB[CombineCounter].red = red; //exits entire code when here
        
        
        unsigned int blue = AddImgOne.RGB[CombineCounter].blue + AddImgTwo.RGB[CombineCounter].blue;
        if (blue > 255)
            {blue = 255;}
        AddImgThree.RGB[CombineCounter].blue = blue;
        

        unsigned int green = AddImgOne.RGB[CombineCounter].green + AddImgTwo.RGB[CombineCounter].green;
        if (green > 255)
        {green = 255;}
        AddImgThree.RGB[CombineCounter].green =  green;

        }

*won't enter the for loop*. So debug the code. The best way to do that is to use a debugger. For example, which exact condition/variable value prevents it from entering the `for` loop? That is the first thing to find out and is easily found with a debugger or even basic debug print statements. — kaylum, Dec 26 '20 at 21:33
What is `fseek(stdin,0,SEEK_END);` supposed to do? If `stdin` isn't seekable - like a console window - it does nothing. And if it **is** seekable - like being redirected from a file - that line will mean the next input will fail and set EOF. And you're not checking your input `scanf()` calls for failure. — Andrew Henle, Dec 26 '20 at 21:35
`free(AddImgThree.ImageMemory); return AddImgThree;` looks wrong, why do you `free` the memory before returning it? Your `for` loop starts with index 1, what happens to index 0? Your pointer is an `int*`, but in the `malloc` you are using `sizeof(unsigned long long)`, which one is the correct one? You never initialize the member `RGB`, it points nowhere, but you use it. — mch, Dec 26 '20 at 21:50
In `AddImages`, you do: `if (!AddImgThree.ImageMemory)` and call `exit`. Below that you do: `if (AddImgThree.ImageMemory)` and the add code. The 2nd `if` is superfluous. Also, your `for` should probably start at 0 (_not_ 1). — Craig Estey, Dec 26 '20 at 21:51
Your saturation logic is done "too late" [because the RGB pixel probably uses `unsigned char`]. You want (e.g): `unsigned int red = AddImgOne.RGB[CombineCounter].red + AddImgTwo.RGB[CombineCounter].red; if (red > 255) red = 255; AddImgThree.RGB[CombineCounter].red = red;` Do the same for green and blue. — Craig Estey, Dec 26 '20 at 21:56
@CraigEstey I've corrected everything you've said but still can't work out why its not going past the '''AddImgThree.RGB[CombineCounter].red = red;''' stage — Eve Pitt, Dec 27 '20 at 17:10
A lot of the potential issues may be in the code that you've _not_ posted: `Reading`, `Display`, `Save` (so please post these). You assume `P3` format, but most `.ppm` files are `P6`. You're doing a `free` on `ImageMemory` so that implies that has the pixel data. But, you're _operating_ on `RGB`, so we need to see how the buffers are allocated and how the pixel data is read in. Your `RGBPixel` uses `int red, green, blue;` but most `.ppm` files use `unsigned char red, green, blue;`. This _may_ be fine, _if_ your `Reading` and `Save` handle the size differences. But, it's suspect. — Craig Estey, Dec 28 '20 at 00:15
I have working `.ppm` code, so if you can post your remaining code, I can help you with yours. I would want to fix _your_ code, rather than just posting a solution that looks "alien" to you. Upon looking at `AddImages` again, you're allocating (and freeing) `ImageMemory` but nothing uses it [so you can probably eliminate it]. The real place for the data is in `RGB`, so `Reading` would have to get the geometry, do the allocation, and read in the data. It's also the place to _set_ `ImageSize`, etc. `Reading` would _have_ to set this, to know how much to `malloc` for `RGB` in the struct. — Craig Estey, Dec 28 '20 at 00:22
@CraigEstey So the reading function does do the main heavy lifting here (I've added it above), because it's needed for all the functions I'm adding later on. — Eve Pitt, Dec 28 '20 at 13:08

Craig Estey · Answer 1 · 2020-12-29T08:06:14.387

With the adjusted clipping, AddImages was pretty close.

But, there were a number of issues elsewhere in the code.

In Reading, we need to allocate RGB (not ImageMemory). As I mentioned in my top comments, ImageMemory is superfluous.

We want fgetc and not getc.

Reading checks for valid formats, but it only handles the P6 [binary] format.

The P6 format can have pixels larger than a byte [if MaxInfo is 65535 instead of 255]. But, Reading tries to do a single fread for the pixel data, so we can only handle the 255 case.

But, [again] as I mentioned in my top comments, these are byte (i.e. unsigned char) values. So, each element of RGBPixel needs to be unsigned char and not int.

As you had it (e.g.) the RGB values from the file for pixel 0 and the R value for pixel 1 were being stored in the [4 byte] red element of the the first RGB array element.

fread returns a count and not EOF.

The fread call [and the fwrite in Save] does not transfer the correct number of bytes.

Not all calls are checked for error returns. And, if they do error, it helps to print the reason why (e.g. strerror(errno)).

Don't do fseek on stdin.

While it is valid to pass a struct to a function "by value", it's much more common to pass a struct around "by pointer". When passed by value, the entire contents of the struct must be pushed onto the stack. And, if returning a struct by value, the caller must reserve space for the return value in the stack frame.

In the current case, this isn't too bad. But, it doesn't scale too well. If we had (e.g.) a struct with a lot of data, it could be very slow and could cause a stack overflow:

struct bigstruct {
    int x;
    int y;
    int array[10000000];
};

Stylistically, it's good practice to keep argument names and stack variable names short.

Here's the refactored code. I've added some annotations. Where possible, I've bracketed old vs. new code with cpp conditionals:

#if 0
// old code
#else
// new code
#endif

I also added typedef statements for the structs. And, I converted the functions to use pointers to the structs.

Although I tried to keep as much of your code as possible, I had to refactor it a bit.

As I mentioned, AddImages was in good shape, but I refactored it a bit to be simpler and illustrate the use of pointers to pixels.

I tested it with some P6 file images and it appears to work.

So, here's the code:

#include <stdio.h>                      /* printf, scanf, seekf */
#include <string.h>                     /* strstr, strcmp, strcat */
#include <stdlib.h>                     /* malloc free command */
#include <errno.h>
#include <sys/stat.h>

#define fault(_fmt...) \
    do { \
        printf(_fmt); \
        exit(1); \
    } while (0)

typedef struct RGBPixel {
#if 0
    int red;
    int green;
    int blue;
#else
    unsigned char red;
    unsigned char green;
    unsigned char blue;
#endif
} RGBPixel;

typedef struct MFI {
    char Format[3];
    int ImgHeight;
    int ImgWidth;
    int MaxInfo;
    int ImgSize;
    RGBPixel *RGB;
} MFI;

void
Display(MFI *D)
{
}

static inline unsigned char
AddPixel(unsigned int p1,unsigned int p2)
{
    unsigned int p3;

    // NOTE: to prevent truncation on the right side of the assignment, we're
    // using "unsigned int" [instead of "unsigned char"] for the arguments.
    //
    // as part of the C calling conventions, passing "unsigned char" values are
    // "zero extended" to "unsigned int" _even_ if the arg is "unsigned char"
    // because [most] stack pushes have to be word aligned
    //
    // we take advantage of that to eliminate the explicit casting below

#if 0
    p3 = (unsigned int) p1 + (unsigned int) p2;
#else
    p3 = p1 + p2;
#endif

    if (p3 > 255)
        p3 = 255;

    return p3;
}

void
AddImages(MFI *img3,const MFI *img1,const MFI *img2)
{
    int CombineCounter;
    const RGBPixel *pix1;
    const RGBPixel *pix2;
    RGBPixel *pix3;

    if (img1->ImgWidth != img2->ImgWidth || img1->ImgHeight != img2->ImgHeight)
        fault("They're not the same size you dummy!");

    *img3 = *img1;

    img3->RGB = malloc(sizeof(*img3->RGB) * img3->ImgSize);
    if (img3 == NULL)
        fault("no memory for img3 -- %s\n",strerror(errno));

    pix1 = img1->RGB;
    pix2 = img2->RGB;
    pix3 = img3->RGB;

    for (CombineCounter = 0; CombineCounter < img1->ImgSize;
        ++CombineCounter, ++pix3, ++pix1, ++pix2) {
        pix3->red = AddPixel(pix1->red,pix2->red);
        pix3->green = AddPixel(pix1->green,pix2->green);
        pix3->blue = AddPixel(pix1->blue,pix2->blue);
    }
}

void
P6read(MFI *img,FILE *ImageRead)
{
    size_t count;

    count = fread(img->RGB,sizeof(*img->RGB),img->ImgSize,ImageRead);
    if (count != img->ImgSize)
        fault("error with loading image -- %s\n",strerror(errno));
}

void
Reading(MFI *img,char *filename)
{
    FILE *ImageRead;                    // file pointer or handle
    int p;

    if (strstr(filename, ".ppm") == NULL) {
        strcat(filename, ".ppm");
    }

    printf("Reading: %s ...\n",filename);
    ImageRead = fopen(filename, "rb");

    // check if it exists
    if (ImageRead == NULL)
        fault("Nope doesn't exist\n");

    // get all the image information
    fscanf(ImageRead, "%s %d %d %d",
        img->Format, &img->ImgWidth, &img->ImgHeight, &img->MaxInfo);

    img->ImgSize = img->ImgWidth * img->ImgHeight;

    // allocate memory for image
    img->RGB = malloc(sizeof(*img->RGB) * img->ImgSize);
    if (img->RGB == NULL)
        fault("theres no space!\n");

    // skip over comment
#if 0
    p = fgetc(ImageRead);
    while (p == '#') {
        while (fgetc(ImageRead) != '\n') {
            p = fgetc(ImageRead);
        }
    }
    ungetc(p, ImageRead);
#else
    while (1) {
        p = fgetc(ImageRead);
        if (p != '#')
            break;

        while (1) {
            if (p == '\n')
                break;
            if (p == EOF)
                break;
            p = fgetc(ImageRead);
        }
    }
    if (p != '\n')
        fault("image file too short\n");
#endif

    // check image size
    if (img->ImgHeight > 1080 || img->ImgWidth > 1920)
        fault("This is too big -- ImgHeight=%d ImgWidth=%d\n",
            img->ImgHeight,img->ImgWidth);

    // reading image width and height
    printf("The height of the image is %d and the width is %d\n",
        img->ImgWidth, img->ImgHeight);
    printf("The maximum value of each pixel is %d\n", img->MaxInfo);

    switch (img->Format[0]) {
    case 'P':
        switch (img->Format[1]) {
        case '1':
            fault("This is a P1\n");
            break;
        case '2':
            fault("This is a pgm\n");
            break;
        case '3':
            fault("This is a ppm P3\n");
            break;
        case '5':
            printf("This is a pgm\n");
            break;
        case '6':
            printf("This is a ppm P6\n");
            if (img->MaxInfo == 255)
                P6read(img,ImageRead);
            else
                fault("Unsupported pixel size -- MaxInfo=%d\n",img->MaxInfo);
            break;
        default:
            fault("this isn't a ppm file");
            break;
        }
        break;
    default:
        fault("this isn't a ppm file");
        break;
    }
}

void
Save(const MFI *img,char *InputFileName)
{
    FILE *SavePointer;

    // NOTE/FIX: this must be done _before_ the existence check
#if 1
    if (strstr(InputFileName, ".ppm") == NULL)
        strcat(InputFileName, ".ppm");
#endif

#if 0
    SavePointer = fopen(InputFileName, "r");
    if (SavePointer != NULL) {
        fclose(SavePointer);
        exit(0);
    }
    else {
        fclose(SavePointer);
        printf("File is good to go!");
    }
#else
    struct stat st;
    if (stat(InputFileName,&st) >= 0)
        fault("File exists, try another name");
    printf("File is good to go!");
#endif

    // NOTE/BUG: this must be done _before_ the existence check
#if 0
    if (strstr(InputFileName, ".ppm") == NULL)
        strcat(InputFileName, ".ppm");
#endif

    SavePointer = fopen(InputFileName, "wb");
    if (!SavePointer)
        fault("unable to open output file -- %s\n",strerror(errno));

    fprintf(SavePointer, "%s\n%d %d\n%d\n",
        img->Format, img->ImgWidth, img->ImgHeight, img->MaxInfo);
#if 0
    fwrite(S.RGB, 3 * S.ImgHeight, 4 * S.ImgWidth - 3, SavePointer);
#else
    size_t count = fwrite(img->RGB,sizeof(*img->RGB),img->ImgSize,SavePointer);
    if (count != img->ImgSize)
        fault("fault during fwrite -- %s\n",strerror(errno));
#endif

    fclose(SavePointer);
}

void
getfile(char ***argp,char *file,const char *prompt)
{
    char **argv = *argp;
    char *cp;

    do {
        cp = *argv;

        if (cp != NULL) {
            ++argv;
            strcpy(file,cp);
            printf("%s is %s\n",prompt,file);
            break;
        }

        printf("please enter the name of %s:",prompt);
        scanf(" %s",file);
    } while (0);

    *argp = argv;
}

int
main(int argc,char **argv)
{
    struct MFI ImgOne;
    struct MFI ImgTwo;
    struct MFI ImgThree;
    char Image1[128];
    char Image2[128];
    char Image3[128];

    --argc;
    ++argv;

    getfile(&argv,Image1,"image 1");
    getfile(&argv,Image2,"image 2");
    getfile(&argv,Image3,"output");

#if 0
    ImgOne = Reading(Image1);
    ImgTwo = Reading(Image2);
#else
    Reading(&ImgOne,Image1);
    Reading(&ImgTwo,Image2);
#endif

#if 0
    Display(ImgOne);
    Display(ImgTwo);
#endif

#if 0
    ImgThree = AddImages(ImgOne, ImgTwo);
#else
    AddImages(&ImgThree, &ImgOne, &ImgTwo);
#endif

    Save(&ImgThree,Image3);

    printf("\nfile is closed\n");

#if 0
    free(ImgOne.ImageMemory);
    free(ImgTwo.ImageMemory);
    free(ImgThree.ImageMemory);
#else
    free(ImgOne.RGB);
    free(ImgTwo.RGB);
    free(ImgThree.RGB);
#endif
    printf("\nimage freed\n");

    return 0;
}

Ah, okay, thank you very much! So my main problem is only P6 files can be read with both your code and mine. I'm assuming adding with P3 files would be different due to them being ASCII not binary? Thank you again! — Eve Pitt, Dec 29 '20 at 19:18

C not entering For Loop which merge 2 PPM images

1 Answers1