I am working on the Filter problem set for CS50 and run into a problem related to check50 (which is the program that checks my solution for correctness).
"Blur" function calculates the averange values incorrectly.
Here are the check50 errors:
blur correctly filters middle pixel
expected "127 140 149\n", not "120 140 150\n":
testing with sample 3x3 image
first row: (10, 20, 30), (40, 50, 60), (70, 80, 90)
second row: (110, 130, 140), (120, 140, 150), (130, 150, 160)
third row: (200, 210, 220), (220, 230, 240), (240, 250, 255)
running ./testing 3 0...
checking for output "127 140 149\n"...
blur correctly filters pixel on edge
expected "127 140 149\n", not "120 140 150\n":
testing with sample 3x3 image
first row: (10, 20, 30), (40, 50, 60), (70, 80, 90)
second row: (110, 130, 140), (120, 140, 150), (130, 150, 160)
third row: (200, 210, 220), (220, 230, 240), (240, 250, 255)
running ./testing 3 0...
checking for output "127 140 149\n"...
blur correctly filters 3x3 image
expected "70 85 95\n80 9...", not "70 85 95\n76 9...":
testing with sample 3x3 image
first row: (10, 20, 30), (40, 50, 60), (70, 80, 90)
second row: (110, 130, 140), (120, 140, 150), (130, 150, 160)
third row: (200, 210, 220), (220, 230, 240), (240, 250, 255)
running ./testing 3 3...
checking for output "70 85 95\n80 95 105\n90 105 115\n117 130 140\n127 140 149\n137 150 159\n163 178 188\n170 185 194\n178 193 201\n"...
blur correctly filters 4x4 image
expected "70 85 95\n80 9...", not "70 85 95\n76 9...":
testing with sample 4x4 image
first row: (10, 20, 30), (40, 50, 60), (70, 80, 90), (100, 110, 120)
second row: (110, 130, 140), (120, 140, 150), (130, 150, 160), (140, 160, 170)
third row: (195, 204, 213), (205, 214, 223), (225, 234, 243), (245, 254, 253)
fourth row: (50, 28, 90), (0, 0, 0), (255, 255, 255), (85, 85, 85)
running ./testing 3 4...
checking for output "70 85 95\n80 95 105\n100 115 125\n110 125 135\n113 126 136\n123 136 145\n142 155 163\n152 165 173\n113 119 136\n143 151 164\n156 166 171\n180 190 194\n113 112 132\n155 156 171\n169 174 177\n203 207 209\n"...
And this is the code i wrote:
void blur(int height, int width, RGBTRIPLE image[height][width])
{
RGBTRIPLE copy[height][width];
float sumRed = 0;
float sumGreen = 0;
float sumBlue = 0;
float counter = 0;
for (int i = 0; i < height; i++)
{
for (int j = 0; j < width; j++)
{
copy[i][j] = image[i][j];
for (int k = -1; k < 2; k++)
{
for (int l = -1; l < 2; l++)
{
if (i + k < 0 || i + k > height - 1)
{
continue;
}
else if (j + l < 0 || j + l > width - 1)
{
continue;
}
sumRed += image[i + k][j + l].rgbtRed;
sumGreen += image[i + k][j + l].rgbtGreen;
sumBlue += image[i + k][j + l].rgbtBlue;
counter++;
}
}
copy[i][j].rgbtRed = round(sumRed / counter);
copy[i][j].rgbtGreen = round(sumGreen / counter);
copy[i][j].rgbtBlue = round(sumBlue / counter);
}
}
for (int i = 0; i < height; i++)
{
for (int j = 0; j < width; j++)
{
image[i][j].rgbtRed = copy[i][j].rgbtRed;
image[i][j].rgbtGreen = copy[i][j].rgbtGreen;
image[i][j].rgbtBlue = copy[i][j].rgbtBlue;
}
return;
}
}
I already tried fixing it with cs50.ai, but it tells me that I only calculate Red values wrong, while it is not entirly true according to check50 errors. I am not sure if my calculations of RGB values are correct when I am saving them to a copy[][] and if it is wrong - I don't know how to proceed.
Thank you.
As @WeatherVane observed in comments, you need to reset the sums and counter for each pixel, but your original code fails to do this.
Additionally, your return statement is misplaced. It is inside the second i loop, with the effect that only the first row of the blurred image is copied to the original image before the function returns.
In addition to the secondary issues that WeatherVane also called out,
it is unconventional to use the autoincrement and autodecrement operators with floating-point operands (though allowed).
your use of round() suggests that the elements of an RGBTRIPLE are integers, in which case it would be better as a matter of style to avoid floating-point math except at the division.
When you do use floating point, use double unless you have a compelling reason to do otherwise. This is the most natural C floating point type.
It is somewhat dangerous to declare local variable-length arrays. For one thing, VLAs are an optional feature in C11 and C17, and partially so in C23 too. For another thing, it is really easy to overflow the stack that way, without that necessarily showing up in testing. The assignment seems to assume a C implementation with VLA support, and the tester might happen not to exercise a case that overflows the stack, but in real-world code you would be better off dynamically allocating the temporary.
instead of a pixel-by-pixel copy loop, it would be better to memcpy() the whole blurred result back to the original image all at once. That's shorter and clearer, as evidenced in part by the fact that it would not have afforded the possibility of misplacing the return as you did.
Here's a version of your code incorporating all of those recommendations except WeatherVane's variable renaming (which is nonetheless a good suggestion):
void blur(int height, int width, RGBTRIPLE image[height][width]) {
size_t image_size = (size_t) height * width * sizeof(image[0][0]);
RGBTRIPLE (*copy)[width] = malloc(image_size);
// ... check for and handle allocation failure ...
for (int i = 0; i < height; i++) {
for (int j = 0; j < width; j++) {
int sumRed = 0;
int sumGreen = 0;
int sumBlue = 0;
int counter = 0;
for (int k = -1; k < 2; k++) {
for (int l = -1; l < 2; l++) {
if (i + k < 0 || i + k >= height) {
continue;
} else if (j + l < 0 || j + l >= width) {
continue;
}
sumRed += image[i + k][j + l].rgbtRed;
sumGreen += image[i + k][j + l].rgbtGreen;
sumBlue += image[i + k][j + l].rgbtBlue;
counter++;
}
}
copy[i][j].rgbtRed = round(sumRed / (double) counter);
copy[i][j].rgbtGreen = round(sumGreen / (double) counter);
copy[i][j].rgbtBlue = round(sumBlue / (double) counter);
}
}
memcpy(image, copy, image_size);
free(copy);
return;
}