I spent several day trying to figure out what's wrong with this function. It actually compile fine but it blurs the image in the wrong way. I believe maybe my numbers or formulas are wrong but after checking it again and again for several hours I cannot figure out what s wrong.
The exercise wants you to blur an image using a 3*3 blur box so adding up the RGB values of the blur box and dividing them by the valid blur box pixels (the ones in side the image boundaries).Then assigning the 'blurred' values to the image.
In my blurred image the blur does not allow to distinguish any figure in the picture. I checked other blur function answers and tried to make and re-make the code from scratch with different variations but nothing seems to work. The code below it seems to me it s the closer to the right solution. Any help would be very much appreciate. Sorry for any mistake in my english
// Blur image
void blur(int height, int width, RGBTRIPLE image[height][width])
{ // create image copy for holding modifies RGB values
RGBTRIPLE copy[height][width];
for(int i = 0; i < height; i )
{
for (int j = 0; j < width; j )
{
copy[i][j] = image[i][j];
}
}
// setting variable for holding sum of blur box RGB values
float blurred = 0.0;
float blurgreen = 0.0;
float blurblue = 0.0;
// setting counter for valid blur box pixels
float validpixels = 0.0;
// looping through image rows
for( int i = 0; i < height; i )
{ // looping through image columns
for(int j = 0; j < width; j )
{ // looping through blur box rows
for(int r = -1; r < 2; r )
{ //looping through blur box columns
for(int c = -1; c < 2; c )
{ // counting only valid blur-box pixels(inside image boundaries)
if(i c >= 0 && j r >= 0 && i c < height && j r < width)
{ // storing total of blur box RGB values
blurred = copy[i c][j r].rgbtRed;
blurgreen = copy[i c][j r].rgbtGreen;
blurblue = copy[i c][j r].rgbtBlue;
// counting valid pixels total
validpixels ;
}
}
}
// calculate RGB blurred values and assigning them to image copy
copy[i][j].rgbtRed = round(blurred / validpixels);
copy[i][j].rgbtGreen = round(blurgreen / validpixels);
copy[i][j].rgbtBlue = round(blurblue / validpixels);
}
}
//giving blur values back to image
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;
}
CodePudding user response：
You have 2 main issues:
- You must re-initialize your
blur*variables and also the counter for each pixel. Otherwise you keep summing up more and more pixels. That will basically create an average of the whole image when you get to the end. - You are storing blurred values in the same array where you read the values to blur. That spills the whole purpose of making a copy at all.
Not an issue but could be improved:
- Do not use
floatin your programs. Unless you have some very strict limitations in memory or computing power, you should always usedoublefor floating point numbers. - Counters are best defined as integers, not floating point numbers.
- Pointed out by Chux: As your destination is an integer value, you could replace
roundbylround. Or use integer arithmetics all the way as shown in Chux's answer.
The fixes are marked in the code using //####
// Blur image
void blur(int height, int width, RGBTRIPLE image[height][width])
{ // create image copy for holding modifies RGB values
RGBTRIPLE copy[height][width];
for(int i = 0; i < height; i )
{
for (int j = 0; j < width; j )
{
copy[i][j] = image[i][j];
}
}
// looping through image rows
for( int i = 0; i < height; i )
{ // looping through image columns
for(int j = 0; j < width; j )
{ // looping through blur box rows
// setting variable for holding sum of blur box RGB values
//#### Initialize your blurring variables for each pixel you are visiting.
//#### Always use double instead of float except you have specific requirements.
double blurred = 0.0;
double blurgreen = 0.0;
double blurblue = 0.0;
// setting counter for valid blur box pixels
//#### Also re-initialize counter for each pixel
//#### Counters are normally integers, no floating point numbers
size_t validpixels = 0;
for(int r = -1; r < 2; r )
{ //looping through blur box columns
for(int c = -1; c < 2; c )
{ // counting only valid blur-box pixels(inside image boundaries)
if(i c >= 0 && j r >= 0 && i c < height && j r < width)
{ // storing total of blur box RGB values
blurred = copy[i c][j r].rgbtRed;
blurgreen = copy[i c][j r].rgbtGreen;
blurblue = copy[i c][j r].rgbtBlue;
// counting valid pixels total
validpixels ;
}
}
}
// calculate RGB blurred values and assigning them to image copy
//#### No! Don't store in the copy again. This will disturb
//#### calculation of the other pixels.
image[i][j].rgbtRed = lround(blurred / validpixels);
image[i][j].rgbtGreen = lround(blurgreen / validpixels);
image[i][j].rgbtBlue = lround(blurblue / validpixels);
}
}
//giving blur values back to image
//#### No! Copying again is not required. Results were stored there already
return;
}
CodePudding user response：
Any help would be very much appreciate.
Not a fix to a problem, but a simplification.
//for(int i = 0; i < height; i ) {
// for (int j = 0; j < width; j ) {
// copy[i][j] = image[i][j];
// }
//}
memcpy(copy, image, sizeof copy);
Likewise with final copy back to image. Yet since copy is not needed, just drop this step.
// memcpy(image, copy, sizeof copy);
Do not use sizeof image as that is not the size of the image.
Rather than float math, stay with integer math.
// float blurred = 0.0;
unsigned blurred = 0;
// float validpixels = 0.0;
unsigned validpixels = 0;
...
// Leave as is.
blurred = copy[i c][j r].rgbtRed;
...
// copy[i][j].rgbtRed = round(blurred / validpixels);
// Put result in `image[]`
image[i][j].rgbtRed = (blurred validpixels/2) / validpixels;
Rather than test r in the deepest loop:
{ //looping through blur box columns
for(int c = -1; c < 2; c )
{ // counting only valid blur-box pixels(inside image boundaries)
if(i c >= 0 && j r >= 0 && i c < height && j r < width)
{ // storing total of blur box RGB values
blurred = copy[i c][j r].rgbtRed;
blurgreen = copy[i c][j r].rgbtGreen;
blurblue = copy[i c][j r].rgbtBlue;
// counting valid pixels total
validpixels ;
}
}
}
Move the test and reset the validpixels
unsigned blurred = 0;
unsigned blurgreen = 0;
unsigned blurblue = 0;
unsigned validpixels = 0;
for(int r = -1; r < 2; r ) {
// Add
if(!(j r >= 0 && j r < width)) continue;
for(int c = -1; c < 2; c ) {
// if(i c >= 0 && j r >= 0 && i c < height && j r < width)
if(i c >= 0 && i c < height) {
blurred = copy[i c][j r].rgbtRed;
blurgreen = copy[i c][j r].rgbtGreen;
blurblue = copy[i c][j r].rgbtBlue;
validpixels ;
}
}
}
Advanced
Division can be expensive in time. As the vast majority of divisions is by 9, consider:
image[i][j].rgbtRed = validpixels == 9 ?
(blurred 9/2) / 9 :
(blurred validpixels/2) / validpixels;
With optimizations turned on, the compiler is certain to create faster code with a fixed divisor of 9, even with the validpixels == 9 overheard.