Storing an array from a file then writing it to another file

Question

I've been having trouble with this part of code for a while now. I'm supposed to store the pixel values from the ppm file in an array and then write them to another file upon a keypress. I've been trying this same piece of code for over a week now and it still doesn't seem to work. Any help is very much appreciated (this isn't all my code but only the bits relevant to the question). What seems to happen is that the code seems to stop running after reading in the file for writing so I have no idea if the array is even successfully allocated. Sorry if its a bit of a mess, I was more focused on functionality than neatness.

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

#define RGB_COMPONENT_COLOUR 255
#define height 1080
#define width 1920

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

typedef struct
{
    int x, y;
    PPMPixel *data;
} PPMImage;

int main(PPMImage *readPPM())
{
    char key = 0;
    do
    {
        printf("\tPress r to read in an image in ppm format\n");
        printf("\tPress s to save image in ppm format\n");
        printf("\tPress q to quit\n");

        {
            scanf("\t %c", &key);
            fseek(stdin, 0, SEEK_END);
            switch(key)
            {
                FILE *fp, *pf;
                PPMImage *img;
                int i, j;
                int **array = malloc(height * sizeof(int*));
                array[i] = malloc(width * sizeof(int));
                case 'r' :
                {
                    char buff[16];
                    int c, rgb_comp_colour;
                    int e;
                    char fname[100];
                    printf("Enter file name: ");
                    scanf("%s", fname);
                    fseek(stdin,0,SEEK_END);
                    fp = fopen(fname, "r");
                    if (fp == NULL)
                    {   
                        printf("\tError while opening the file\n");
                    }                        
                    else    
                    {
                        printf("\tReading in %s\n", fname);
                    }

                    if (fp)
                    {
                        while ((e = getc(fp))!=EOF)
                        putchar(e);

                        if (!array)
                        {
                            perror("\tError occured allocating memory to the array\n");
                            exit(1);
                        }
                        else
                        {
                            printf("\tMemory allocated to the array successfully\n");
                        }

                        for(i=0;i<width;i++)
                        {
                            for(j=0;j<height;j++)
                            {
                                fseek(fp, 0, SEEK_SET);
                                fscanf(fp, "%d", &array[i][j]);
                            }
                        }
                    else
                    {
                        printf("\tCould not successfully allocate array\n");
                    }
                
                }
                break;
            
                case 's' :
                {
                    char fname2[100];
                    printf("Enter file name: ");
                    scanf("%s", fname2);
                    fseek(stdin,0,SEEK_END);
                    pf = fopen(fname2, "w");
                    if (pf == NULL)
                    {   
                        printf("\tError while opening the file\n");
                    }                        
                    else    
                    {
                        printf("\tWriting in %s\n", fname2);
                    }
                
                    for(i=0;i<width;i++)
                    {
                        for(j=0;j<height;j++)
                        {
                            fseek(pf, 0, SEEK_SET);
                            fprintf(pf, "%d ", array[i][j]);
                        }
                    }
                    if (!array[i][j])
                    {
                        printf("File write error");
                    }
                    else
                    {
                        printf("File written successfully");
                    }
                }
                break;

                case 'q' : //If q is pressed the code ends and a message is printed so the user knows the program has been terminated.
                {
                    printf("\tTerminating program...\n");
                }
                break;

                default: //If anything other than the specified case statement key presses is entered, the code is looped again and an error message is printed.
                {
                    printf("\tInvalid Input\n");
                }

                fclose(fp);
                fclose(pf);
                free(array);
                free(array[i]);
                free(img);
            }
        }
    }
    while(key != 'q');
    return 0;    
}

Aside: Why do you have `\t` at the beginning of the scanf format string? — Barmar, Dec 27 '21 at 18:04
`array[i] = malloc(width * sizeof(int));` You haven't assigned `i` yet. — Barmar, Dec 27 '21 at 18:04
`fscanf(fp, "%d", &array[i][j]);` Will try to read a text integer. You want to read only 3 bytes binary for rgb. And why do you have `array` anyway? Shouldn't your read directly to the `PPMImage.data` array? — 001, Dec 27 '21 at 18:10
Also relevant: [Can I put code outside of cases in a switch?](https://stackoverflow.com/a/29023816) — 001, Dec 27 '21 at 18:12
@Barmar That's a mistake. Tab doesn't work for scanf's thanks for pointing it out. — Daniel Bland, Dec 27 '21 at 19:09
@JohnnyMopp The problem is that if the mallocs and frees inside case r then I won't be able to access the stored array from case s because the memory would have been freed. At least that's how I understand it to work. — Daniel Bland, Dec 27 '21 at 19:14
@Barmar I didn't put the malloc inside the for loop because it messed up the switch statement for a reason I do not know (the program just ends after one case instead of breaking and looping to the start). I was also told that looping mallocs can put strain on a PC because its essentially allocating memory in a loop. — Daniel Bland, Dec 27 '21 at 19:22
Once you `malloc` something, it doesn't go away until you `free()` it. The pointer that holds the value may go out of scope though. — 001, Dec 27 '21 at 19:40
Yes, it's allocating memory in a loop. But that's what you want to do. You need to allocate a string for each element of the array. — Barmar, Dec 27 '21 at 19:43
@Barmar I see. Thanks. My code now stops after case r however when it should loop despite that being the only thing I have changed and I'm not sure why its caused this. — Daniel Bland, Dec 27 '21 at 19:54

001 · Answer 1 · 2021-12-27T21:58:07.877

You should make a couple of functions: one to read a ppm file and one to save one. This will greatly simplify main(). Also I don't see any need for the intermediate arrays. malloc() the PPMPixel *data; array after you know the width and height and read the data directly into it with fread().

// You should not call fflush on stdin. Use this instead
void clear_to_end(FILE *fp)
{
    int c;
    while ((c = fgetc(fp)) != -1 && c != '\n') ;
}

PPMImage* load_file()
{
    char path[1024]
    printf("Enter file name: ");
    if (!fgets(path, sizeof(path), stdin)) return NULL;
    path[strcspn(path, "\n")] = '\0';

    FILE *fp = fopen(path, "rb");
    if (fp) {
        // Create a PPMImage struct
        PPMImage *img = malloc(sizeof(*img));
        if (img) {
            char buffer[1024];
            // Read the header
            fgets(buffer, sizeof(buffer), fp);
            // Some ppm files contain comments
            do {
                fgets(buffer, sizeof(buffer), fp);
            } while (buffer[0] == '#');
            // Get the dimensions
            if (2 == sscanf(buffer, "%d %d", &img->x, &img->y)) {
                int max_color;  // Not used
                fscanf(fp, "%d", &max_color);
                // The header is text, but the rest is binary data
                clear_to_end(fp);
                // Calc the size of the pixel array and allocate
                size_t bytes = sizeof(PPMPixel) * img->x * img->y;
                img->data = malloc(bytes);
                if (img->data) {
                    int items_read = 0;
                    // Read ALL the pixels
                    items_read = fread(img->data, sizeof(PPMPixel), img->x * img->y, fp);
                    if (items_read == img->x * img->y) {
                        // Success!
                        return img;
                    }
                    else {
                        free(img->data);
                    }
                }
            }
            else free(img);
        }
        fclose(fp);
    }
    return NULL;
}

void save_file(PPMImage *img)
{
    // TODO .....
}

int main()
{
    char key = 0;
    PPMImage* img = NULL;
    do {
        puts("\tPress r to read in an image in ppm format");
        puts("\tPress s to save image in ppm format");
        puts("\tPress q to quit");
        
        scanf(" %c", &key);
        clear_to_end(stdin);
        switch (key) {
            case 'r':
                // TODO: if img != NULL, free it
                img = load_file();
                break;
            case 's':
                save_file(img);
                break;
            case 'q':
                puts("\tTerminating program...");
                break;
            default:
                puts("\tInvalid Input");
                break;
        }
    } while (key != 'q');
    return 0;
}

I left the function bodies empty/sparse as this looks like homework. You need to fill in the blanks and add error handling.

I will give this a go thanks! Yes this is unfortunately a small part of a very big assignment that relies heavily on being able to access arrays like this for image manipulation so I wanted to be able to understand how to make it work. — Daniel Bland, Dec 27 '21 at 20:03
So I've implemented your code into mine but come across a few things I'm not too sure about. The array is now defined as image but when I try to do fscanf(fp, "%d", &image[i][j]); it tells me that it must be pointer to pointer type. But other parts require it to be pointer to struct. Which one is better to use? — Daniel Bland, Dec 27 '21 at 21:50
Don't use `fscanf` to read binary data. That would only work if the file is text. I have in my answer how to read all the pixels at once with: `fread(img->data, sizeof(PPMPixel), img->x * img->y, fp);` First, parse the head then call that. Just one read gets all the pixel data. I'll update with full code... — 001, Dec 27 '21 at 21:52

Storing an array from a file then writing it to another file

1 Answers1