PSET4: Filter(less)

I am attempting the Filter problem of Pset4(less comfortable). I wrote this code below to reflect the image. But it doesn't work at all. It compiles alright however.

// Reflect image horizontally
void reflect(int height, int width, RGBTRIPLE image[height][width])
{
    for (int i = 0; i < height; i++)  //for rows
    {
        int start = 0;
        for (int end = width; end > 0; end--)
        {
            BYTE tempR, tempB, tempG;
            if(start < end)
            {
                tempR = image[i][start].rgbtRed;
                tempB = image[i][start].rgbtBlue;
                tempG = image[i][start].rgbtGreen;
                image[i][start].rgbtRed = image[i][end].rgbtRed;
                image[i][start].rgbtBlue = image[i][end].rgbtBlue;
                image[i][start].rgbtGreen = image[i][end].rgbtGreen;
                image[i][end].rgbtRed = tempR;
                image[i][end].rgbtBlue = tempB;
                image[i][end].rgbtGreen = tempG;
                start += 1;
            }
        }
    }
    return;
}

Before I saw the comment, this looked like a "reverse the order of all pixels on a [raster] line", based on the code.

A few issues ...

end should start at width - 1 [and not width]. It's starting at one past the last pixel in the row (i.e. the first pixel of the next row). For the last row, this is UB [undefined behavior].

So, the effect was a shift by one pixel and I bet the first and last pixels of the row looked "interesting" ...

The inner loop condition should be start < end instead of end > 0 and we should eliminate the if (start < end) altogether.

In addition to correctness, this will eliminate the last 50% of the inner loops. Those were not transferring data [because the if would become false and remain so for the rest of the row]. So, the loop was doing twice as many loop iterations (i.e.) only the first iterations did useful work.

In other words, the inner for loop was doing width iterations instead of width / 2 iterations.

Here's the cleaned up and corrected code:

// Reflect image horizontally
void
reflect(int height, int width, RGBTRIPLE image[height][width])
{

    // for rows
    for (int i = 0; i < height; i++) {
        int start = 0;

        for (int end = width - 1;  start < end;  --end, ++start) {
            BYTE tempR, tempB, tempG;

            tempR = image[i][start].rgbtRed;
            tempB = image[i][start].rgbtBlue;
            tempG = image[i][start].rgbtGreen;

            image[i][start].rgbtRed = image[i][end].rgbtRed;
            image[i][start].rgbtBlue = image[i][end].rgbtBlue;
            image[i][start].rgbtGreen = image[i][end].rgbtGreen;

            image[i][end].rgbtRed = tempR;
            image[i][end].rgbtBlue = tempB;
            image[i][end].rgbtGreen = tempG;
        }
    }
}

Although, the optimizer may generate efficient code from the above, here's a version where start and end are pointers. This makes it easier for the compiler to see the intent, and is simpler and easier to read:

// Reflect image horizontally
void
reflect(int height, int width, RGBTRIPLE image[height][width])
{

    // for rows
    for (int i = 0; i < height; i++) {
        RGBTRIPLE *start = &image[i][0];
        RGBTRIPLE *end = &image[i][width - 1];

        for (;  start < end;  --end, ++start) {
            BYTE tempR, tempB, tempG;

            tempR = start->rgbtRed;
            tempB = start->rgbtBlue;
            tempG = start->rgbtGreen;

            start->rgbtRed = end->rgbtRed;
            start->rgbtBlue = end->rgbtBlue;
            start->rgbtGreen = end->rgbtGreen;

            end->rgbtRed = tempR;
            end->rgbtBlue = tempB;
            end->rgbtGreen = tempG;
        }
    }
}