sdl/surface.go: Fix incorrect pixel color reading for RGB888 in Surface.At

[tmazeika]

Given the following drawing code:

surface, err := window.GetSurface()
if err != nil {
    panic(err)
}
surface.FillRect(nil, 0xffffff00) // yellow

cpy, err := surface.Duplicate()
if err != nil {
    panic(err)
}
cpy.FillRect(nil, 0xff996633) // brown
// draws the bottom-right box:
for y := 110; y < 200; y++ {
    for x := 110; x < 200; x++ {
        surface.Set(x, y, cpy.At(x, y))
    }
}
// draws the top-left box:
cpy.Blit(&sdl.Rect{10, 10, 90, 90}, surface, &sdl.Rect{10, 10, 90, 90})

Actual:

Expected:

It appears that, in Surface.At, the order of pixel data access is incorrect when initializing color.RGBA. This PR fixes that.

[veeableful]

Hi @tmazeika, thanks for submitting a PR and bringing attention to this issue! I was wondering if perhaps thePIXELFORMAT_RGB24, PIXELFORMAT_RGB888 case in the Set() method could be corrected instead? It seems to me that the pixel channels are assigned values backward.

Before:

case PIXELFORMAT_RGB24, PIXELFORMAT_RGB888:
        col := surface.ColorModel().Convert(c).(color.RGBA)
        pix[i+0] = col.B
        pix[i+1] = col.G
        pix[i+2] = col.R

After:

case PIXELFORMAT_RGB24, PIXELFORMAT_RGB888:
        col := surface.ColorModel().Convert(c).(color.RGBA)
        pix[i+0] = col.R
        pix[i+1] = col.G
        pix[i+2] = col.B

[tmazeika]

Good call; changing the test code a bit, I can see that it actually is that exact issue in Surface.Set:

// ...
surface.Set(x, y, color.RGBA{
    R: 0x99,
    G: 0x66,
    B: 0x33,
    A: 0xFF,
})
// ...

This produces the same incorrect blue box. I'll update the PR with that change, thanks.

[tmazeika]

Actually, I spoke too soon: those changes that you made in Surface.Set don't seem to fix the issue. A blue box gets drawn for R: 0x99, G: 0x66, B: 0x33, A: 0xFF. However, when I change the test code to:

// ...
at := cpy.At(x, y)
// Breakpoint:
// at.R = 0x33
// at.G = 0x66
// at.B = 0x99
// at.A = 0xFF
surface.Set(x, y, at)
// ...

I believe that this means the error must be in Surface.At because the channels are wrong before Set is called. Even changing the FillRect() color to sdl.MapRGB(cpy.Format, 0x99, 0x66, 0x33), just to make sure there's no endianness issues, produces the same incorrect channels.

[veeableful]

I think you're right! The problem does seem to be in the At() function. Do you think it's better to use SDL2's GetRGB to make sure it is correct for other cases like big-endian systems as well? For example:

    case PIXELFORMAT_RGB888:
        r, g, b := GetRGB(*((*uint32)(unsafe.Pointer(&pix[i]))), surface.Format)
        return color.RGBA{r, g, b, 0xff}

[tmazeika]

That makes sense to me; maybe we can take it a step further and replace the entire switch statement with:

r, g, b, a := GetRGBA(*((*uint32)(unsafe.Pointer(&pix[i]))), surface.Format)
return color.RGBA{r, g, b, a}

This produces the same result, deals with formats with an alpha component (or without, given that a gets 0xff for RGB888), and simplifies the code. I'm not sure of the reason for the commented-out cases above RGB888.

[veeableful]

Oh I didn't think GetRGBA() would work with RGB format as well, that's good to know. Then definitely, I think it's much better compared to before since the previous code wasn't working correctly. I believe the code was simply not finished and tested which is why some of it is commented out but it should be fine to update over it!

[tmazeika]

Awesome, I think this PR is ready then on my end :)

[veeableful]

Thank you very much for the change! I shall apply the fix to v0.4.x branch and tag it as v0.4.19 as well.