CS50 Pset 4 edges: Image is static

Question

#include "helpers.h"
#include <math.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

int sq(int x)
{
    return x*x;
}

void edges(int height, int width, RGBTRIPLE image[height][width])
{
    int gx_square[] = {-1,0,1,-2,0,2,-1,0,1};
    int gy_square[] = {-1,-2,-1,0,0,0,1,2,1};
    int gx_red[height][width], gx_blue[height][width], gx_green[height][width];
    int gy_red[height][width], gy_blue[height][width], gy_green[height][width];
    int square = -1;

    for (int i = 0; i < height; i++)
    {
        // Width
        for (int j = 0; j < width; j++)
        {
            gx_red[i][j] = 0;
            gx_blue[i][j] = 0;
            gx_green[i][j] = 0;
            gy_red[i][j] = 0;
            gy_blue[i][j] = 0;
            gy_green[i][j] = 0;
        }
    }
    // Height
    for (int i = 0; i < height; i++)
    {
        // Width
        for (int j = 0; j < width; j++)
        {
            //Fill gx and gy arrays
            for (int column = i - 1; column < i + 2; column++)
            {
                for (int row = j - 1; row < j + 2; row++)
                {
                    //If pixel is outside image
                    square++;
                    if (column < 0 || column >= height || row < 0 || row >= width)
                    {
                            continue;
                    }
                    // Update array
                    gx_red[i][j] += image[column][row].rgbtRed * gx_square[square];
                    gx_blue[i][j] += image[column][row].rgbtBlue * gx_square[square];
                    gx_green[i][j] += image[column][row].rgbtGreen * gx_square[square];
                    gy_red[i][j] += image[column][row].rgbtRed * gy_square[square];
                    gy_blue[i][j] += image[column][row].rgbtBlue * gy_square[square];
                    gy_green[i][j] += image[column][row].rgbtGreen * gy_square[square];
                }
            }
            square = 0;
        }
    }

    // Height
    for (int i = 0; i < height; i++)
    {
        //Width
        for (int j = 0; j < width; j++)
        {
            // Return color
            image[i][j].rgbtRed = sqrt(sq(gx_red[i][j]) + sq(gy_red[i][j]));
            if (image[i][j].rgbtRed > 255)
            {
                image[i][j].rgbtRed = 255;
            }
            image[i][j].rgbtBlue = sqrt(sq(gx_blue[i][j]) + sq(gy_blue[i][j]));
            if (image[i][j].rgbtBlue > 255)
            {
                image[i][j].rgbtBlue = 255;
            }
            image[i][j].rgbtGreen = sqrt(sq(gx_green[i][j]) + sq(gy_green[i][j]));
            if (image[i][j].rgbtGreen > 255)
            {
                image[i][j].rgbtGreen= 255;
            }
        }
    }
    return;
}

    //./filter -e images/stadium.bmp OUTFILE.bmp
    // debug50 filter -e images/stadium.bmp OUTFILE.bmp

My task in edges is to outline any object in an image like this:

You do this by taking all the pixels color around an image, and running 2 algorithms through it called gx and gy. The algorithms work by mulitplying all the pixels around a pixel by a couple of numbers. Here's a visual representation of the multiplication order. (The middle pixel is the current pixel)

So for gx, the first pixel gets multiplied by -1, the next by 0, then 1, and so on. And then we keep the sum. But it's not the pixel that's getting multipled, but the color value. Lets use Red as an example. Lets say all the pixels around a pixel (including the pixel itself) has 255 red.

Since there's little change between the pixels, everything cancels out, and the sum of all the numbers is 0.

If there's a big change, we're left with a big number. (I'll get to what we do with this number soon) And we do this with green and blue. The purpose of gx and gy is to detect changes on the x coordinates and on the y coordinates. You also might be thinking, what if the program looked at pixels not inside the image. If that is the case, we just treat them as 0. (Which is the same thing as ignoring them)

Finally, we take the square root of (gx.red squared + gy.red squared) to get our new red value for that pixel. (You still do this with green and blue.)

I posted this question because I'm not really sure what's wrong with my code. I have gx and gy arrays that store all the color values, as well as arrays for the actual multiplication algorithm. If the pixel is at the border, it skips it (which is the same as 0). And then I make all the pixels equal to the square root of the square of the gx color + the square of the gy color. This is what the image I use normally looks like:

And this is what it looks like with my code.

I wonder why this is.

Okay with your suggestions, it looks like this:

And this is the current code:

void edges(int height, int width, RGBTRIPLE image[height][width])
{
    int gx_square[] = {-1,0,1,-2,0,2,-1,0,1};
    int gy_square[] = {-1,-2,-1,0,0,0,1,2,1};
    int gx_red[height][width], gx_blue[height][width], gx_green[height][width];
    int gy_red[height][width], gy_blue[height][width], gy_green[height][width];


    memset( gx_red, 0, sizeof gx_red ); // use library functions when possible.
    memset( gx_blue, 0, sizeof gx_blue );
    memset( gx_green, 0, sizeof gx_green );
    memset( gy_red, 0, sizeof gy_red );
    memset( gy_blue, 0, sizeof gy_blue );
    memset( gy_green, 0, sizeof gy_green );

    // Height
        for (size_t row = 0; row < height; row++)
        {
            // Width
            for (size_t col = 0; col < width; col++)
            {
                size_t square = 0;
                //Fill gx and gy arrays
                for (int r = row - 1; r < row + 2; r++)
                {
                    for (int c = col - 1; c < col + 2; c++, square++)
                    {
                        //If pixel is outside image
                        if ( ( 0 <= r && r < width) && ( 0 <= c && c < height ) )
                        {
                            // Update array
                            gx_red[row][col] += image[r][c].rgbtRed * gx_square[square];
                            gx_blue[row][col] += image[r][c].rgbtBlue * gx_square[square];
                            gx_green[row][col] += image[r][c].rgbtGreen * gx_square[square];
                            gy_red[row][col] += image[r][c].rgbtRed * gy_square[square];
                            gy_blue[row][col] += image[r][c].rgbtBlue * gy_square[square];
                            gy_green[row][col] += image[r][c].rgbtGreen * gy_square[square];
                        }
                    }
                }
            }
        }

        //Width
    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            int clr;

            clr = sqrt(sq(gx_red[i][j]) + sq(gy_red[i][j]));
            image[i][j].rgbtRed = ( clr <= 255 ) ? clr : 255;

            clr = sqrt(sq(gx_blue[i][j]) + sq(gy_blue[i][j]));
            image[i][j].rgbtBlue = ( clr <= 255 ) ? clr : 255;

            clr = sqrt(sq(gx_green[i][j]) + sq(gy_green[i][j]));
            image[i][j].rgbtGreen = ( clr <= 255 ) ? clr : 255;
        }
    }
    return;
}

    //./filter -e images/stadium.bmp OUTFILE.bmp
    // debug50 filter -e images/stadium.bmp OUTFILE.bmp

Answer 1

The index variable square is initialised to -1 when it is declared. This is a red flag. Starting an index with a negative value suggests something bad is happening. Properly declared, any index should be of type size_t, an unsigned datatype.

The bug is compounded by the scope of that variable. Its scope should be restricted to the only the code where it is used.

Here's an untested revision:

void edges(int height, int width, RGBTRIPLE image[height][width])
{
    int gx_square[] = {-1,0,1,-2,0,2,-1,0,1};
    int gy_square[] = {-1,-2,-1,0,0,0,1,2,1};
    int gx_red[height][width], gx_blue[height][width], gx_green[height][width];
    int gy_red[height][width], gy_blue[height][width], gy_green[height][width];


    memset( gx_red, 0, sizeof gx_red ); // use library functions when possible.
    memset( gx_blue, 0, sizeof gx_blue );
    memset( gx_green, 0, sizeof gx_green );
    memset( gy_red, 0, sizeof gy_red );
    memset( gy_blue, 0, sizeof gy_blue );
    memset( gy_green, 0, sizeof gy_green );

    // Height
    for (size_t row = 0; row < height; row++)
    {
        // Width
        for (size_t col = 0; col < width; col++)
        {
            size_t square = 0; // local to this single pixel

            // DO NOT switch from row/col to col/row... Just confusing!!!

            //Fill gx and gy arrays
            for (int r = row - 1; r <= row + 1; r++)
            {
                // square increments AFTER each iteration.
                // No mysterious -1 values...
                for (int c = col - 1; c <= col + 1; c++, square++)
                {
                    // avoid "continue;" whenever possible.
                    // use "positive" logic instead.
                    if ( ( 0 <= r && r < height) && ( 0 <= c && c < width ) )
                    {
                        // Update array
                        gx_red[row][col] += image[r][c].rgbtRed * gx_square[square];
                        gx_blue[row][col] += image[r][c].rgbtBlue * gx_square[square];
                        gx_green[row][col] += image[r][c].rgbtGreen * gx_square[square];
                        gy_red[row][col] += image[r][c].rgbtRed * gy_square[square];
                        gy_blue[row][col] += image[r][c].rgbtBlue * gy_square[square];
                        gy_green[row][col] += image[r][c].rgbtGreen * gy_square[square];
                    }
                }
            }
        }
    }
    // unaltered function code continues below.

I'm not going to go down the rabbit hole, but it's also possible that your version, switching from row/col to col/row, might be using the wrong multipliers from the Sobel array. Maybe you will check that...

---

EDIT (with credit to @n.m. could be an ai)

It's likely that the RGB triplet is made up of single byte values (0-255). Assigning the result of sqrt() without checking may lead to corrupting those values trying to fit into a single byte.

The following corrects this (again, untested.)

    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            int clr;

            clr = sqrt(sq(gx_red[i][j]) + sq(gy_red[i][j]));
            image[i][j].rgbtRed = ( clr <= 255 ) ? clr : 255;

            clr = sqrt(sq(gx_blue[i][j]) + sq(gy_blue[i][j]));
            image[i][j].rgbtBlue = ( clr <= 255 ) ? clr : 255;

            clr = sqrt(sq(gx_green[i][j]) + sq(gy_green[i][j]));
            image[i][j].rgbtGreen = ( clr <= 255 ) ? clr : 255;
        }
    }