Cs50 sepia problem with converting image from normal to sepia

Question

I'm working on a cs50 program called filter(less comfortable, week 4), and it has to transfer images from normal to sepia. It's working fine unless it has to transfer the color white. When trying to transfer the color white, it just converts it to blue and green. Like so:

ORIGINAL

SEPIA

As you can see, it converted everything fine, except for the white or close-to-white colors. Here's my code(Sepia part only):


void sepia(int height, int width, RGBTRIPLE image[height][width])
{
    for(int j = 0; j < width; j++)
    {
       for (int i = 0; i < height; i++)
       {
           int sepiared =  image[i][j].rgbtRed *.393  +   image[i][j].rgbtGreen *.769 +  image[i][j].rgbtBlue *.189;
           int sepiagreen =  image[i][j].rgbtRed *.349  +  image[i][j].rgbtGreen *.686 +  image[i][j].rgbtBlue *.168;
            int sepiablue =  image[i][j].rgbtRed *.272  +   image[i][j].rgbtGreen *.534 +  image[i][j].rgbtBlue *.131;
           image[i][j].rgbtRed = sepiared;
           image[i][j].rgbtGreen = sepiagreen;
           image[i][j].rgbtBlue = sepiablue;
       }
    }

    return;
}

Please help me understand why this happens. Clang prints no error messages.

Yours truly, Lost in code:)

You might try some calculations what your maximum values for the sub channels are. Then think about what will happen if you assign that value to the fields in your `RGBTRIPLE`. — Gerhardh, Aug 14 '20 at 16:33
Not related to your question but for passing the test you shold think about rounding the result to nearest integer value. — Gerhardh, Aug 14 '20 at 17:23

score 2 · Answer 1 · answered Aug 14 '20 at 19:33

First, as an aside, when you have a CS50 question, please search for the keywords in StackOverflow before asking. The answer is probably there. If you search for sepia RGBTRIPLE cs50 you get quite a few hits.

After doing a lot of pixel processing, you'll hone some useful debugging intuitions. Among those:

If you see diagonal offsets, your image's bytes-per-row may be greater than the width times the pixel size. (Especially in YCbCr images or on platforms where image buffers are aligned to 128-bit vector sizes.)
2x or 0.5x image displays probably mean you're not heeding the scale value on a Retina display.
Certain colorspace errors will point you immediately to BGR vs RGB byte ordering issues. No blue channel at all or all blue? Probably mixing ARGB with BGRA.

But more to the point:

If you see wackiness in the bright or color-saturated areas, your pixel component values are probably oversaturating (exceeding the maximum value, and dropping the high bit(s)).

Every time you either (1) multiply a color component by a number great than 1 or (2) add multiple color components together, you need to think about what happens if you exceed the maximum value. If your intermediate math will add two values and then divide by 2, make sure your compiled operations will be using a large enough variable size to hold that extra bit.

So in your inner loop here, when operating on a white pixel, almost every color component will exceed 255 (i.e. red and green will exceed, but not blue, because sepia is low in blue):

int sepiared =  image[i][j].rgbtRed *.393  +   image[i][j].rgbtGreen *.769 +  image[i][j].rgbtBlue *.189;
int sepiagreen =  image[i][j].rgbtRed *.349  +  image[i][j].rgbtGreen *.686 +  image[i][j].rgbtBlue *.168;
int sepiablue =  image[i][j].rgbtRed *.272  +   image[i][j].rgbtGreen *.534 +  image[i][j].rgbtBlue *.131;

The resulting value would be {255, 255, 255} x {.393+.769+.189, .349+.686+.168, .272+.534+.131}, or {344.5, 306.8, 238.9}.

But because you don't have that enough bits for those values in the BYTE components of an RGBTRIPLE struct, your values will be incorrect. So instead, you can do this:

int sepiared =  (int) image[i][j].rgbtRed *.393  +   image[i][j].rgbtGreen *.769 +  image[i][j].rgbtBlue *.189;
int sepiagreen =  (int) image[i][j].rgbtRed *.349  +  image[i][j].rgbtGreen *.686 +  image[i][j].rgbtBlue *.168;
int sepiablue =  (int) image[i][j].rgbtRed *.272  +   image[i][j].rgbtGreen *.534 +  image[i][j].rgbtBlue *.131;
sepiared = min(sepiared, 255);
sepiagreen = min(sepiagreen, 255);
sepiablue = min(sepiablue, 255);

Note that I have made two changes:

Cast the first value in each expression to (int); otherwise the calculation will be done on bytes and you'll lose your high bit.
Enforce the maximum value of 255 on each pixel component.

When considering the other answers, please add my first fix. Checking for a max of 255 won't help if you've already dropped your high bit!

Thanks alot I understand now. I appreciate your help. – Lost in code Aug 15 '20 at 09:05 — Lost in code, Aug 15 '20 at 09:05

Craig Estey · Answer 2 · 2020-08-14T17:42:35.630

You need to use "saturation math".

For near white colors, your intermediate values (e.g. sepiared) can exceed 255.

255 (0xFF) is the maximum value that can fit in an unsigned char

For example, if sepiared were 256 (0x100), when it gets put into rgbtRed, only the rightmost 8 bits will be retained and the value will be truncated to 0. So, instead of a very bright value [near white], you'll end up with a very dark value [near black].

To fix this, add:

if (sepiared > 255)
    sepiared = 255;

Also, note with the ordering of your for loops, it is very cache inefficient.

And, it's wasteful [and can be slow] to use image[i][j].whatever everywhere. Better to use a pointer to the current pixel.

Anyway, here's an updated version of your code with these changes:

void
sepia(int height, int width, RGBTRIPLE image[height][width])
{
    RGBTRIPLE *pix;

    for (int i = 0; i < height; i++) {
        pix = &image[i][0];
        for (int j = 0; j < width; j++, pix++) {
            int sepiared = pix->rgbtRed * .393 +
                pix->rgbtGreen * .769 +
                pix->rgbtBlue * .189;
            int sepiagreen = pix->rgbtRed * .349 +
                pix->rgbtGreen * .686 +
                pix->rgbtBlue * .168;
            int sepiablue = pix->rgbtRed * .272 +
                pix->rgbtGreen * .534 +
                pix->rgbtBlue * .131;

            if (sepiared > 255)
                sepiared = 255;
            if (sepiagreen > 255)
                sepiagreen = 255;
            if (sepiablue > 255)
                sepiablue = 255;

            pix->rgbtRed = sepiared;
            pix->rgbtGreen = sepiagreen;
            pix->rgbtBlue = sepiablue;
        }
    }
}

Also, note that it can be a bit slow to use floating point math on pixel images. In this case, it's faster/better to use scaled integer math.

Here's a version that does that:

void
sepia(int height, int width, RGBTRIPLE image[height][width])
{
    RGBTRIPLE *pix;

    for (int i = 0; i < height; i++) {
        pix = &image[i][0];
        for (int j = 0; j < width; j++, pix++) {
            int sepiared = pix->rgbtRed * 393 +
                pix->rgbtGreen * 769 +
                pix->rgbtBlue * 189;
            int sepiagreen = pix->rgbtRed * 349 +
                pix->rgbtGreen * 686 +
                pix->rgbtBlue * 168;
            int sepiablue = pix->rgbtRed * 272 +
                pix->rgbtGreen * 534 +
                pix->rgbtBlue * 131;

            sepiared /= 1000;
            sepiagreen /= 1000;
            sepiablue /= 1000;

            if (sepiared > 255)
                sepiared = 255;
            if (sepiagreen > 255)
                sepiagreen = 255;
            if (sepiablue > 255)
                sepiablue = 255;

            pix->rgbtRed = sepiared;
            pix->rgbtGreen = sepiagreen;
            pix->rgbtBlue = sepiablue;
        }
    }
}

score 1 · Accepted Answer · answered Aug 14 '20 at 18:32

So You Need To Edit Your Code Sligitly

Store Orginal Image To a temp for a while


            originalBlue = image[i][j].rgbtBlue;
            originalRed = image[i][j].rgbtRed;
            originalGreen = image[i][j].rgbtGreen;

the result of each of these formulas may not be an integer so use float and round them to nearest integer

            sepiaRed = round(.393 * originalRed + .769 * originalGreen + .189 * originalBlue);
            sepiaGreen = round(.349 * originalRed + .686 * originalGreen + .168 * originalBlue);
            sepiaBlue = round(.272 * originalRed + .534 * originalGreen + .131 * originalBlue);

            if (sepiaRed > 255)
            {
                sepiaRed = 255;
            }

            if (sepiaGreen > 255)
            {
                sepiaGreen = 255;
            }

            if (sepiaBlue > 255)
            {
                sepiaBlue = 255;
            }

now store the values to the orignal ones

            image[i][j].rgbtBlue = sepiaBlue;
            image[i][j].rgbtRed = sepiaRed;
            image[i][j].rgbtGreen = sepiaGreen;

Declare all Variable Outside For Loop

    float sepiaRed;
    float sepiaBlue;
    float sepiaGreen;
    int originalRed;
    int originalBlue;
    int originalGreen;

I Hope It's Help

score 0 · Answer 4 · answered Sep 16 '22 at 11:00

0

For me it worked when I kept all the variables outside the loop and rounded off in the end after checking the value of each R G B less than 255.

answered Sep 16 '22 at 11:00

Kripa

1

As it’s currently written, your answer is unclear. Please [edit] to add additional details that will help others understand how this addresses the question asked. You can find more information on how to write good answers [in the help center](/help/how-to-answer). – Community Sep 21 '22 at 18:34

Cs50 sepia problem with converting image from normal to sepia

4 Answers4

Linked