Why do i get weird artefacts when changing pixels of an SDL_Surface?

Question

So I am trying to edit every pixels of an SDL_Surface to apply the grayscaling formula. But when running my code I get an area of the width of my entire screen and the height of the surface filled with weird RGB stripes.

void grayscale32(Uint8 *pixels, SDL_PixelFormat *format, int width, int height,
                 int pitch) {
    Uint32 *targetPixel = NULL;
    Uint8 r, g, b, gray;

    for (int y = 0; y * pitch < height * width * 3; y++) {
        for (int x = 0; x < pitch / 4; x++) {
            targetPixel = pixels + y * pitch + x * sizeof(*targetPixel);
            SDL_GetRGB(*targetPixel, format, &r, &g, &b);
            gray = 0.21 * r + 0.72 * g + 0.07 * b;
            *targetPixel = SDL_MapRGB(format, gray, gray, gray);
        }
    }
}

I suppose it's a matter of Byte when converting between Uint8 and Uint32 back and forth but i don't know exactly why. I tried passing Uint8 *pixels as a Uint32 * but it didn't fix it and caused a segmentation fault.

Answer 1

You have vertical banding and each row lines up (vs. stairstepping). So, I believe that your use of 3 is correct (i.e. the pixels are 3 bytes each)

This can/could be caused by a few things.

You could be off by one byte in the row address.

You could be wrapping/truncating pixel color values (i.e. you need saturation math). That is, (e.g.) if you have a calculated gray value of 256, it will wrap/truncate to 0.

I think there's a simpler way to index into the pixel array.

Anyway, here's some refactored code. It's not been tested but it should give you something to look at.

I'm assuming the format is RGB (3 bytes / pixel) [vs RGBA with 4 bytes / pixel] and pitch is the number of bytes per row.

#include <SDL2/SDL.h>
typedef unsigned char Uint8;
typedef unsigned int Uint32;

static inline Uint8
grayof(Uint8 r, Uint8 g, Uint8 b)
{
    Uint8 gray;

#if ORIG
    gray = 0.21 * r + 0.72 * g + 0.07 * b;
#else
    Uint32 acc = 0;

    // use scaled integer arithmetic (vs. float)
    acc += 210 * (Uint32) r;
    acc += 720 * (Uint32) g;
    acc += 70 * (Uint32) b;
    acc /= 1000;

    // saturation math
    // (e.g.) prevent pixel value of 256 from wrapping to 1
    if (acc > 255)
        acc = 255;

    gray = acc;
#endif

    return gray;
}

void
grayscale32(Uint8 *pixels, SDL_PixelFormat *format,
    int width, int height, int pitch)
{
    Uint8 *byteptr;
    Uint32 *targetPixel;
    Uint8 r, g, b, gray;

    for (int y = 0;  y < height;  ++y) {
        byteptr = &pixels[y * pitch];
        targetPixel = (Uint32 *) byteptr;
        for (int x = 0;  x < width;  ++x, ++targetPixel) {
            SDL_GetRGB(*targetPixel, format, &r, &g, &b);

            gray = grayof(r, g, b);

            *targetPixel = SDL_MapRGB(format, gray, gray, gray);
        }
    }
}

---

Without more information [or the original image], it's difficult to tell exactly what the format and geometry are.

Although less likely, here's an alternate indexing scheme:

void
grayscale32(Uint8 *pixels, SDL_PixelFormat *format,
    int width, int height, int pitch)
{
    Uint8 *byteptr;
    Uint32 *targetPixel;
    Uint8 r, g, b, gray;

    for (int y = 0;  y < height;  ++y) {
        byteptr = &pixels[y * pitch];
        for (int x = 0;  x < width;  ++x, byteptr += 3) {
            targetPixel = (Uint32 *) byteptr;
            SDL_GetRGB(*targetPixel, format, &r, &g, &b);

            gray = grayof(r, g, b);

            *targetPixel = SDL_MapRGB(format, gray, gray, gray);
        }
    }
}

---

UPDATE:

I don't think the saturation check is really required here since the factors in the equation add up to 1, so even with an original value of rgb(255, 255, 255) we get a gray of 255.

Yes, you are correct about not needing the sat math. I was being conservative [because I was too lazy to check the factor values :-)].

The first one still gave me stripes despite using the grayof function, but the second one worked perfectly fine by itself.

Okay, this means that each pixel is 3 bytes [R/G/B]. I wasn't sure whether it was 4 bytes with a format of R/G/B/A where A is the alpha value.

But, given that the memory format is 3 byte/RGB this presents an issue with targetPixel being Uint32 *.

That's because when doing *targetPixel = ...;, it's storing 4 bytes but the loop increment is 3. This means that a given store is bleeding one byte into the next pixel area.

This would look something like:

Memory layout:
| 0       3       6       9
| R G B | R G B | R G B | R G B |

Store progression:
| 1 2 3 | 4
        | 1 2 3 | 4
                | 1 2 3 | 4
                        | 1 2 3 | 4

So, effectively the second store is not getting the original R value

It may seem/look okay, but I suspect that the resultant gray values are a bit off.

Here's a version that may fix the problem. You may need to compile with -DALT=1 if the R and B values seem to be reversed. Try it both ways: with/without.

#include <SDL2/SDL.h>
typedef unsigned char Uint8;
typedef unsigned int Uint32;

#ifndef ALT
#define ALT     0
#endif

enum {
#if ALT
    OFF_R = 2,
    OFF_G = 1,
    OFF_B = 0,
#else
    OFF_R = 0,
    OFF_G = 1,
    OFF_B = 2,
#endif

    PIXBYTES = 3
};

static inline Uint8
grayof(Uint8 r, Uint8 g, Uint8 b)
{
    Uint8 gray;

#if ORIG
    gray = 0.21 * r + 0.72 * g + 0.07 * b;
#else
    Uint32 acc = 0;

    // use scaled integer arithmetic (vs. float)
    acc += 210 * (Uint32) r;
    acc += 720 * (Uint32) g;
    acc += 70 * (Uint32) b;
    acc /= 1000;

    // saturation math
    // (e.g.) prevent pixel value of 256 from wrapping to 1
#if SAT
    if (acc > 255)
        acc = 255;
#endif

    gray = acc;
#endif

    return gray;
}

void
grayscale32(Uint8 *pixels, SDL_PixelFormat *format,
    int width, int height, int pitch)
{
    Uint8 *byteptr;
    Uint8 *bytelim;
    Uint8 gray;

    for (int y = 0;  y < height;  ++y) {
        byteptr = &pixels[y * pitch];
        bytelim = &byteptr[width * PIXBYTES];
        for (;  byteptr < bytelim;  byteptr += PIXBYTES) {
            gray = grayof(byteptr[OFF_R], byteptr[OFF_G], byteptr[OFF_B]);
            byteptr[OFF_R] = gray;
            byteptr[OFF_G] = gray;
            byteptr[OFF_B] = gray;
        }
    }
}