# cs50 filter(more comfortable) - Image filters in C

I've been taking cs50 for about under a month now and I've finally finished with "filter" in pset4. The code passes all the green checks on check50. Our task was to implement functions in helpers.c so that a user can apply grayscale, reflection, blur, or edge detection filters to their images. We were asked not to modify any of the function signatures or any files other than helpers.c.

My question is, how I could refactor the code in the edge detection block so that it is shorter with less repeated code? If there is a solution involving pointers, I'd much prefer that if possible! Any tips on other blocks are welcome as well. Thank you!

Here are links to the outline of cs50/pset4/filter and information on the sobel operator:
cs50/pset4/filter
Sobel

Files provided that we were asked not to modify:
bmp.h

// BMP-related data types based on Microsoft's own

#include <stdint.h>

/**
* Common Data Types
*
* The data types in this section are essentially aliases for C/C++
* primitive data types.
*
* See http://en.wikipedia.org/wiki/Stdint.h for more on stdint.h.
*/
typedef uint8_t  BYTE;
typedef uint32_t DWORD;
typedef int32_t  LONG;
typedef uint16_t WORD;

/**
*
* and layout of a file that contains a DIB [device-independent bitmap].
*
*/
typedef struct
{
WORD   bfType;
DWORD  bfSize;
WORD   bfReserved1;
WORD   bfReserved2;
DWORD  bfOffBits;
} __attribute__((__packed__))

/**
*
* dimensions and color format of a DIB [device-independent bitmap].
*
*/
typedef struct
{
DWORD  biSize;
LONG   biWidth;
LONG   biHeight;
WORD   biPlanes;
WORD   biBitCount;
DWORD  biCompression;
DWORD  biSizeImage;
LONG   biXPelsPerMeter;
LONG   biYPelsPerMeter;
DWORD  biClrUsed;
DWORD  biClrImportant;
} __attribute__((__packed__))

/**
* RGBTRIPLE
*
* This structure describes a color consisting of relative intensities of
* red, green, and blue.
*
*/
typedef struct
{
BYTE  rgbtBlue;
BYTE  rgbtGreen;
BYTE  rgbtRed;
} __attribute__((__packed__))
RGBTRIPLE;


filter.c

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

#include "helpers.h"

int main(int argc, char *argv[])
{

// Define allowable filters
char *filters = "begr";

// Get filter flag and check validity
char filter = getopt(argc, argv, filters);
if (filter == '?')
{
fprintf(stderr, "Invalid filter.\n");
return 1;
}

// Ensure only one filter
if (getopt(argc, argv, filters) != -1)
{
fprintf(stderr, "Only one filter allowed.\n");
return 2;
}

// Ensure proper usage
if (argc != optind + 2)
{
fprintf(stderr, "Usage: filter [flag] infile outfile\n");
return 3;
}

// Remember filenames
char *infile = argv[optind];
char *outfile = argv[optind + 1];

// Open input file
FILE *inptr = fopen(infile, "r");
if (inptr == NULL)
{
fprintf(stderr, "Could not open %s.\n", infile);
return 4;
}

// Open output file
FILE *outptr = fopen(outfile, "w");
if (outptr == NULL)
{
fclose(inptr);
fprintf(stderr, "Could not create %s.\n", outfile);
return 5;
}

// Ensure infile is (likely) a 24-bit uncompressed BMP 4.0
if (bf.bfType != 0x4d42 || bf.bfOffBits != 54 || bi.biSize != 40 ||
bi.biBitCount != 24 || bi.biCompression != 0)
{
fclose(outptr);
fclose(inptr);
fprintf(stderr, "Unsupported file format.\n");
return 6;
}

int height = abs(bi.biHeight);
int width = bi.biWidth;

// Allocate memory for image
RGBTRIPLE(*image)[width] = calloc(height, width * sizeof(RGBTRIPLE));
if (image == NULL)
{
fprintf(stderr, "Not enough memory to store image.\n");
fclose(outptr);
fclose(inptr);
return 7;
}

int padding = (4 - (width * sizeof(RGBTRIPLE)) % 4) % 4;

// Iterate over infile's scanlines
for (int i = 0; i < height; i++)
{
// Read row into pixel array

}

// Filter image
switch (filter)
{
// Blur
case 'b':
blur(height, width, image);
break;

// Edges
case 'e':
edges(height, width, image);
break;

// Grayscale
case 'g':
grayscale(height, width, image);
break;

// Reflect
case 'r':
reflect(height, width, image);
break;
}

// Write new pixels to outfile
for (int i = 0; i < height; i++)
{
// Write row to outfile
fwrite(image[i], sizeof(RGBTRIPLE), width, outptr);

// Write padding at end of row
for (int k = 0; k < padding; k++)
{
fputc(0x00, outptr);
}
}

// Free memory for image
free(image);

// Close infile
fclose(inptr);

// Close outfile
fclose(outptr);

return 0;
}


My helpers.c

#include "helpers.h"
#include <stdio.h>
#include <ctype.h>
#include <math.h>
#include <string.h>

// Prototypes
void swap(RGBTRIPLE *a, RGBTRIPLE *b);

// Convert image to grayscale
void grayscale(int height, int width, RGBTRIPLE image[height][width])
{
// Iterate through the height or also known as each row
for (int i = 0; i < height; i++)
{
// Iterate through the width or also known as each pixel/column
for (int j = 0; j < width; j++)
{
// Calculate the average of the R, G, and B values and round to nearest integer
int average = round(((double) image[i][j].rgbtBlue + (double) image[i][j].rgbtGreen + (double) image[i][j].rgbtRed) / 3);
// Set the values of R, G, and B to the average, making them the same, to produce the correct shade of gray
image[i][j].rgbtBlue = average;
image[i][j].rgbtGreen = average;
image[i][j].rgbtRed = average;
}
}
return;
}

// Reflect image horizontally
void reflect(int height, int width, RGBTRIPLE image[height][width])
{
// Iterate through the height or also known as each row
for (int i = 0; i < height; i++)
{
// Iterate through the width or also known as each pixel/column
for (int j = 0; j < width; j++)
{
// Perform swap up until the middle
if (j < width / 2)
{
swap(&image[i][j], &image[i][width - (j + 1)]);
}
}
}
return;
}

// Function to swap two elements
void swap(RGBTRIPLE *a, RGBTRIPLE *b)
{
RGBTRIPLE temp;
temp = *a;
*a = *b;
*b = temp;
}

// Blur image
void blur(int height, int width, RGBTRIPLE image[height][width])
{
// Initialize copy of image
RGBTRIPLE temp[height][width];

// Make a copy of image to preserve original values
for (int i = 0; i < height; i++)
{
for (int j = 0; j < width; j++)
{
temp[i][j] = image[i][j];
}
}

// Iterate through the height or also known as each row
for (int i = 0; i < height; i++)
{
// Iterate through the width or also known as each pixel/column
for (int j = 0; j < width; j++)
{
// Variable that counts how many numbers added to arrive at the sum
int count = 0;

// Sum variables for each colour
double sum_blue = 0;
double sum_green = 0;
double sum_red = 0;

// Loop to check the surrounding pixels within 1 column and 1 row
for (int k = i - 1; k <= i + 1; k++)
{
for (int l = j - 1; l <= j + 1; l++)
{
// Only adds pixels that are within the image boundaries
if (k >= 0 && l >= 0 && k < height && l < width)
{
sum_blue += temp[k][l].rgbtBlue;
sum_green += temp[k][l].rgbtGreen;
sum_red += temp[k][l].rgbtRed;
count++;
}
}
}
// Use the averages from the surrounding pixels and set the new colour values for the iterated pixel
image[i][j].rgbtBlue = round(sum_blue / count);
image[i][j].rgbtGreen = round(sum_green / count);
image[i][j].rgbtRed = round(sum_red / count);
}
}
return;
}

// Detect edges
void edges(int height, int width, RGBTRIPLE image[height][width])
{
RGBTRIPLE temp[height][width];

// Make copy of image
for (int i = 0; i < height; i++)
{
for (int j = 0; j < width; j++)
{
temp[i][j] = image[i][j];
}
}

// Sobel Operator matrices for Gx and Gy
int kernel_Gx[3][3] = {{-1, 0, 1}, {-2, 0, 2}, {-1, 0, 1}};
int kernel_Gy[3][3] = {{-1, -2, -1}, {0, 0, 0}, {1, 2, 1}};

// Iterate through the height or also known as each row
for (int i = 0; i < height; i++)
{
// Iterate through the width or also known as each pixel/column
for (int j = 0; j < width; j++)
{
// Initialize values for weighted sums in the x direction
double gx_blue = 0;
double gx_green = 0;
double gx_red = 0;
// Initialize values for weighted sums in the y direction
double gy_blue = 0;
double gy_green = 0;
double gy_red = 0;
// Counter to detect what row of the 3x3 the loop is iterating
int row = 0;

// Loop to check the surrounding pixels within 1 row
for (int k = i - 1; k <= i + 1; k++)
{
// Counter to detect what column of the 3x3 grid the loop is iterating
int column = 0;
// Loop to check the surrounding pixels within 1 column
for (int l = j - 1; l <= j + 1; l++)
{
// Only adds pixels that are within the image boundaries
if (k >= 0 && l >= 0 && k < height && l < width)
{
// Calculate Gx
gx_blue += (kernel_Gx[row][column] * temp[k][l].rgbtBlue);
gx_green += (kernel_Gx[row][column] * temp[k][l].rgbtGreen);
gx_red += (kernel_Gx[row][column] * temp[k][l].rgbtRed);
// Calculate Gy
gy_blue += (kernel_Gy[row][column] * temp[k][l].rgbtBlue);
gy_green += (kernel_Gy[row][column] * temp[k][l].rgbtGreen);
gy_red += (kernel_Gy[row][column] * temp[k][l].rgbtRed);
}
column++;
}
row++;
}
// Combine Gx and Gy
int sobel_blue = round(sqrt(pow(gx_blue, 2) + pow(gy_blue, 2)));
int sobel_green = round(sqrt(pow(gx_green, 2) + pow(gy_green, 2)));
int sobel_red = round(sqrt(pow(gx_red, 2) + pow(gy_red, 2)));

// Set the new colour values for the iterated pixel and cap at 255 if necessary
image[i][j].rgbtBlue = (sobel_blue > 255) ? 255 : sobel_blue;
image[i][j].rgbtGreen = (sobel_green > 255) ? 255 : sobel_green;
image[i][j].rgbtRed = (sobel_red > 255) ? 255 : sobel_red;
}
}
return;
}

• khoaHyh, why does code use LONG instead of int32_t? Sep 16, 2020 at 23:57
• @chux-ReinstateMonica: note the typedef int32_t LONG in one of the files we were asked not to modify.) Sep 17, 2020 at 6:39

• I strongly recommend to declare temp as RGBTRIPLE[hight + 2][width + 2], and initialize the border pixels to 0, 0, 0. This would eliminate the need to test for pixels that are within the image boundaries.

• Initializing row and column inside the for loop makes it more clear how the are used, and that they go lockstep with k and l respectively. Consider

  for (int k = i - 1, row = 0; k <= i + 1; k++, row++)


and

      for (int l = j - 1, column = 0; l <= j + 1; l++, column++)

• No naked loops please. Every time you feel compelled to add a comment like Make a copy of image, consider factoring the applicable code into a properly named function, e.g. make_copy_of_image(....).

Ditto for Calculate_Gx and Calculate_Gy. These blocks only differ in which kernel they use. Pass it as a parameter.

Note that gx_blue, gx_green, gx_red are crying to be an RGBTRIPLE.

• This was very educational, I appreciate it a lot. Thank you! Sep 17, 2020 at 14:04

Bug: Careful how you open

A common problem when near windows is opening a binary file in text mode. Avoid end-of-line translations and open in binary.

// FILE *inptr = fopen(infile, "r");
FILE *inptr = fopen(infile, "rb");


If the fread() fails, error out. Also recommend to size to the object and not the type. Easier to code right, review and maintain.

// fread(&bf, sizeof(BITMAPFILEHEADER), 1, inptr);
if (fread(&bf, sizeof bf, 1, inptr) != 1) {
Handle_error();
}


No need for floating point in averaging

    // double sum_blue = 0;
long long sum_blue = 0;
for (int k = i - 1; k <= i + 1; k++)
for (int l = j - 1; l <= j + 1; l++)
sum_blue += temp[k][l].rgbtBlue;
...
// round by adding half the count
image[i][j].rgbtBlue = (sum_blue + count/2)/ count;
// image[i][j].rgbtBlue = round(sum_blue / count);


and

// int average = round(((double) image[i][j].rgbtBlue + (double) image[i][j].rgbtGreen + (double) image[i][j].rgbtRed) / 3);
int average = (image[i][j].rgbtBlue + image[i][j].rgbtGreen +
image[i][j].rgbtRed + 1) / 3;


User loops vs. memcpy()

Library functions tend to be faster than equivalent user code.

RGBTRIPLE temp[height][width];

// Make a copy of image to preserve original values
//for (int i = 0; i < height; i++) {
//    for (int j = 0; j < width; j++) {
//        temp[i][j] = image[i][j];
//    }
//}
memcpy(temp, image, sizeof temp);