CS50 pset4 filter - Image Filters in C

Question 1

I started the CS50 and am at pset4. I completed both the less and more parts for the filter app that takes an image and applies a filter to it. In this pset we need to write the filters code only(greyscale, sepia, reflect, blur and edges). The edges function should apply the Sobel operator to an image so that it detects its edges.

It works by taking each pixel and modifying it based on the 3x3 grid of pixels that surrounds that pixel. Each neighbour is multiplied by the its correspondent Gx kernel value and added to the sum. The same is done for Gy. the middle pixel is also included. In this case bmp images are used and are represented by a 2d array of values for Red Green and Blue. So we need to do the process above for all three colour channels. The new pixels get the value of the square root of Gx^2 + Gy^2 to a max of 255; Pixels past the edge should be treated as solid black (0 0 0 RGB value);

Gx and Gy kernels: enter image description here

Link if not very clear explanation : https://cs50.harvard.edu/x/2020/psets/4/filter/more

After 2 days I completed the task but I am sure my way is not the best way to do it, so I'm asking for some improvements that can be implement by a beginer.

Files bmp.h and filter.c were written for us and we are not allowed to modify them in any way. Code for the 2 files below.

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.
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/cc230309.aspx.
 * 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;
/**
 * BITMAPFILEHEADER
 *
 * The BITMAPFILEHEADER structure contains information about the type, size,
 * and layout of a file that contains a DIB [device-independent bitmap].
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183374(VS.85).aspx.
 */
typedef struct
{
 WORD bfType;
 DWORD bfSize;
 WORD bfReserved1;
 WORD bfReserved2;
 DWORD bfOffBits;
} __attribute__((__packed__))
BITMAPFILEHEADER;
/**
 * BITMAPINFOHEADER
 *
 * The BITMAPINFOHEADER structure contains information about the
 * dimensions and color format of a DIB [device-independent bitmap].
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183376(VS.85).aspx.
 */
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__))
BITMAPINFOHEADER;
/**
 * RGBTRIPLE
 *
 * This structure describes a color consisting of relative intensities of
 * red, green, and blue.
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/aa922590.aspx.
 */
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;
 }
 // Read infile's BITMAPFILEHEADER
 BITMAPFILEHEADER bf;
 fread(&bf, sizeof(BITMAPFILEHEADER), 1, inptr);
 // Read infile's BITMAPINFOHEADER
 BITMAPINFOHEADER bi;
 fread(&bi, sizeof(BITMAPINFOHEADER), 1, inptr);
 // 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;
 }
 // Determine padding for scanlines
 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
 fread(image[i], sizeof(RGBTRIPLE), width, inptr);
 // Skip over padding
 fseek(inptr, padding, SEEK_CUR);
 }
 // 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 outfile's BITMAPFILEHEADER
 fwrite(&bf, sizeof(BITMAPFILEHEADER), 1, outptr);
 // Write outfile's BITMAPINFOHEADER
 fwrite(&bi, sizeof(BITMAPINFOHEADER), 1, outptr);
 // 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;
}

I created the functions for the filters in the file bellow

helpers.c

#include "helpers.h"
#include <math.h>
void grayscale(int height, int width, RGBTRIPLE image[height][width])
{
 //declare a variable average to store the average colour
 float average;
 //loop through image rows
 for (int i = 0; i < height; i++)
 {
 //loop through image pixels
 for (int j = 0; j < width; j++)
 {
 //calculate average value for each pixel
 average = 1.0 * (image[i][j].rgbtBlue + image[i][j].rgbtGreen + image[i][j].rgbtRed) / 3;
 //assign the average value to all values of each pixel
 image[i][j].rgbtBlue = round(average);
 image[i][j].rgbtGreen = round(average);
 image[i][j].rgbtRed = round(average);
 }
 }
}
// Reflect image horizontally
void reflect(int height, int width, RGBTRIPLE image[height][width])
{
 //loop through image rows
 for (int i = 0; i < height; i++)
 {
 //loop through image pixels
 for (int j = 0; j < width / 2; j++)
 {
 int swapBlue = image[i][j].rgbtBlue;
 int swapGreen = image[i][j].rgbtGreen;
 int swapRed = image[i][j].rgbtRed;
 image[i][j].rgbtBlue = image[i][width - j - 1].rgbtBlue;
 image[i][j].rgbtGreen = image[i][width - j - 1].rgbtGreen;
 image[i][j].rgbtRed = image[i][width - j - 1].rgbtRed;
 image[i][width - j - 1].rgbtBlue = swapBlue;
 image[i][width - j - 1].rgbtGreen = swapGreen;
 image[i][width - j - 1].rgbtRed = swapRed;
 }
 }
}
// Blur image
void blur(int height, int width, RGBTRIPLE image[height][width])
{
 //initialise a variable average for each colour, a new array to copy to and a counter
 float averageGreen;
 float averageRed;
 float averageBlue;
 RGBTRIPLE image2[height][width];
 int count;
 //loop through array
 for (int i = 0; i < height; i++)
 {
 for (int j = 0; j < width; j++)
 {
 //set counter and averages to 0
 count = 0;
 averageBlue = 0;
 averageGreen = 0;
 averageRed = 0;
 //loop to get pixels above and below height
 for (int r = i - 1; r <= i + 1 && r < height; r++)
 {
 //if index out of bounds make it 0
 if (r < 0)
 {
 continue;
 }
 //loop to get pixels to the left and right of width
 for (int c = j - 1; c <= j + 1 && c < width; c++)
 {
 //if index out of bounds make it 0
 if (c < 0)
 {
 continue;
 }
 //add values to the average and increment count
 averageBlue += image[r][c].rgbtBlue;
 averageGreen += image[r][c].rgbtGreen;
 averageRed += image[r][c].rgbtRed;
 count++;
 }
 }
 //udpdate copy array with average values divided by counter
 image2[i][j].rgbtBlue = round(averageBlue / count);
 image2[i][j].rgbtGreen = round(averageGreen / count);
 image2[i][j].rgbtRed = round(averageRed / count);
 }
 }
 for (int i = 0; i < height; i++)
 {
 for (int j = 0; j < width; j++)
 {
 image[i][j] = image2[i][j];
 }
 }
}
// Detect edges
void edges(int height, int width, RGBTRIPLE image[height][width])
{
 //delcare variables for Gx and Gy to calculate values for each colour
 int GxBlue;
 int GxGreen;
 int GxRed;
 int GyBlue;
 int GyGreen;
 int GyRed;
 //initialise the Gx and Gy kernels
 int Gx[3][3] =
 {
 {-1, 0, 1},
 {-2, 0, 2},
 {-1, 0, 1}
 };
 int Gy[3][3] =
 {
 {-1, -2, -1},
 {0, 0, 0},
 {1, 2, 1}
 };
 //delcare new array of bigger size to pad original array with 0's
 RGBTRIPLE image2[height + 2][width + 2];
 //delcare new array to store new values
 RGBTRIPLE image3[height][width];
 //loop big aray to add 0 padding and original array
 for (int i = 0 ; i < height + 2; i ++)
 {
 for (int j = 0; j < width + 2; j++)
 {
 //if on edges add 0
 if (i == 0 || j == 0 || i == height + 1 || j == width + 1)
 {
 image2[i][j].rgbtBlue = 0;
 image2[i][j].rgbtGreen = 0;
 image2[i][j].rgbtRed = 0;
 }
 //else add original array values
 if (i > 0 && i < height + 1 && j > 0 && j < width + 1)
 {
 image2[i][j].rgbtBlue = image[i - 1][j - 1].rgbtBlue;
 image2[i][j].rgbtGreen = image[i - 1][j - 1].rgbtGreen;
 image2[i][j].rgbtRed = image[i - 1][j - 1].rgbtRed;
 }
 }
 }
 //loop inner array
 for (int i = 1; i < height + 1; i++)
 {
 for (int j = 1; j < width + 1; j++)
 {
 //initialise variables to 0 every time we switch pixel
 GxBlue = 0;
 GxGreen = 0;
 GxRed = 0;
 GyBlue = 0;
 GyGreen = 0;
 GyRed = 0;
 //loop all neighbours of inner array
 for (int row = i - 1; row <= i + 1; row++)
 {
 for (int col = j - 1; col <= j + 1; col++)
 {
 //add values of the neighbours multiplied to the corresponded Gx and Gy values to the sum
 GxBlue += image2[row][col].rgbtBlue * Gx[row - i + 1][col - j + 1];
 GxGreen += image2[row][col].rgbtGreen * Gx[row - i + 1][col - j + 1];
 GxRed += image2[row][col].rgbtRed * Gx[row - i + 1][col - j + 1];
 GyBlue += image2[row][col].rgbtBlue * Gy[row - i + 1][col - j + 1];
 GyGreen += image2[row][col].rgbtGreen * Gy[row - i + 1][col - j + 1];
 GyRed += image2[row][col].rgbtRed * Gy[row - i + 1][col - j + 1];
 }
 }
 //make sure each value does not exceed 255
 //calculte the square root of the squared sums
 if (sqrt(GxBlue * GxBlue + GyBlue * GyBlue) > 255)
 {
 image3[i - 1][j - 1].rgbtBlue = 255;
 }
 else
 {
 image3[i - 1][j - 1].rgbtBlue = round(sqrt(GxBlue * GxBlue + GyBlue * GyBlue));
 }
 if (sqrt(GxGreen * GxGreen + GyGreen * GyGreen) > 255)
 {
 image3[i - 1][j - 1].rgbtGreen = 255;
 }
 else
 {
 image3[i - 1][j - 1].rgbtGreen = round(sqrt(GxGreen * GxGreen + GyGreen * GyGreen));
 }
 if (sqrt(GxRed * GxRed + GyRed * GyRed) > 255)
 {
 image3[i - 1][j - 1].rgbtRed = 255;
 }
 else
 {
 image3[i - 1][j - 1].rgbtRed = round(sqrt(GxRed * GxRed + GyRed * GyRed));
 }
 }
 }
 //copy values in original array
 for (int i = 0; i < height; i++)
 {
 for (int j = 0; j < width; j++)
 {
 image[i][j].rgbtBlue = image3[i][j].rgbtBlue;
 image[i][j].rgbtGreen = image3[i][j].rgbtGreen;
 image[i][j].rgbtRed = image3[i][j].rgbtRed;
 }
 }
}

If you want to see the effects of the edge filter:

original picture: original

picture after filter: filtered

So I have 2 questions:

Is there any way of writing the edge function code cleaner and more efficient for a beginer
In the blur function it took me waaaaaay to long to make all the for loops work and I almost gave up. Is there a good way to find write this type of loops easier if I clearly know what to do(ie: I know I need to access the i - 1 to i + 1 element but how do I write this without going out of bounds). I could probably rewrite the function using the 0 padding as well, like for the edge one but this seems cleaner.

3.For the edge function I admit the padding with 0 on edges on a bigger array idea isn't mine and I found it on stackoverflow, but it wasn't implemented there, so I just used the idea. Is it a bad practice or something that is usually done?

Thanks you and sorry if unclear.

PS: my first post here :)

Question 2

Thanks for letting me know. I've updated it and hope it's better this time!

Question 3

What a cool topic!

Here are my comments on your code:

Please post everything next time! You didn't post helpers.h, so I made my own helpers.h out of helpers.c and some guesses:

#include <math.h>
#include "bmp.h"
void grayscale(int height, int width, RGBTRIPLE image[height][width]);
void reflect(int height, int width, RGBTRIPLE image[height][width]);
void blur(int height, int width, RGBTRIPLE image[height][width]);
void edges(int height, int width, RGBTRIPLE image[height][width]);

Hopefully I'm not missing anything! But this compiles, and even with -Wall -Wextra -pedantic there are no warnings. Nice :)

I tried the Sobel filter on a few BMPs. It seems to mostly work (yay!), but I got a segmentation fault on the large ish images I tried (few MB). That's because you do this:

RGBTRIPLE image2[height + 2][width + 2];

This will take up a lot of stack space -- too much for my computer to handle. A better idea is to allocate your images on the heap. Look at filter.c for an example. You want something like:

RGBTRIPLE(*image2)[width + 2] = calloc(height + 2, sizeof(RGBTRIPLE) * (width + 2));

This makes the big BMPs work! You'll also have to free this memory since it's being allocated on the heap.

In the function greyscale:

float average;
....
average = 1.0 * (image[i][j].rgbtBlue + image[i][j].rgbtGreen + image[i][j].rgbtRed) / 3;

A lot of C programmers declare all of their variables at the beginning of functions. C89 required that local variables be declared at the beginning of the block they live in, and some people took that to the extreme and declared stuff as far up as they could go.

In 2020, you don't have to do this. You haven't followed C89 in a lot of other places anyway (compile with -std=c89 to see the 60 warnings and 3 errors). So you might as well do:

float average = ....

This way you avoid uninitialized variables, and you will never have to scroll up/down/up/down... while reading your C code.

As a nit, I think instead of 1.0 * (...) / 3 you could just as easily have written (...) / 3.0. Or better yet, just do integer division. You're about to round anyway. Does it matter if you are 0.33/255 off?

Either way, if you're going to round average, you might as well make it an int and then call round once instead of calling round(average) three times.

In the function reflect:

How about making a function void swap(BYTE* lhs, BYTE* rhs, int size);? Then you can write

for (int i ....) {
 for (int j .....) {
 swap(image[i][j], image[i][width - j - 1], sizeof(RGBTRIPLE));
 }
}

instead of 9 bulky lines of code.

In the function blur:

float averageGreen;

Same comment as before about initializing at the top of functions.

RGBTRIPLE image2[height][width];

Same comment about stack vs. heap. This will fail for large images. Also, how about a better name like "blurredImage" or something to that effect. Most of your names are good, but it's a bad sign when you have to use a counter to distinguish your variables in human readable code.

for (int r = i - 1; r <= i + 1 && r < height; r++) 
{
 if (r < 0)
 {
 continue;
 }

This breaks my brain. Cognitive overload. No wonder you wrote "it took me way too long to make all the for loops work."

it looks like you did get it right (as far as I can tell...), so good job. Even though it's not as elegant as it could be, it's good you stuck with it.
As you write more code, you'll get used to writing loops/boolean logic, and this will get easier. That said there are some guidelines to make this easier.

I think my brain blew while I read this because you wrote && r < height in the for loop's condition and then put if (r < 0) in an if statement. You didn't separate concerns well enough.

Your first concern is to find i and j.

Your second concern is to find how far from i and j you need to stray.

And your final concern is to avoid going out of bounds.

How does this look?

for (int i = 0; i < height; ++i) {
 for (int j = 0; j < width; ++j) {
 for (int i_offset = -1; i_offset <= -1; ++i_offset) {
 for (int j_offset = -1; j_offset <= -1; ++j_offset) {
 int row = i + i_offset;
 int col = j + j_offset;
 if (0 <= row && row < height && ...) {
 ...
 }
 }
 }
 }
}

I think this is relatively easy to read.

Some would argue this is less efficient since you'll run through all the col values when row is out of bounds. Don't listen to these arguments though -- the compiler can hoist these checks for you (at least Clang 10.0.0 with -O does this). As a rule, write the clear thing and only optimize manually if you have to.

I could probably rewrite the function using the 0 padding as well, like for the edge one but this seems cleaner.

I agree that in this case it's cleaner not to use 0 padding. How could you figure out count if you use 0 padding?

for (int i = 0; i < height; i++)
{
 for (int j = 0; j < width; j++)
 {
 image[i][j] = image2[i][j];
 }
}

You basically want to do image = image2. This can be done more elegantly.

First idea (very common idiom): use memcpy. It will be faster and it's easier to read.

Second idea (maybe controversial):

swap(image, image2); // swap the pointers
free(image2); // free the original image

Faster and simpler! This only works if you know you can call free on the memory passed into the function -- that's a strong assumption! In this case you can (assuming you fix your declaration of image2 to use calloc or similar), but you should at the very least add a comment to the function definition saying you plan to free the argument.

In the function edges:

int GxBlue;

Same comment.

//initialise the Gx and Gy kernels

Good to see Gx and Gy written out so clearly! An uglier implementation would have -1 * blah + 1 * bleh + .... Good that you didn't do that.

RGBTRIPLE image2[height + 2][width + 2];

Same comment.

//if on edges add 0

If you use calloc, then you can assume the memory you receive is zeroed out.

//else add original array values
if (i > 0 && i < height + 1 && j > 0 && j < width + 1)

How about just else instead of this comment/a written out negation of the condition? This isn't just a style tip -- you don't want to make it hard for the compiler to prove it only needs to compute the condition once.

if (sqrt(GxBlue * GxBlue + GyBlue * GyBlue) > 255)
{
 image3[i - 1][j - 1].rgbtBlue = 255;
}
else
{
 image3[i - 1][j - 1].rgbtBlue = round(sqrt(GxBlue * GxBlue + GyBlue * GyBlue));
}

How about

image3[i - 1][j - 1].rgbtBlue = edgeHelper(GxBlue);

Where edgeHelper is a function that looks like:

BYTE edgeHelper(float Gx, float Gy) {
 float magnitude = round(euclideanDistance(Gx, Gy));
 return magnitude > 255 ? 255 : magnitude; 
}

Note that I've rounded before doing the comparison rather than after. I don't think it matters much, but it's a difference all the same. You'll have to figure out euclideanDistance but I think you'll have no issue with it!

image[i][j].rgbtBlue = image3[i][j].rgbtBlue;
image[i][j].rgbtGreen = image3[i][j].rgbtGreen;
image[i][j].rgbtRed = image3[i][j].rgbtRed;

Earlier in your program you wrote image[i][j] = image2[i][j] .. that was better than this! but the same comments apply regardless. Since you [height + 2][width + 2] and relied on having 0s in memory rather than doing bound checks, you can no longer do that swap/free hack.

Architectural tip:

How about adding (and using) these:

// could store height/width in the image
struct Image { ... };
Image* createImage(int height, int width);
void destroyImage(Image*);
void swapImage(Image*, Image*);
// this could return 0 for out of bounds
RGBTRIPLE getPixel(Image*, int i, int j);
setPixel(Image*, int i, int j, RGBTRIPLE);
// this would help you get rid of a lot of the for loops!
forEachImageIndex(Image*, void(*callback)(Image*, int j, int j));

Properly using these would probably mean modifying filter.c, but maybe it's worth thinking about.

Is there any way of writing the edge function code cleaner and more efficient for a beginer

I think I already answered about cleanliness, but I'll answer here about efficiency. Your code is pretty efficient. One of the beautiful things about C is that it encourages writing simple code, and the compiler can do a really good job optimizing simple code for you. I played around with different compiler flags, and Clang can optimize/vectorize this pretty well.

For the edge function I admit the padding with 0 ... I found on stackoverflow, ... I just used the idea. Is it a bad practice or something that is usually done?

There are a few concerns here:

Can you legally copy stuff from SO? I'll link you to an answer that can explain better than I can: https://meta.stackoverflow.com/questions/339152/plagiarism-and-using-copying-code-from-stack-overflow-and-submitting-it-in-an-as.
Is it a good idea to copy ideas from SO? In my opinion, it's OK as long as you understand what you're copying. You should copy a link to what you're copying (except for the most generic ideas).
Is it a good idea to copy code from SO? Copying code is so dangerous. I would avoid it if you can.
"Is copying code from SO common" It is so common that people have made memes about it. But that doesn't make it a good idea...

TL;DR: I think what you did is OK, but you should cite SO.com somewhere.

General tips:

Learn more of stdlib.h like memcpy and when to use malloc/free.
Don't be afraid to make helper functions to avoid repetitive code.
Use a more modern C standard/more modern C style guide.
Test your code with lots of files. Seek out big ones/small ones/weird ones etc. You could have caught that segfault.
Keep practicing!

Question 4

Thank you so much for your answer and the time you invested to answer. to be honest I started learning Java and then I heard of CS50 and wanted to get a hang of C to understand memory and low level a bit better, and imo C is way harder than Java as you need to do everything yourself, but I understood a bit better how things work under the bonnet. I will indeed refactor the code for this one using your tips and see how I get along. Thank you again.

Question 5

 #pragma once
#include "helpers.h"
#include <math.h>
#include <stdlib.h>
#include <stdio.h>
void swap(BYTE* c1, BYTE* c2){
 BYTE temp;
 temp = *c1;
 *c1 = *c2;
 *c2 = temp;
}
int get_blue(int i,int j,int height,int width,int* matrix,int which,RGBTRIPLE image[height][width]){
 int sum = 0;
 int k = 0;
 int count = 0;
 for(int m = i; m < i+3 ; m++){
 for(int n = j; n < j+3; n++){
 if((m < 0) || (m > (height -1)) || (n < 0) || (n > (width - 1))){
 }
 else {
 sum = sum + (image[m][n].rgbtBlue)*(*(matrix+k));
 count++;
 }
 k++;
 }
 }
 if (which == 0)
 return sum/count;
 else
 return sum;
}
int get_green(int i,int j,int height,int width,int* matrix,int which,RGBTRIPLE image[height][width]){
 int sum = 0;
 int k = 0;
 int count = 0;
 for(int m = i; m < i+3 ; m++){
 for(int n = j; n < j+3; n++){
 if((m < 0) || (m > (height -1)) || (n < 0) || (n > (width - 1))){
 }
 else {
 sum = sum + (image[m][n].rgbtGreen)*(*(matrix+k));
 count++;
 }
 k++;
 }
 }
 if (which == 0)
 return sum/count;
 else
 return sum;
}
int get_red(int i,int j,int height,int width,int* matrix, int which,RGBTRIPLE image[height][width]){
 int sum = 0;
 int k = 0;
 int count = 0;
 for(int m = i; m < i+3 ; m++){
 for(int n = j; n < j+3; n++){
 if((m < 0) || (m > (height -1)) || (n < 0) || (n > (width - 1))){
 }
 else {
 sum = sum + (image[m][n].rgbtRed)*(*(matrix+k));
 count++;
 }
 k++;
 }
 }
 if (which == 0)
 return sum/count;
 else
 return sum;
}
// Convert image to grayscale
void grayscale(int height, int width, RGBTRIPLE image[height][width])
{
 //need to take average 
 //take each pixel to that value
 int average;
 for(int i = 0 ; i < height ; i++){
 for(int j =0 ; j < width ; j++){
 average = (image[i][j].rgbtBlue + image[i][j].rgbtGreen + image[i][j].rgbtRed)/3;
 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])
{
 int middle = width/2;
 for(int i =0 ; i < height ; i++){
 //swap image[0][0] with image[0][width - 1]
 for(int j = 0; j < middle; j++){
 swap(&image[i][j].rgbtBlue, &image[i][width-j-1].rgbtBlue);
 swap(&image[i][j].rgbtGreen, &image[i][width-j-1].rgbtGreen);
 swap(&image[i][j].rgbtRed, &image[i][width-j-1].rgbtRed);
 }
 }
 return;
}
// Blur image
void blur(int height, int width, RGBTRIPLE image[height][width])
{
 RGBTRIPLE(*newimage)[width]=calloc(height, width*sizeof(RGBTRIPLE));
 int matrix[] = {1,1,1,1,1,1,1,1,1};
 for(int i = 0; i < height ; i++){
 for(int j = 0; j < width ; j++){
 newimage[i][j].rgbtBlue = round(get_blue(i-1,j-1,height,width,matrix,0,image));
 newimage[i][j].rgbtGreen= round(get_green(i-1,j-1,height,width,matrix,0,image));
 newimage[i][j].rgbtRed = round(get_red(i-1,j-1,height,width,matrix,0,image));
 }
 }
 //copy
 for(int i = 0; i < height ; i++){
 for(int j = 0; j < width ; j++){
 image[i][j].rgbtBlue = newimage[i][j].rgbtBlue;
 image[i][j].rgbtGreen= newimage[i][j].rgbtGreen;
 image[i][j].rgbtRed= newimage[i][j].rgbtRed;
 }
 }
 free(newimage);
 return;
}
// Detect edges
void edges(int height, int width, RGBTRIPLE image[height][width])
{
 blur(height,width,image);
 int bluegx,bluegy,greengx,greengy,redgx,redgy;
 int bluegxy,greenxy,redxy;
 int gx[] = {1,0,-1,2,0,-2,1,0,-1};
 int gy[] = {1,2,1,0,0,0,-1,-2,-1};
 RGBTRIPLE(*newimage)[width]=calloc(height, width*sizeof(RGBTRIPLE));
 for(int i = 0; i < height ; i++){
 for(int j = 0; j < width ; j++){
 bluegx= get_blue(i-1,j-1,height,width,gx,1,image);
 greengx= get_green(i-1,j-1,height,width,gx,1,image);
 redgx= get_red(i-1,j-1,height,width,gx,1,image);
 bluegy= get_blue(i-1,j-1,height,width,gy,1,image);
 greengy= get_green(i-1,j-1,height,width,gy,1,image);
 redgy= get_red(i-1,j-1,height,width,gy,1,image);
 bluegxy = (round(sqrt(bluegx*bluegx + bluegy*bluegy)));
 greenxy= (round(sqrt(greengx*greengx+ greengy*greengy)));
 redxy = (round(sqrt(redgx*redgx+ redgy*redgy)));
 newimage[i][j].rgbtBlue = (bluegxy> 255)?255:bluegxy;
 newimage[i][j].rgbtGreen= (greenxy > 255)?255:greenxy;
 newimage[i][j].rgbtRed = (redxy> 255)?255:redxy;
 }
 }
 for(int i = 0; i < height ; i++){
 for(int j = 0; j < width ; j++){
 image[i][j].rgbtBlue = newimage[i][j].rgbtBlue;
 image[i][j].rgbtGreen= newimage[i][j].rgbtGreen;
 image[i][j].rgbtRed= newimage[i][j].rgbtRed;
 }
 }
 free(newimage);
 return;
}

This is the code one can implement as it contains fewer loops, and is well implemented. The result from the above code is as follows(for Sobel operator): enter image description here

Question 6

This looks like less of an answer and more of a new question. The answer makes only one clear observation about the original code less loops. It isn't clear what you mean by well implemented. The Code Review Community is about improving the original posters coding skills, not about how to solve the problem. Code only alternate solutions are considered poor answers. Please read How do I write a good answer?.

score 3 · Accepted Answer · 2020-05-02 04:50:06Z

What a cool topic!

Here are my comments on your code:

Please post everything next time! You didn't post helpers.h, so I made my own helpers.h out of helpers.c and some guesses:

#include <math.h>
#include "bmp.h"
void grayscale(int height, int width, RGBTRIPLE image[height][width]);
void reflect(int height, int width, RGBTRIPLE image[height][width]);
void blur(int height, int width, RGBTRIPLE image[height][width]);
void edges(int height, int width, RGBTRIPLE image[height][width]);

Hopefully I'm not missing anything! But this compiles, and even with -Wall -Wextra -pedantic there are no warnings. Nice :)

I tried the Sobel filter on a few BMPs. It seems to mostly work (yay!), but I got a segmentation fault on the large ish images I tried (few MB). That's because you do this:

RGBTRIPLE image2[height + 2][width + 2];

This will take up a lot of stack space -- too much for my computer to handle. A better idea is to allocate your images on the heap. Look at filter.c for an example. You want something like:

RGBTRIPLE(*image2)[width + 2] = calloc(height + 2, sizeof(RGBTRIPLE) * (width + 2));

This makes the big BMPs work! You'll also have to free this memory since it's being allocated on the heap.

In the function greyscale:

float average;
....
average = 1.0 * (image[i][j].rgbtBlue + image[i][j].rgbtGreen + image[i][j].rgbtRed) / 3;

A lot of C programmers declare all of their variables at the beginning of functions. C89 required that local variables be declared at the beginning of the block they live in, and some people took that to the extreme and declared stuff as far up as they could go.

In 2020, you don't have to do this. You haven't followed C89 in a lot of other places anyway (compile with -std=c89 to see the 60 warnings and 3 errors). So you might as well do:

float average = ....

This way you avoid uninitialized variables, and you will never have to scroll up/down/up/down... while reading your C code.

As a nit, I think instead of 1.0 * (...) / 3 you could just as easily have written (...) / 3.0. Or better yet, just do integer division. You're about to round anyway. Does it matter if you are 0.33/255 off?

Either way, if you're going to round average, you might as well make it an int and then call round once instead of calling round(average) three times.

In the function reflect:

How about making a function void swap(BYTE* lhs, BYTE* rhs, int size);? Then you can write

for (int i ....) {
 for (int j .....) {
 swap(image[i][j], image[i][width - j - 1], sizeof(RGBTRIPLE));
 }
}

instead of 9 bulky lines of code.

In the function blur:

float averageGreen;

Same comment as before about initializing at the top of functions.

RGBTRIPLE image2[height][width];

Same comment about stack vs. heap. This will fail for large images. Also, how about a better name like "blurredImage" or something to that effect. Most of your names are good, but it's a bad sign when you have to use a counter to distinguish your variables in human readable code.

for (int r = i - 1; r <= i + 1 && r < height; r++) 
{
 if (r < 0)
 {
 continue;
 }

This breaks my brain. Cognitive overload. No wonder you wrote "it took me way too long to make all the for loops work."

it looks like you did get it right (as far as I can tell...), so good job. Even though it's not as elegant as it could be, it's good you stuck with it.
As you write more code, you'll get used to writing loops/boolean logic, and this will get easier. That said there are some guidelines to make this easier.

I think my brain blew while I read this because you wrote && r < height in the for loop's condition and then put if (r < 0) in an if statement. You didn't separate concerns well enough.

Your first concern is to find i and j.

Your second concern is to find how far from i and j you need to stray.

And your final concern is to avoid going out of bounds.

How does this look?

for (int i = 0; i < height; ++i) {
 for (int j = 0; j < width; ++j) {
 for (int i_offset = -1; i_offset <= -1; ++i_offset) {
 for (int j_offset = -1; j_offset <= -1; ++j_offset) {
 int row = i + i_offset;
 int col = j + j_offset;
 if (0 <= row && row < height && ...) {
 ...
 }
 }
 }
 }
}

I think this is relatively easy to read.

Some would argue this is less efficient since you'll run through all the col values when row is out of bounds. Don't listen to these arguments though -- the compiler can hoist these checks for you (at least Clang 10.0.0 with -O does this). As a rule, write the clear thing and only optimize manually if you have to.

I could probably rewrite the function using the 0 padding as well, like for the edge one but this seems cleaner.

I agree that in this case it's cleaner not to use 0 padding. How could you figure out count if you use 0 padding?

for (int i = 0; i < height; i++)
{
 for (int j = 0; j < width; j++)
 {
 image[i][j] = image2[i][j];
 }
}

You basically want to do image = image2. This can be done more elegantly.

First idea (very common idiom): use memcpy. It will be faster and it's easier to read.

Second idea (maybe controversial):

swap(image, image2); // swap the pointers
free(image2); // free the original image

Faster and simpler! This only works if you know you can call free on the memory passed into the function -- that's a strong assumption! In this case you can (assuming you fix your declaration of image2 to use calloc or similar), but you should at the very least add a comment to the function definition saying you plan to free the argument.

In the function edges:

int GxBlue;

Same comment.

//initialise the Gx and Gy kernels

Good to see Gx and Gy written out so clearly! An uglier implementation would have -1 * blah + 1 * bleh + .... Good that you didn't do that.

RGBTRIPLE image2[height + 2][width + 2];

Same comment.

//if on edges add 0

If you use calloc, then you can assume the memory you receive is zeroed out.

//else add original array values
if (i > 0 && i < height + 1 && j > 0 && j < width + 1)

How about just else instead of this comment/a written out negation of the condition? This isn't just a style tip -- you don't want to make it hard for the compiler to prove it only needs to compute the condition once.

if (sqrt(GxBlue * GxBlue + GyBlue * GyBlue) > 255)
{
 image3[i - 1][j - 1].rgbtBlue = 255;
}
else
{
 image3[i - 1][j - 1].rgbtBlue = round(sqrt(GxBlue * GxBlue + GyBlue * GyBlue));
}

How about

image3[i - 1][j - 1].rgbtBlue = edgeHelper(GxBlue);

Where edgeHelper is a function that looks like:

BYTE edgeHelper(float Gx, float Gy) {
 float magnitude = round(euclideanDistance(Gx, Gy));
 return magnitude > 255 ? 255 : magnitude; 
}

Note that I've rounded before doing the comparison rather than after. I don't think it matters much, but it's a difference all the same. You'll have to figure out euclideanDistance but I think you'll have no issue with it!

image[i][j].rgbtBlue = image3[i][j].rgbtBlue;
image[i][j].rgbtGreen = image3[i][j].rgbtGreen;
image[i][j].rgbtRed = image3[i][j].rgbtRed;

Earlier in your program you wrote image[i][j] = image2[i][j] .. that was better than this! but the same comments apply regardless. Since you [height + 2][width + 2] and relied on having 0s in memory rather than doing bound checks, you can no longer do that swap/free hack.

Architectural tip:

How about adding (and using) these:

// could store height/width in the image
struct Image { ... };
Image* createImage(int height, int width);
void destroyImage(Image*);
void swapImage(Image*, Image*);
// this could return 0 for out of bounds
RGBTRIPLE getPixel(Image*, int i, int j);
setPixel(Image*, int i, int j, RGBTRIPLE);
// this would help you get rid of a lot of the for loops!
forEachImageIndex(Image*, void(*callback)(Image*, int j, int j));

Properly using these would probably mean modifying filter.c, but maybe it's worth thinking about.

Is there any way of writing the edge function code cleaner and more efficient for a beginer

I think I already answered about cleanliness, but I'll answer here about efficiency. Your code is pretty efficient. One of the beautiful things about C is that it encourages writing simple code, and the compiler can do a really good job optimizing simple code for you. I played around with different compiler flags, and Clang can optimize/vectorize this pretty well.

For the edge function I admit the padding with 0 ... I found on stackoverflow, ... I just used the idea. Is it a bad practice or something that is usually done?

There are a few concerns here:

Can you legally copy stuff from SO? I'll link you to an answer that can explain better than I can: https://meta.stackoverflow.com/questions/339152/plagiarism-and-using-copying-code-from-stack-overflow-and-submitting-it-in-an-as.
Is it a good idea to copy ideas from SO? In my opinion, it's OK as long as you understand what you're copying. You should copy a link to what you're copying (except for the most generic ideas).
Is it a good idea to copy code from SO? Copying code is so dangerous. I would avoid it if you can.
"Is copying code from SO common" It is so common that people have made memes about it. But that doesn't make it a good idea...

TL;DR: I think what you did is OK, but you should cite SO.com somewhere.

General tips:

Learn more of stdlib.h like memcpy and when to use malloc/free.
Don't be afraid to make helper functions to avoid repetitive code.
Use a more modern C standard/more modern C style guide.
Test your code with lots of files. Seek out big ones/small ones/weird ones etc. You could have caught that segfault.
Keep practicing!

Thank you so much for your answer and the time you invested to answer. to be honest I started learning Java and then I heard of CS50 and wanted to get a hang of C to understand memory and low level a bit better, and imo C is way harder than Java as you need to do everything yourself, but I understood a bit better how things work under the bonnet. I will indeed refactor the code for this one using your tips and see how I get along. Thank you again.

Stack Exchange Network

CS50 pset4 filter - Image Filters in C

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

CS50 pset4 filter - Image Filters in C

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions