# CS50 pset4 filter - Image Filters in C

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:

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.
*
* 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;
}


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];

for (int i = 0 ; i < height + 2; i ++)
{
for (int j = 0; j < width + 2; j++)
{
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;
}
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:

picture after filter:

So I have 2 questions:

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

2. 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 :)

• Thanks for letting me know. I've updated it and hope it's better this time! Apr 24, 2020 at 15:35

What a cool topic!

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."

1. 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.

2. 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));
}


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:

// 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:

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. May 3, 2020 at 12:08
    #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):

• 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?. Sep 25, 2021 at 12:15