16
\$\begingroup\$

I'm making a image processing application in c from scratch using P6 ppm images, I want some input on my code before I start adding more features to prevent it from falling apart if it's not well structured

sample output enter image description here improc.h

#ifndef IMPROC_H #define IMPROC_H #include <stdint.h> typedef struct { int size; double *weights; } Kernel; typedef struct { uint8_t r; uint8_t g; uint8_t b; } Pixel; typedef struct { double r; double g; double b; } Accumulator; typedef struct { int height; int width; Pixel *pixels; } Image; typedef struct { double re; double im; } Complex; Image *create_image(int width, int height); void free_image(Image *image); Image *load_image(char *filename); int save_image(char *filename, Image image); Image *grayscale(Image image); Image *perceptual_grayscale(Image image); double kernel_min(Kernel kernel); double kernel_max(Kernel kernel); Accumulator convolve(Image image, Kernel kernel, int row, int col); Image *apply_kernel(Image image, Kernel kernel); Image *sobel(Image image); Kernel sobel_y(int n); Kernel sobel_x(int n); unsigned modulo(int value, unsigned m); #endif

improc.c

#include <stdio.h> #include <stdlib.h> #include <stdint.h> #include <string.h> #include <math.h> #include <unistd.h> #include <errno.h> #include "improc.h" Image *create_image(int width, int height) { Image *image = malloc(sizeof(Image)); if (image == NULL) { fprintf(stderr, "Could not allocate memory to Image: %s\n", strerror(errno)); return NULL; } image->width = width; image->height = height; image->pixels = malloc(width*height*sizeof(Pixel)); if (image->pixels == NULL) { free(image); fprintf(stderr, "Could not allocate memory to pixels: %s\n", strerror(errno)); return NULL; } return image; } void free_image(Image *image) { if (image != NULL) { if (image->pixels != NULL) free(image->pixels); free(image); } } Image *load_image(char *filename) { char magic[3]; int maxval, width, height; FILE *fp = fopen(filename, "rb"); if (fp == NULL) { fprintf(stderr, "Error opening file: %s\n", strerror(errno)); return NULL; } fscanf(fp, "%2s", magic); if (magic[0] != 'P' || magic[1] != '6') { fprintf(stderr, "Not a valid P6 ppm file: %s\n", strerror(errno)); fclose(fp); return NULL; } fscanf(fp, "%d%d%*c", &width, &height); Image *image = create_image(width, height); fscanf(fp, "%d%*c", &maxval); fread(image->pixels, sizeof(Pixel),image->width*image->height, fp); fclose(fp); return image; } int save_image(char *filename, Image image) { FILE *fp = fopen(filename, "wb"); if (fp == NULL) { fprintf(stderr, "Error opening file: %s\n", strerror(errno)); return -1; } fprintf(fp, "P6\n%d %d\n255\n", image.width, image.height); fwrite(image.pixels, sizeof(Pixel), image.width*image.height, fp); fclose(fp); return 0; } Image *grayscale(Image image) { Image *gray = create_image(image.width, image.height); Pixel pix; int index; uint8_t avg; for (int row = 0; row < image.height; row++) { for (int col = 0; col < image.width; col++) { index = row*image.width + col; pix = image.pixels[index]; avg = (uint8_t) ((pix.r + pix.g + pix.b)/3.0); gray->pixels[index] = (Pixel) {avg, avg, avg}; } } return gray; } Image *perceptual_grayscale(Image image) { Image *gray = create_image(image.width, image.height); Pixel pix; uint8_t bt_601; int index; for (int row = 0; row < image.height; row++) { for (int col = 0; col < image.width; col++) { index = row*image.width + col; pix = image.pixels[index]; bt_601 = (uint8_t) (0.2126*pix.r + 0.7152*pix.g + 0.0722*pix.b); gray->pixels[index] = (Pixel) {bt_601, bt_601, bt_601}; } } return gray; } double kernel_min(Kernel kernel) { double min = 0; for (int index = 0; index < kernel.size*kernel.size; index++) { if (kernel.weights[index] < 0) min += kernel.weights[index]; } return min*255; } double kernel_max(Kernel kernel) { double max = 0; for (int index = 0; index < kernel.size*kernel.size; index++) { if (kernel.weights[index] > 0) max += kernel.weights[index]; } return max*255; } Accumulator convolve(Image image, Kernel kernel, int row, int col) { Accumulator accumulator = {0, 0, 0}; for (int r_off = -kernel.size/2; r_off <= kernel.size/2; r_off++) { for (int c_off = -kernel.size/2; c_off <= kernel.size/2; c_off++) { int ir = modulo(row + r_off, image.height); int ic = modulo(col + c_off, image.width); int kr = r_off + kernel.size/2; int kc = c_off + kernel.size/2; int index = ir*image.width + ic; Pixel pixel = image.pixels[index]; accumulator.r += pixel.r*kernel.weights[kr*kernel.size + kc]; accumulator.g += pixel.g*kernel.weights[kr*kernel.size + kc]; accumulator.b += pixel.b*kernel.weights[kr*kernel.size + kc]; } } return accumulator; } Image *apply_kernel(Image image, Kernel kernel) { Image *conv = create_image(image.width, image.height); double max = kernel_max(kernel); double min = kernel_min(kernel); double alpha = max - min; for (int row = 0; row < image.height; row++) { for (int col = 0; col < image.width; col++) { Accumulator accumulator = convolve(image, kernel, row, col); accumulator.r = 255*(accumulator.r - min)/alpha; accumulator.g = 255*(accumulator.g - min)/alpha; accumulator.b = 255*(accumulator.b - min)/alpha; conv->pixels[row*image.width + col].r = accumulator.r; conv->pixels[row*image.width + col].g = accumulator.g; conv->pixels[row*image.width + col].b = accumulator.b; } printf("%lf\r", 100.0*(1.0*row)/image.height); fflush(stdout); } return conv; } Kernel sobel_x(int n) { Kernel sx; sx.size = 3; sx.weights = malloc(sizeof(double)*sx.size*sx.size); sx.weights[0] = -1; sx.weights[1] = -2; sx.weights[2] = -1; sx.weights[3] = 0; sx.weights[4] = 0; sx.weights[5] = 0; sx.weights[6] = 1; sx.weights[7] = 2; sx.weights[8] = 1; return sx; } Kernel sobel_y(int n) { Kernel sy; sy.size = 3; sy.weights = malloc(sy.size*sy.size*sizeof(double)); sy.weights[0] = -1; sy.weights[1] = 0; sy.weights[2] = 1; sy.weights[3] = -2; sy.weights[4] = 0; sy.weights[5] = 2; sy.weights[6] = -1; sy.weights[7] = 0; sy.weights[8] = 1; return sy; } Image *sobel(Image image) { Image *conv = create_image(image.width, image.height); Image *sobx, *soby; Kernel sx = sobel_x(3); Kernel sy = sobel_y(3); sobx = apply_kernel(image, sx); soby = apply_kernel(image, sy); save_image("sobel_x.ppm", *sobx); save_image("sobel_y.ppm", *soby); double max = kernel_max(sx); double min = kernel_min(sx); double alpha = max - min; for (int row = 0; row < image.height; row++) { for (int col = 0; col < image.width; col++) { Accumulator x = convolve(image, sx, row, col); Accumulator y = convolve(image, sy, row, col); x.r = 255*(x.r)/alpha; x.g = 255*(x.g)/alpha; x.b = 255*(x.b)/alpha; y.r = 255*(y.r)/alpha; y.g = 255*(y.g)/alpha; y.b = 255*(y.b)/alpha; Accumulator gradient = { sqrt(x.r*x.r + y.r*y.r), sqrt(x.g*x.g + y.g*y.g), sqrt(x.b*x.b + y.b*y.b), }; gradient.r = (gradient.r > 255)? 255: gradient.r; gradient.g = (gradient.g > 255)? 255: gradient.g; gradient.b = (gradient.b > 255)? 255: gradient.b; conv->pixels[row*image.width + col].r = (uint8_t) gradient.r; conv->pixels[row*image.width + col].g = (uint8_t) gradient.g; conv->pixels[row*image.width + col].b = (uint8_t) gradient.b; } printf("%lf\r", 100.0*(1.0*row)/image.height); fflush(stdout); } return conv; } unsigned modulo(int value, unsigned m) { int mod = value % (int) m; if (mod < 0) mod += m; return mod; }

main.c

#include <stdio.h> #include <stdlib.h> #include <math.h> #include "improc.h" int main(int argc, char **argv) { (void) argc; Image *ema = load_image(argv[1]); save_image("identity.ppm", *ema); Image *sob = sobel(*ema); save_image("sobel.ppm", *sob); }

build script

#!/bin/bash gcc improc.c main.c -O3 -g -Wall -Wpedantic -lm -Wextra -o main
\$\endgroup\$
6
  • 1
    \$\begingroup\$ That is impressive you did it in C and from scratch (without using OpenCV ...etc) \$\endgroup\$ Commented Nov 15, 2023 at 10:15
  • 1
    \$\begingroup\$ Pray, why is this tagged with "signal-processing"? \$\endgroup\$ Commented Nov 15, 2023 at 11:19
  • 1
    \$\begingroup\$ You are right, I think he means "image processing" @Harith \$\endgroup\$ Commented Nov 15, 2023 at 11:46
  • 9
    \$\begingroup\$ @BillalBegueradj: An image is a particular type of signal, and image processing is a specialization of signal processing. A great many topics apply to both, such as edge detection (seen here), linearity, resampling, and aliasing just to name a few. \$\endgroup\$ Commented Nov 15, 2023 at 19:20
  • 2
    \$\begingroup\$ Do not use build shell scripts, but Makefiles, which are made for this. \$\endgroup\$ Commented Nov 16, 2023 at 9:08

7 Answers 7

Reset to default
10
\$\begingroup\$

Let's talk about the API. I've worked with multiple imaging libraries over the past ~25 years, and have significant experience designing them too.

API:

Let's start with the image container, the core of any imaging library.

typedef struct { int height; int width; Pixel *pixels; } Image; //… Image *ema = load_image(argv[1]); Image *sob = sobel(*ema); free_image(ema); /* (missing from the code, but should be there.) //…

Most of the times that you refer to an image, you use *name, except in a few places where it's just name. This is confusing, and unnecessarily complicated. First of all, passing a struct (even if it's small now) by value (i.e. its values are copied) is more expensive than passing its pointer. Second, when you create the image object you malloc it, and it's awkward to then copy its contents to the stack to call a function. Third, as the library grows, so will this struct (a flag for RGB vs grayscale, or maybe an int for the number of channels, maybe a flag for which color space it's in, maybe a flag for the data type, maybe a flag for whether the struct owns the pixel data or not, ...). At some point copying the struct will be prohibitive.

So, let's enforce passing the struct by pointer, and let's make it easy to do so!

typedef struct { int height; int width; Pixel *pixels; } *Image; //… Image ema = load_image(argv[1]); Image sob = sobel(ema); free_image(ema); //…

Now Image is always a pointer to the struct. There's no type you can use to refer to the struct itself, you can only refer to the pointer. But it looks nice in the code, the user doesn't even need to know it's a pointer!

For Kernel you're doing something totally different: when you create a kernel (e.g. sobel_x) you create the struct on the stack and return it by value. Why the distinction? A kernel is, after all, just an image with a different type for the pixels.

Why is the kernel always square? This is an important limitation. At some point you'll be looking to use a 15x1 kernel, and you'll have to create a 15x15 kernel with lots of zeros, which will be 15 times as expensive to use.

The function convolve doesn't convolve two images, it only computes the result of the convolution at one pixel. This is surprising. Your function apply_kernel should be called convolve (or maybe convolution). The single pixel sub-function should, IMO, be private.

Likewise, Accumulator could be private until you have a reason to make it public. The fewer things you make public initially, the easier it will be to improve on the API. Making something public (i.e. putting it into improc.h) sort of fixes them in perpetuity. As soon as people start using that API, you can't change it any more*, but you can always add to it.

* At least not without forcing users to update their code. And you're a user too. As soon as you start using this library to write programs, it will become hard to make changes to the API, you'll have to update all the programs that use it too.

kernel_min and kernel_max have the wrong name. I was reading the code, and wondering why you were using addition and not max(). Later I came to realize that you use these functions to determine what the minimum and maximum possible values of the output image will be when you compute the convolution with that kernel.

You could instead consider adding offset and scale arguments to your convolution function, and clip the result of the convolution before writing it to the uint8 output. This makes the function more flexible: the maximum and minimum possible values are not often obtained, so your scaling is a bit too drastic, the result is a very dim image, and a strongly quantized derivative. A user might want to pick a smaller scaling value.

improc.h, which defines your API, should contain documentation for the functions and types it makes public. You can document in a separate file, but it's always easier to put the documentation directly in the header. You user will be able to easily find the documentation in their IDE, and many IDEs will even show this documentation in a tooltip when you hover over a function call with the mouse. I suggest you use the Doxygen style for documentation. Doxygen is a nice tool to generate HTML from the documentation in the header files, though it has some downsides as well (many people, including me, have written alternatives, but most of these will use the same style for the documentation source).

Efficiency:

The convolution tests, for each pixel, whether a neighbor is inside the image or not (you use modulo for this, a neat solution, but it still has a branch in it). It is actually (in my experience) faster to copy the image into a larger buffer, and pick some padding scheme to fill those values outside the original image. The convolution now doesn't need to do any tests at all.

You can also consider reducing the amount of coordinate computation you do within the loop:

Accumulator convolve(Image image, Kernel kernel, int row, int col) { Accumulator accumulator = {0, 0, 0}; r_offset = row - kernel.size / 2; c_offset = col - kernel.size / 2; int kindex = 0; for (int kr = 0; kr < kernel.size; kr++) { int ir = modulo(r_offset + kr, image.height); int iindex = ir * image.width; for (int kc = 0; kc <= kernel.size; kc++, kindex++) { int ic = modulo(c_offset + kc, image.width); Pixel pixel = image.pixels[iindex + ic]; accumulator.r += pixel.r * kernel.weights[kindex]; accumulator.g += pixel.g * kernel.weights[kindex]; accumulator.b += pixel.b * kernel.weights[kindex]; } } return accumulator; }

kindex, the index into the kernel, increases by 1 every inner loop iteration, so just increment it, don't compute kr*kernel.size + kc (and certainly don't compute that 3 times, even though your compiler will likely optimize that out). ir doesn't change during the inner loop, so compute it outside that loop. And a lot of the remaining computation you did was because your loop goes from -size/2 to size/2, rather than from 0 to size, and so you needed to add an offset again to index into the kernel.

(By the way, your code has a bug if kernel.size is even)

Style:

Please use an empty line in between functions. Vertical space is very important for readability.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I upvoted the answer, but I don't agree on the "hide pointer with typedef". I tend to agree with Linux coding style. But there is no general consensus: stackoverflow.com/q/750178/6070341 \$\endgroup\$ Commented Nov 17, 2023 at 12:10
  • 2
    \$\begingroup\$ The hidden pointer in your Image makes it hard to distinguish between const and non-const image data (even more so than the pixels member in the original). \$\endgroup\$ Commented Nov 18, 2023 at 9:26
  • 1
    \$\begingroup\$ @TobySpeight That’s a good point. You could of course define a ConstImage as a const pointer, and use that as the input to functions that don’t write to the image. But the pixel data is then still writeable. Making the pixels member a pointer to const would be strange though, it’d be a different struct altogether, not sure that casting from one to the other is pretty. \$\endgroup\$ Commented Nov 18, 2023 at 16:09
12
\$\begingroup\$

Enable more compiler warnings:

gcc -std=c17 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion -Wstrict-prototypes -fanalyzer main.c image.c -o image -lm main.c: In function ‘main’: main.c:7:20: warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 7 | save_image("identity.ppm", *ema); | ^~~~~~~~~~~~~~ In file included from main.c:2: image.h:34:22: note: expected ‘char *’ but argument is of type ‘const char *’ 34 | int save_image(char *filename, Image image); | ~~~~~~^~~~~~~~ main.c:9:20: warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 9 | save_image("sobel.ppm", *sob); | ^~~~~~~~~~~ In file included from main.c:2: image.h:34:22: note: expected ‘char *’ but argument is of type ‘const char *’ 34 | int save_image(char *filename, Image image); | ~~~~~~^~~~~~~~ main.c:4:14: warning: unused parameter ‘argc’ [-Wunused-parameter] 4 | int main(int argc, char **argv) | ~~~~^~~~ image.c: In function ‘create_image’: image.c:18:40: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 18 | image->pixels = malloc(width*height*sizeof(Pixel)); | ^ image.c: In function ‘load_image’: image.c:52:52: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion] 52 | fread(image->pixels, sizeof(Pixel),image->width*image->height, fp); | ~~~~~~~~~~~~^~~~~~~~~~~~~~ image.c: In function ‘save_image’: image.c:64:52: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion] 64 | fwrite(image.pixels, sizeof(Pixel), image.width*image.height, fp); | ~~~~~~~~~~~^~~~~~~~~~~~~ image.c: In function ‘convolve’: image.c:124:47: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 124 | int ir = modulo(row + r_off, image.height); | ~~~~~^~~~~~~ image.c:124:22: warning: conversion to ‘int’ from ‘unsigned int’ may change the sign of the result [-Wsign-conversion] 124 | int ir = modulo(row + r_off, image.height); | ^~~~~~ image.c:125:47: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 125 | int ic = modulo(col + c_off, image.width); | ~~~~~^~~~~~ image.c:125:22: warning: conversion to ‘int’ from ‘unsigned int’ may change the sign of the result [-Wsign-conversion] 125 | int ic = modulo(col + c_off, image.width); | ^~~~~~ image.c: In function ‘apply_kernel’: image.c:152:53: warning: conversion from ‘double’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Wfloat-conversion] 152 | conv->pixels[row*image.width + col].r = accumulator.r; | ^~~~~~~~~~~ image.c:153:53: warning: conversion from ‘double’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Wfloat-conversion] 153 | conv->pixels[row*image.width + col].g = accumulator.g; | ^~~~~~~~~~~ image.c:154:53: warning: conversion from ‘double’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Wfloat-conversion] 154 | conv->pixels[row*image.width + col].b = accumulator.b; | ^~~~~~~~~~~ image.c: In function ‘sobel_x’: image.c:165:39: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 165 | sx.weights = malloc(sizeof(double)*sx.size*sx.size); | ^ image.c:165:47: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 165 | sx.weights = malloc(sizeof(double)*sx.size*sx.size); | ^ image.c:161:20: warning: unused parameter ‘n’ [-Wunused-parameter] 161 | Kernel sobel_x(int n) | ~~~~^ image.c: In function ‘sobel_y’: image.c:181:40: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 181 | sy.weights = malloc(sy.size*sy.size*sizeof(double)); | ^ image.c:177:20: warning: unused parameter ‘n’ [-Wunused-parameter] 177 | Kernel sobel_y(int n) | ~~~~^ image.c: In function ‘sobel’: image.c:201:16: warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 201 | save_image("sobel_x.ppm", *sobx); | ^~~~~~~~~~~~~ image.c:56:22: note: expected ‘char *’ but argument is of type ‘const char *’ 56 | int save_image(char *filename, Image image) | ~~~~~~^~~~~~~~ image.c:202:16: warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 202 | save_image("sobel_y.ppm", *soby); | ^~~~~~~~~~~~~ image.c:56:22: note: expected ‘char *’ but argument is of type ‘const char *’ 56 | int save_image(char *filename, Image image) | ~~~~~~^~~~~~~~ image.c: In function ‘modulo’: image.c:240:13: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 240 | mod += m; | ^~ image.c:240:16: warning: conversion to ‘int’ from ‘unsigned int’ may change the sign of the result [-Wsign-conversion] 240 | mod += m; | ^ image.c:241:12: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 241 | return mod; | ^~~ In function ‘apply_kernel’: image.c:152:17: warning: dereference of NULL ‘conv’ [CWE-476] [-Wanalyzer-null-dereference] 152 | conv->pixels[row*image.width + col].r = accumulator.r; | ~~~~^~~~~~~~ ‘sobel’: events 1-2 | | 193 | Image *sobel(Image image) | | ^~~~~ | | | | | (1) entry to ‘sobel’ | 194 | { | 195 | Image *conv = create_image(image.width, image.height); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (2) calling ‘create_image’ from ‘sobel’ | +--> ‘create_image’: event 3 | | 9 | Image *create_image(int width, int height) | | ^~~~~~~~~~~~ | | | | | (3) entry to ‘create_image’ | ‘create_image’: event 4 | | 14 | return NULL; | | ^~~~ | | | | | (4) ‘0’ is NULL | <------+ | ‘sobel’: events 5-6 | | 195 | Image *conv = create_image(image.width, image.height); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (5) returning to ‘sobel’ from ‘create_image’ |...... | 199 | sobx = apply_kernel(image, sx); | | ~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (6) calling ‘apply_kernel’ from ‘sobel’ | +--> ‘apply_kernel’: events 7-8 | | 138 | Image *apply_kernel(Image image, Kernel kernel) | | ^~~~~~~~~~~~ | | | | | (7) entry to ‘apply_kernel’ | 139 | { | 140 | Image *conv = create_image(image.width, image.height); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (8) calling ‘create_image’ from ‘apply_kernel’ | +--> ‘create_image’: event 9 | | 9 | Image *create_image(int width, int height) | | ^~~~~~~~~~~~ | | | | | (9) entry to ‘create_image’ | ‘create_image’: event 10 | | 14 | return NULL; | | ^~~~ | | | | | (10) ‘conv’ is NULL | <------+ | ‘apply_kernel’: events 11-12 | | 140 | Image *conv = create_image(image.width, image.height); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (11) return of NULL to ‘apply_kernel’ from ‘create_image’ | 141 | double max = kernel_max(kernel); | | ~~~~~~~~~~~~~~~~~~ | | | | | (12) calling ‘kernel_max’ from ‘apply_kernel’ | +--> ‘kernel_max’: events 13-15 | | 110 | double kernel_max(Kernel kernel) | | ^~~~~~~~~~ | | | | | (13) entry to ‘kernel_max’ |...... | 113 | for (int index = 0; index < kernel.size*kernel.size; index++) { | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (14) following ‘true’ branch... | 114 | if (kernel.weights[index] > 0) | | ~~~~~~~~~~~~~~ | | | | | (15) ...to here | <------+ | ‘apply_kernel’: events 16-17 | | 141 | double max = kernel_max(kernel); | | ^~~~~~~~~~~~~~~~~~ | | | | | (16) returning to ‘apply_kernel’ from ‘kernel_max’ | 142 | double min = kernel_min(kernel); | | ~~~~~~~~~~~~~~~~~~ | | | | | (17) calling ‘kernel_min’ from ‘apply_kernel’ | +--> ‘kernel_min’: events 18-20 | | 101 | double kernel_min(Kernel kernel) | | ^~~~~~~~~~ | | | | | (18) entry to ‘kernel_min’ |...... | 104 | for (int index = 0; index < kernel.size*kernel.size; index++) { | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (19) following ‘true’ branch... | 105 | if (kernel.weights[index] < 0) | | ~~~~~~~~~~~~~~ | | | | | (20) ...to here | <------+ | ‘apply_kernel’: events 21-26 | | 142 | double min = kernel_min(kernel); | | ^~~~~~~~~~~~~~~~~~ | | | | | (21) returning to ‘apply_kernel’ from ‘kernel_min’ | 143 | double alpha = max - min; | 144 | for (int row = 0; row < image.height; row++) { | | ~~~~~~~~~~~~~~~~~~ | | | | | (22) following ‘true’ branch... | 145 | for (int col = 0; col < image.width; col++) { | | ~~~ ~~~~~~~~~~~~~~~~~ | | | | | | | (24) following ‘true’ branch... | | (23) ...to here | 146 | Accumulator accumulator = convolve(image, kernel, row, col); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (25) ...to here | | (26) calling ‘convolve’ from ‘apply_kernel’ | +--> ‘convolve’: events 27-31 | | 119 | Accumulator convolve(Image image, Kernel kernel, int row, int col) | | ^~~~~~~~ | | | | | (27) entry to ‘convolve’ |...... | 122 | for (int r_off = -kernel.size/2; r_off <= kernel.size/2; r_off++) { | | ~~~~~~~~~~~~~~~~~~~~~~ | | | | | (28) following ‘true’ branch... | 123 | for (int c_off = -kernel.size/2; c_off <= kernel.size/2; c_off++) { | | ~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~ | | | | | | | (30) following ‘true’ branch... | | (29) ...to here | 124 | int ir = modulo(row + r_off, image.height); | | ~~~~~~~~~~~~ | | | | | (31) ...to here | <------+ | ‘apply_kernel’: events 32-33 | | 146 | Accumulator accumulator = convolve(image, kernel, row, col); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (32) returning to ‘apply_kernel’ from ‘convolve’ |...... | 152 | conv->pixels[row*image.width + col].r = accumulator.r; | | ~~~~~~~~~~~~ | | | | | (33) dereference of NULL ‘conv’ | In function ‘sobel_x’: image.c:166:19: warning: dereference of possibly-NULL ‘sx.weights’ [CWE-690] [-Wanalyzer-possible-null-dereference] 166 | sx.weights[0] = -1; | ~~~~~~~~~~~~~~^~~~ ‘sobel’: events 1-2 | | 193 | Image *sobel(Image image) | | ^~~~~ | | | | | (1) entry to ‘sobel’ |...... | 197 | Kernel sx = sobel_x(3); | | ~~~~~~~~~~ | | | | | (2) calling ‘sobel_x’ from ‘sobel’ | +--> ‘sobel_x’: events 3-5 | | 161 | Kernel sobel_x(int n) | | ^~~~~~~ | | | | | (3) entry to ‘sobel_x’ |...... | 165 | sx.weights = malloc(sizeof(double)*sx.size*sx.size); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (4) this call could return NULL | 166 | sx.weights[0] = -1; | | ~~~~~~~~~~~~~~~~~~ | | | | | (5) ‘sx.weights’ could be NULL: unchecked value from (4) | In function ‘sobel_y’: image.c:182:19: warning: dereference of possibly-NULL ‘sy.weights’ [CWE-690] [-Wanalyzer-possible-null-dereference] 182 | sy.weights[0] = -1; | ~~~~~~~~~~~~~~^~~~ ‘sobel’: events 1-2 | | 193 | Image *sobel(Image image) | | ^~~~~ | | | | | (1) entry to ‘sobel’ |...... | 198 | Kernel sy = sobel_y(3); | | ~~~~~~~~~~ | | | | | (2) calling ‘sobel_y’ from ‘sobel’ | +--> ‘sobel_y’: events 3-5 | | 177 | Kernel sobel_y(int n) | | ^~~~~~~ | | | | | (3) entry to ‘sobel_y’ |...... | 181 | sy.weights = malloc(sy.size*sy.size*sizeof(double)); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (4) this call could return NULL | 182 | sy.weights[0] = -1; | | ~~~~~~~~~~~~~~~~~~ | | | | | (5) ‘sy.weights’ could be NULL: unchecked value from (4) |

These should be trivial to fix.

Though, this:

"warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type"

is a non-C spec warning as string literals such as "identity.ppm" are not const. The warning comes up due to compiling as C++ or telling the C compiler to treat string literals as const. This can create false concerns. In this case, though it is a good idea to change int save_image(char *filename, Image image); to int save_image(const char *filename, Image image);. – chux - Reinstate Monica

argv[1] could be NULL:

In main(), argv[1] is used without checking if it was a valid pointer. Check if argc is equal to 2 instead of casting it to void.

#if 0 int main(int argc, char **argv) { (void) argc; .. } #else int main(int argc, char **argv) { if (argc != 2) { /* Handle error here. */ } /* It is safe to use argv[1] now. */ } #endif

The functions that are not used outside of a translation unit should be defined as having internal linkage:

kernel_min(), kernel_max(), apply_kernel(), sobel_y(), sobel_x(), modulo() (and perhaps some more) are specific to improc.c. Their declarations should be removed from improc.h, and they should be defined with the static keyword.

Don't include unnecessary header files:

improc.c doesn't use anything from unistd.h or string.h.

main.c doesn't use anything from stdlib.h or math.h.

Check for overflow:

In create_image():

image->pixels = malloc(width*height*sizeof(Pixel));

width * height *sizeof(Pixel) can be greater than SIZE_MAX. Whilst that does not invoke undefined behavior and would simply wrap-around, it would allocate less memory than expected and you'd consider it a successful operation.

On the other hand, width * height * sizeof(Pixel) can evaluate to 0, which would pass a size of 0 to malloc().

The C standard (C17 7.22.3/1) says:

If the size of the space requested is zero, the behavior is implementation defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.

I wouldn't rely on it.

Also consider allocating to the referenced object, and not the type. If the type of image->pixels changes, you wouldn't have to change anything in the call to malloc().

On a side-note, malloc() and fopen() aren't required to set errno. You may want to fall back to fprintf()/fputs() in case it is not set. (This also requires setting errno to 0 before calling malloc())

Good job on not casting the return of malloc() and family.

free(NULL) is a NO-OP:

According to the C standard:

If ptr is a null pointer, no action occurs.

Checking for NULL simply adds more clutter to the code. Consider allowing free(NULL).

Simplify:

#if 0 if (magic[0] != 'P' || magic[1] != '6') #else if (strcmp(magic, "P6")) #endif

Check the return value of library functions:

fread(), fwrite(), and fscanf() have their return values ignored.

In sobel_x() and sobel_y(), the return value of malloc() is ignored.

Do not return an error code if the you don't intend to check for it:

In grayscale(), and other functions, the call to create_image() can return NULL if there was a memory allocation error, but there's no check for it. If NULL is returned, a subsequent operation would invoke undefined behavior by dereferencing it.

In main(), the return value of load_image() is ignored. I receive a segmentation fault if I feed improc.c to the executable.

As another side-note, grayscale() and perpetual_grayscale() are quite similar. They can be merged into a single function.

Eliminate unused parameters:

In sobel_x() and sobel_y(), n goes unused. The compiler did raise warnings, but you didn't cater to them.

Valgrind reports memory leaks:

valgrind ./image boxes_1.ppm ==4581== Memcheck, a memory error detector ==4581== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==4581== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info ==4581== Command: ./image boxes_1.ppm ==4581== ==4581== ==4581== HEAP SUMMARY: ==4581== in use at exit: 47,836 bytes in 10 blocks ==4581== total heap usage: 21 allocs, 11 frees, 71,700 bytes allocated ==4581== ==4581== LEAK SUMMARY: ==4581== definitely lost: 208 bytes in 6 blocks ==4581== indirectly lost: 47,628 bytes in 4 blocks ==4581== possibly lost: 0 bytes in 0 blocks ==4581== still reachable: 0 bytes in 0 blocks ==4581== suppressed: 0 bytes in 0 blocks ==4581== Rerun with --leak-check=full to see details of leaked memory ==4581== ==4581== For lists of detected and suppressed errors, rerun with: -s ==4581== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The pointer returned by malloc() must be later passed to free(). I do not see that in any part of your code.

\$\endgroup\$
7
  • 3
    \$\begingroup\$ "warning: passing argument 1 of ‘save_image’ discards ‘const’ qualifier from pointer target type" is a non-C spec warning as string literals such as "identity.ppm" are not const. The warning comes up due to compiling as C++ or telling the C compiler to treat string literals as const. This can create false concerns. In this case, though it is a good idea to change int save_image(char *filename, Image image); to int save_image(const char *filename, Image image);. \$\endgroup\$ Commented Nov 15, 2023 at 15:32
  • \$\begingroup\$ @chux-ReinstateMonica Yes, that had piqued my curiosity. Added to the answer. \$\endgroup\$ Commented Nov 15, 2023 at 16:16
  • 2
    \$\begingroup\$ Your GCC and Valgrind output code blocks would be easier to read if they weren't double-spaced. \$\endgroup\$ Commented Nov 15, 2023 at 18:21
  • 3
    \$\begingroup\$ Conversion of if (magic[0] != 'P' || magic[1] != '6') to strcmp is not a simplification; it is not even correct. The original code checks for { 'P', '6' }. The new code checks for { 'P', 'M', 0 }. Not only did you substitute the wrong magic, you've added a comparison of magic[2] for a NUL character which is not a valid part of the test. \$\endgroup\$ Commented Nov 15, 2023 at 19:16
  • 2
    \$\begingroup\$ @BenVoigt No big issue with fscanf(fp, "%2s", magic); if (magic[0] != 'P' || magic[1] != '6') { or with fscanf(fp, "%2s", magic); if (strcmp(magic, "P6")) { or strncmp(). All achieve the correct functionality if the return value of fscanf() is tested - and that missing test is the bigger issue. \$\endgroup\$ Commented Nov 15, 2023 at 21:48
11
\$\begingroup\$

Size to the object, not type

Rather than sizeof(Pixel), use sizeof image->pixels[0]. It is easier to code right, review and maintain.

// image->pixels = malloc(width*height*sizeof(Pixel)); image->pixels = malloc(width*height*sizeof image->pixels[0]);

Avoid overflow

The below is an int * int * size_t, the result is a size_t and that is passed to malloc(size_t).

image->pixels = malloc(width*height*sizeof(Pixel));

Aside from width and height being signed, the multiplication of non-negative values risks unnecessary overflow as the first int * int is an int multiplication with an int result which may exceed INT_MAX

By reordering, each multiplication becomes a size_t * a promoted int to size_t multiplication.

image->pixels = malloc(sizeof(Pixel)*width*height);

Given SIZE_MAX is very commonly more than INT_MAX, this re-ordering has less chance of overflow.

This also applies in other parts of OP's code.

Overflow testing

As even sizeof image->pixels[0]*width*height may overflow, code could check with:

if (width <= 0 || height <= 0 || width > SIZE_MAX/sizeof image->pixels[0]/height) { TBD_ErrorCode(); } image->pixels = malloc(sizeof image->pixels[0] * width * height);

Error checking

Each of the 3 below deserve checking of the function return value and acceptable range of values parsed.

fscanf(fp, "%d%d%*c", &width, &height); ... fscanf(fp, "%d%*c", &maxval); fread(image->pixels, sizeof(Pixel),image->width*image->height, fp);

Integer math vs floating point

The below, being deep in a 2x loop, does a fair amount of math and deserves performance profiling.

bt_601 = (uint8_t) (0.2126*pix.r + 0.7152*pix.g + 0.0722*pix.b);

Alternative 1: use float (this also applies in many other parts of OP's code.)

bt_601 = (uint8_t) (0.2126f*pix.r + 0.7152f*pix.g + 0.0722f*pix.b);

Alternative 2: use integers

bt_601 = (uint8_t) ((INT32_C(2126)*pix.r + INT32_C(7152)*pix.g + INT32_C(722)*pix.b)/10000);

Alternative 3: use scaled integers (perhaps using only 16-bit math)

bt_601 = (uint8_t) ((54u*pix.r + 183u*pix.g + 18u*pix.b + 128u) >> 8);

Alternative 4: use scaled integers

#define GR ((uint_least32_t)(0.2126*0x10000)) #define GG ((uint_least32_t)(0.7152*0x10000)) #define GB ((uint_least32_t)(0.0722*0x10000)) bt_601 = (uint8_t) ((GR*pix.r + GG*pix.g + GB*pix.b + 0x8000u) >> 16);

Types

Rather than int in reading/writing the image, consider (u)intN_t.

I'd favor unsigned over signed types where practical for pixel manipulation.

Needed code?

The 2 fflush(stdout); look unnecessary in a loop. Consider moving outside the loop.

\$\endgroup\$
4
  • \$\begingroup\$ IMHO sizeof( Pixel ) is more readable than sizeof image->pixels[0]. The risk of pixels becoming a non-Pixel type is not worth the loss. \$\endgroup\$ Commented Nov 16, 2023 at 1:33
  • 2
    \$\begingroup\$ @Kingsley To know sizeof( Pixel ) is the correct size takes research in the code. image->pixels = malloc(sizeof image->pixels[0] * width * height); is correct on its face. Hence the easier to code right and review. Maintenance is more costly than original writing. Code to make maintenance easier is more efficient overall even if "sizeof( Pixel ) is more readable" for you. \$\endgroup\$ Commented Nov 16, 2023 at 4:16
  • \$\begingroup\$ You make some good points. But code is read many times more often than it is modified. So I think the readability is paramount. It's takes as much research to determine the sizeof image->pixels[0], as it does Pixel. \$\endgroup\$ Commented Nov 20, 2023 at 2:30
  • \$\begingroup\$ @Kingsley The key difference is that image->pixels = malloc(sizeof image->pixels[0] * width * height); is correct without searching other code, is directly readable/reviewable and remains correct should the type of image->pixels[0] change. image->pixels = malloc(width*height*sizeof(Pixel)); require a search to know sizeof(Pixel) is the correct size for image->pixels and should the type change, this line of code needs an update. Note the other highly rated answer has "consider allocating to the referenced object, and not the type. ". Your call. \$\endgroup\$ Commented Nov 20, 2023 at 5:28
5
\$\begingroup\$

The other contributors made an excellent review of your code. I just want to comment on the philosophy or rationale behind this library choices.

Let's talk about the aim of computing a Sobel's filter: you probably don't want an image, but a matrix of values in floating point.

Why 3 channels?

When I load a grayscale image, I want a single matrix with a single channel. When I load an RGB image I want an image with three channels.

In the end why do you store your data as integers? For performance? I don't think it's so important with respect to writing your own library. Use float or even double and allow for arbitrary channels in your pixels.

Don't use a structure for RGB. It's much simpler to use an array of elements, so that you can iterate over the channels.

Then move to ownership. Are you sure that you want an image to always be owner of the raster? Sometimes the image just holds a pointer which in fact belongs to a GUI or camera driver, so it's better to account for this possibility with a flag which will then be extremely useful when destroying your image.

For Sobel's filter you have the matrix of values and the image version of it. Say that you want to compute Harris' corners: you'll definitely need the raw floating point values instead of the byte shifted ones.

This is more a stream of consciousness then a real review, but I still think that you should take a lot of this design decisions into account.

\$\endgroup\$
4
  • \$\begingroup\$ One needs to reach the mastery level to state such things. Best and most valuable feedback I ever read through Code Review. \$\endgroup\$ Commented Nov 16, 2023 at 10:35
  • \$\begingroup\$ Floating-point is rarely what's needed in image-processing. Fixed-point is far more useful, as it has the property of constant precision across the whole representable range. \$\endgroup\$ Commented Nov 18, 2023 at 9:29
  • \$\begingroup\$ @TobySpeight We are talking about a self built library for learning purposes. The lack of fixed point support in C will make it really impractical to use it for this purpose. Then, you look to OpenCV, which is pretty common, and you have floating point. As another example, all image processing going to pyTorch needs to be converted to floating point. \$\endgroup\$ Commented Nov 20, 2023 at 8:58
  • \$\begingroup\$ eh??? C has lots of fixed-point types (they are called integers). You just need to scale values appropriately to use them, and this is very common in image processing. \$\endgroup\$ Commented Nov 20, 2023 at 9:25
4
\$\begingroup\$

Everyone has pretty good answers here already.

But I'd like to add -

There's no concept of a negatively sized image, so consider use unsigned integer sizes, or size_t.

You may not want to willingly accept the dimensions of an image and proceed straight to allocation. I might have sent you a purposefully corrupted image with internal sizes of huge by huge, and/or maybe not enough pixel data. A quick sanity check against sensible internal limits might be a worthwhile addition.

\$\endgroup\$
1
  • \$\begingroup\$ That "or" should be "such as", because size_t is an unsigned integer type. \$\endgroup\$ Commented Nov 18, 2023 at 9:27
4
\$\begingroup\$

I second Cris Luengo's recommendations especially regarding API design. Additionally:

Back in the day I wrote C, it was frowned upon to make a function that allocates space and does something, without freeing that space later on. So, I was told functions should be generally like X (Image input, Image output). Otherwise you can easily imagine that you will do a sequence of image processing steps and forget to free the intermediate results, ending with memory leak. Not a big deal for user processing few images, makes it useless for industrial process control. Harith already found some leaks using Valgrind.

uint8 is problematic with many cameras and displays having higher bit depths than that. You should use something larger. You can always convert to something smaller on output later. Now, what to choose? I don't know. uint16 is large enough for essentially all devices, but I don't think it the best format internally. I suggest int or float or double, depending on the target segment - industrial or consumer. In addition to larger range, it is easier if you internally allow negative numbers for most filters and background subtraction.

grayscale vs perceptual_grayscale: I would merge the both, the purpose is the same, getting grayscale from RGB image. I suggest parameter weights to select which kind of transformation you want. Say weights of [1, 1, 1] would correspond to your grayscale while [21,72,7] would be perceptual one. This also allows users having some weird formats (eg rgbw or rgby) use your function, just with 4 weird weights. Make sure to ignore extra weights and consider missing ones as 0. Alternative is enum that would pick these weights - nice as you need to think less which numbers you require, but less flexible.

Convolution should be done to calculate only internal pixels - where kernel is fully on the image. Then pad the edges to get back to original image size. Those edge pixels will be wrong anyway and this is the simplest and most performant way to get the output.

\$\endgroup\$
2
\$\begingroup\$

You have some great reviews of the C code, so I'll look at the compilation script:

#!/bin/bash gcc improc.c main.c -O3 -g -Wall -Wpedantic -lm -Wextra -o main

Although this works, it's not ideal, and will become less so as the program grows larger.

One of the benefits of separating your code into different translation units is that when we make changes, we only need to recompile the sources that have changed. But this script compiles everything from scratch, whether that's necessary or not.

We have a tool called Make that was created exactly for determining which files are older than their corresponding sources, and building just those files, in the correct order.

I'll build up a basic Makefile for this program, assuming GNU Make (which is universal and recommended).

(Important note: Stack Exchange converts tabs to spaces in posts here, so understand that lines with leading whitespace should actually have tab rather than spaces in your Makefile. It won't work otherwise.)

I'll start with how we make the main program, given the object files:

main: main.o improc.o gcc -o main main.o improc.o -lm

What this rule says is that main needs to be built if it doesn't exist, or if it's older than either of the object files. That's followed by a command (prefixed with tab) which will link those object files.

Make provides some automatic variables which we can use here - specifically, $@ for the target and $^ for its dependencies - to make the command simpler and more reusable:

main: main.o improc.o gcc -o $@ $^ -lm

In fact, because it's so reusable, it's one of Make's built-in rules so we only need write the dependency line and set a variable to tell it about the library we want to include:

LDLIBS = -lm main: main.o improc.o

Next, we need to tell Make how to build the object files from the C sources. Again, there's a built-in rule, so we just need to set the CFLAGS variable to pass our compilation options. But we also need to rebuild if the header file is modified, so we include that in the dependency:

CFLAGS += -Wall -Wextra -Wpedantic CFLAGS = -O3 -g main.o: main.c improc.h improc.o: improc.c improc.h

In fact, we don't need to say that main.o depends on main.c or improc.o on improc.c, as that's part of the built-in rule.

I'll add a few extra warnings that I find helpful, and then we end up with

CFLAGS += -std=c17 CFLAGS += -Wall -Wextra -Wpedantic CFLAGS += -Wwrite-strings -Warray-bounds CFLAGS += -Wmissing-braces CFLAGS += -Wconversion CFLAGS += -Wstrict-prototypes -fanalyzer CFLAGS += -O3 -g LDLIBS = -lm main: improc.o main.o: improc.h improc.o: improc.h

More advanced, we can use GCC preprocessor options to have the compiler produce the dependency lines for us:

CFLAGS += -MMD -MP -include $(wildcard *.d)

The final Makefile I ended up with (adding a couple more features which you can investigate for yourself) looks like this:

CFLAGS += -std=c17 CFLAGS += -Wall -Wextra -Wpedantic CFLAGS += -Wwrite-strings -Warray-bounds CFLAGS += -Wmissing-braces CFLAGS += -Wconversion CFLAGS += -Wstrict-prototypes -fanalyzer CFLAGS += -O3 -g CFLAGS += -MMD -MP LDLIBS = -lm main: improc.o clean: $(RM) *.o *~ main .PHONY: clean .DELETE_ON_ERROR: -include $(wildcard *.d)
\$\endgroup\$
2
  • \$\begingroup\$ Wouldn’t you have main depend on the source file? main: improc.c — It’s been many years since I last wrote a Makefile, I might just remember wrong. \$\endgroup\$ Commented Nov 18, 2023 at 16:06
  • 1
    \$\begingroup\$ No, main depends on the object files (explicitly on improc.o and implicitly on main.o), and the object files depend on the sources (both via the implicit rules). Make is smart enough to know that if it has to rebuild an object file, then it will need to subsequently rebuild the executable that depends on it. Good question, though. \$\endgroup\$ Commented Nov 19, 2023 at 8:45

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.