Is my function correctly returning a pointer to a struct?

Question

I have a project on zybooks. And my code appears to be working properly as it can correctly highlight the edges of different images.But, zybooks is automated and it is failing my edgedetect function. This is my code for both functions:

#include<stdio.h>
#include<math.h>
#include<stdlib.h>
#include<string.h>
#include<limits.h>
typedef struct _image {
    int** pixels;
    int width;
    int height;
} Image;


Image* readImage(char* filename) {
    Image *pic= malloc(sizeof(Image));
    char type[3];
    int maxvalue;
    FILE *f1=NULL;
    f1= fopen(filename,"r");
        if(f1==NULL){
            printf("Unable to read image: %s\n",filename);
             return NULL;
        }
    fscanf(f1,"%s",type);
    fscanf(f1,"%d",&pic->width);
    fscanf(f1,"%d",&pic->height);
    fscanf(f1,"%d",&maxvalue);
    pic->pixels = (int **)malloc(sizeof(int *) * pic->height);
         for (int i = 0; i < pic->height; i++) {
            pic->pixels[i] = (int *)malloc(sizeof(int) * pic->width);
         }
     for (int i = 0; i < pic->height; i++) {
        for (int j = 0; j < pic->width; j++) {
           fscanf(f1,"%d",&pic->pixels[i][j]);
        }

    }
    fclose(f1);
  return pic;


}

Image* edgeDetect(Image* img, int threshold) {
    Image *edges;
    edges=malloc(sizeof(Image));
        edges->pixels = (int **)malloc(sizeof(int *) * img->height);
            for (int i = 0; i < img->height; i++) {
                edges->pixels[i] = (int *)malloc(sizeof(int) * img->width);
            }
    edges->height = img->height;
    edges->width = img->width;
     for (int i = 0; i < img->height; i++) {
        for (int j = 0; j < img->width; j++) {
            edges->pixels[i][j]=0;
        }
     }
     for(int i=0; i< edges->height;i++){
         for(int j=0; j<edges->width; j++){
             if( i > 0 && i< (edges->height-1) && j>0 && j<(edges->width-1) ){
                if(abs(img->pixels[i][j] - img->pixels[i][j+1]) > threshold || abs(img->pixels[i][j] - img->pixels[i-1][j]) > threshold)
                     edges->pixels[i][j]=255;
             }
            }
         }
    return edges;
}

int saveImage(char* filename, Image* img) {
   FILE* f1= NULL;
    f1= fopen(filename,"w");
    if(f1==NULL){
        printf("Unable to write image: %s\n",filename);
    return 1;
    }
    fprintf(f1,"P2\n");
    fprintf(f1,"%d %d\n",img->width,img->height);
    fprintf(f1,"255\n");
    for (int i = 0; i < img->height; i++) {
        for (int j = 0; j < img->width; j++) {
           fprintf(f1,"%d ",img->pixels[i][j]);
        }
        fprintf(f1,"\n");
    }
        fclose(f1);
        return 0;
}

void freeImage(Image* img) {
    for (int i = 0; i < img->height; i++) {
         free(img->pixels[i]);
}
free(img->pixels);
free(img);
}

int main(int argc, char** argv) {
    if( argc !=4){
         printf("Usage: ./a.out input.pgm output.pgm threshold\n");
    return 1;
    }
    int threshold= atoi(argv[3]);
    Image *data;
    data=readImage(argv[1]);
    if (data==NULL)
        return 1;
     Image *edge;
    edge= edgeDetect(data,threshold);
    freeImage(data);
    int result= saveImage(argv[2],edge);
    if (result==1)
        return 1;
    freeImage(edge);


    return 0;
}

Any tip would be helpful.I know the code might be confusing as it might not follow standard formatting but this is literally my first coding class.

Answer 1

regarding:

Image *pic= malloc(sizeof(Image)); 

Always check (!=NULL) the returned value to assure the operation was successful. If not successful, call

perror( "your error message" );

to output to stderr both your error message and the text reason the system thinks the error occurred.

regarding:

printf("Unable to read image: %s\n",filename); 

Error messages should be output to stderr, not stdout. Suggest using:

fprintf( stderr, "Unable to read image: %s\n %s\n", filename, strerror( errno ) );

When calling any of the scanf() family of functions, like fscanf(), always check the returned value (not the parameter values) to assure the operation was successful. Note: That family of functions returns the number of successful 'input format conversion' specifiers.

regarding:

fscanf(f1,"%s",type); 

1. any returned value other than 1 indicates an error occurred.
2. when using %s and/or %[...] always include a MAX CHARACTERS modifier that is one less than the length of the input buffer to avoid any buffer overflow and the resulting undefined behavior, because those 'input format conversion' specifiers always append a NUL byte to the input.

regarding:

pic->pixels = (int **)malloc(sizeof(int *) * pic->height);

in C, the returned type is void* which can be assigned to any pointer. Casting just clutters the code. Suggest removing the cast.

for ease of readability and understanding:

1. separate code blocks: for if else while do...while switch case default should be separated via a single blank line.

2. consistently indent the code. Indent after every opening brace '{'. Unindent before every closing brace '}'. Suggest each indent level be 4 spaces

3. insert appropriate horizontal space: inside parens, inside braces, inside brackets, after commas, after semicolons, around C operators

the function: malloc() expects the parameter to be of type size_t, however; statements like:

edges->pixels = (int **)malloc(sizeof(int *) * img->height);

are being passed a int as part of the parameter. ( I.E. the img->height ) This results in an implicit conversion between int and size_t which is 'usually' harmless, but is still a risky conversion.