Search code examples
ccomputer-sciencecs50

CS50 Pset 4 - Filters - Helpers.c - Blur


The below is my code. There are no apparent bugs in my code when I compile it. However, I fail all of the Check50 tests. What is flawed in the reasoning of my algorithm?

Basically what my code aims to do is copy all the pixels into another struct called 'old' which will maintain the content of the original image.

Next, I create an array of temporary pixel structs which I call 'temp'. The array consists of 9 elements, for itself and each of the surrounding pixels. The following 4 'if statements' test whether or not the pixels are border pixels. If they are, the red color of the relevant elements in the 'temp' array are assigned a negative value of -1. This is essentially to discard them later on in the algorithm.

Next I declare three integer variables keeping track of the total red, green and blue values over the surrounding pixels. I then check for each surrounding pixel if the red value is a negative number, if so I discarded it. If it is indeed a positive number, I add to the red, blue and green total count. I simultaneously keep track of a count of total surrounding pixels so that I can in the end divide the total values of red, green and blue by this count. The above is repeated for each pixel. In theory this should yield the blurred image but it clearly doesn't. What is wrong in my reasoning or my code?

  void blur(int height, int width, RGBTRIPLE image[height][width])
 {
 RGBTRIPLE old[height][width];
 RGBTRIPLE temp[9];

for (int i = 0; i < height; i++)
{
    for (int j = 0; j < width; j++)
    {
        old[i][j] = image[i][j];
    }
}
for (int i = 0; i < height; i++)
{
    for (int j = 0; j < width; j++)
    {
        temp[0] = old[i-1][j-1];
        temp[1] = old[i][j-1];
        temp[2] = old[i+1][j-1];
        temp[3] = old[i-1][j];
        temp [4] = old[i][j];
        temp [5] = old[i+1][j];
        temp [6] = old[i-1][j+1];
        temp [7] = old[i][j+1];
        temp [8] = old[i+1][j+1];

        if (i == 0)
        {
            temp[0].rgbtRed = -1;
            temp[3].rgbtRed = -1;
            temp[6].rgbtRed = -1;
        }
        else if (i == height-1)
        {
            temp[2].rgbtRed = -1;
            temp[5].rgbtRed = -1;
            temp[8].rgbtRed = -1;
        }
        if (j == 0)
        {
            temp[0].rgbtRed = -1;
            temp[1].rgbtRed = -1;
            temp[2].rgbtRed = -1;
        }
        else if (j == width-1)
        {
            temp[6].rgbtRed = -1;
            temp[7].rgbtRed = -1;
            temp[8].rgbtRed = -1;
        }

        int Red = 0;
        int Green = 0;
        int Blue = 0;

        int c = 0;

        for (int m = 0; m <= 8; m++)
        {
            if (temp[m].rgbtRed >= 0)
            {
                c++;
                Red = Red + temp[m].rgbtRed;
                Green = Green + temp[m].rgbtGreen;
                Blue = Blue + temp[m].rgbtBlue;
            }
        }
        image[i][j].rgbtRed = round(Red/c);
        image[i][j].rgbtGreen = round(Green/c);
        image[i][j].rgbtBlue = round(Blue/c);
    }
}

}

[Edit] Main Code:

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

  #include "helpers.h"

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

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

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

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

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

    // Sepia
    case 's':
        sepia(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;
}

[RGB TRIPLE DEF]

 typedef struct
 {
BYTE  rgbtBlue;
BYTE  rgbtGreen;
BYTE  rgbtRed;
  } __attribute__((__packed__))
  RGBTRIPLE;

Solution

  • Code is accessing memory out of bounds with the very first iteraiton of temp[0] = old[i-1][j-1];


    Code needs to insure access on the edges and corners stay within the array.

    I am unclear how OP want to handle those cases.

    For an example: the below does an averaging on the neighboring pixels, be it 8, 5 or 3 neighbors. Not highly efficient, just illustrative.

    void blur(int height, int width, RGBTRIPLE image[height][width]) {
      RGBTRIPLE old[height][width];
      memcpy(old, image, sizeof image[height][width] * height * width);
    
      for (int i = 0; i < height; i++) {
        for (int j = 0; j < width; j++) {
          int Red = 0;
          int Green = 0;
          int Blue = 0;
          int count = 0;
          for (int di = -1; di <= 1; di++) {
            int pi = i + di;
            if (pi >= 0 && pi < height) {
              for (int dj = -1; dj <= 1; dj++) {
                int pj = j + dj;
                if (pj >= 0 && pj < width) {
                  Red += old[pj][pj].rgbtRed;
                  Green += old[pj][pj].rgbtGreen;
                  Blue += old[pj][pj].rgbtBlue;
                  count++;
                }
              }
            }
          }
          // I am suspicious of using FP code here.
          // I'd use image[i][j].rgbtRed = (Red + count/2) / count;
          image[i][j].rgbtRed = round(Red / count);
          image[i][j].rgbtGreen = round(Green / count);
          image[i][j].rgbtBlue = round(Blue / count);
        }
      }
    }
    

    For me I would have made old 1 bigger on all 4 edges and copied the original edges to the new edges and corners to the new corners. Then easy to average the original image with 8 neighbors per pixel.