Surface panics on color conversions due to nonidiomatic color models

[mokiat]

Go version: 1.21.3 Go-SDL2 version: 0.4.35 SDL2 version: irrelevant OS: Linux Architecture: amd64

There is a problem with how Surface handles colors.

I have the following fragment of code and in some situations it panics and in others it does not:

img, err := openImage(cfg.locator, cfg.icon)
if err != nil {
    return fmt.Errorf("failed to open icon %q: %w", cfg.icon, err)
}
bounds := img.Bounds()

surface, err := sdl.CreateRGBSurfaceWithFormat(0, int32(bounds.Dx()), int32(bounds.Dy()), 32, sdl.PIXELFORMAT_BGRA8888)
if err != nil {
    return fmt.Errorf("error creating surface: %v", err)
}
draw.Draw(surface, surface.Bounds(), img, image.Point{}, draw.Src)

window.SetIcon(surface)

(Side note: I am not sure if this is the most optimal way to set an icon but I did not find another)

All of this depends on the color format of the image but it should not, since func (surface *Surface) Set(x, y int, c color.Color) { deals with the Color abstraction and it should handle any input.

The problem is due to how color models are implemented: https://github.com/veandco/go-sdl2/blob/9405dd390eb0a6e9076b629debd80f7f4332b47c/sdl/pixels.go#L809-L815

This returns a color.RGBA color sometimes and BGRA8888 other times. From my understanding of the Go's color package, the model should always convert to one single type - the one specific to the model. So in this case it should always return BGRA8888.

The current implementation results in a panic when trying to convert the color to the respective model type: https://github.com/veandco/go-sdl2/blob/9405dd390eb0a6e9076b629debd80f7f4332b47c/sdl/surface.go#L809-L814

It assumes (normally a correct assumption) that it will get a BGRA8888 type but this is not the case due to the color.RGBA branch above.

[mokiat]

P.S. I am also seeing that the RGBA methods of the different color types are not per spec as well. They need to return alpha-premultiplied 16bit color components but in most cases they return 8bit non-alpha-premultiplied components.

Example: https://github.com/veandco/go-sdl2/blob/9405dd390eb0a6e9076b629debd80f7f4332b47c/sdl/pixels.go#L737-L747

You can compare this implementation with the official NRGBA color, which is the same as RGBA8888: https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/image/color/color.go;l=60-76

You can see how they convert a non-alpha-premultiplied color into an alpha-premultiplied 16bit-per-component color.

I might be open to opening a PR for this through I would have a hard time testing it on my own and it might be breaking change for people (though I think it is worth it).

[mokiat]

Hm... after further analysis, I see that the Set function always deals with 8bit RGBA components that it then manually shuffles and reassigns. It might be sufficient to do the following at the start of the method:

nrgbaColor := color.NRGBAModel.Convert(c).(color.NRGBA)
r, g, b, a := nrgbaColor.R, nrgbaColor.G, nrgbaColor.B, nrgbaColor.A

// the r, g, b, a components above are guaranteed to be 8bit non-alpha-premultiplied
// so we can use them in the switch statement, without having to cast anything more

switch surface.Format.Format {
case PIXELFORMAT_ARGB8888:
    pix[i+0] = b
    pix[i+1] = g
    pix[i+2] = r
    pix[i+3] = a
case .... // and so on, always using the r, g, b, a values from above
}

[mokiat]

I have now implemented a local workaround Wrapper that fixes the problem. It looks as follows.

```go import ( "image/color" "unsafe" "github.com/veandco/go-sdl2/sdl" ) // The following wrapper attempts to workaround // https://github.com/veandco/go-sdl2/issues/580 func WrapSurface(surface *sdl.Surface) WrapperSurface { return WrapperSurface{ Surface: surface, } } type WrapperSurface struct { *sdl.Surface } // Set the color of the pixel at (x, y) using this surface's color model to // convert c to the appropriate color. This method is required for the // draw.Image interface. The surface may require locking before calling Set. func (s WrapperSurface) Set(x, y int, c color.Color) { nrgbaColor := color.NRGBAModel.Convert(c).(color.NRGBA) colR, colG, colB, colA := nrgbaColor.R, nrgbaColor.G, nrgbaColor.B, nrgbaColor.A pix := s.Pixels() i := int32(y)*s.Pitch + int32(x)*int32(s.Format.BytesPerPixel) switch s.Format.Format { case sdl.PIXELFORMAT_ARGB8888: pix[i+0] = colB pix[i+1] = colG pix[i+2] = colR pix[i+3] = colA case sdl.PIXELFORMAT_ABGR8888: pix[i+3] = colR pix[i+2] = colG pix[i+1] = colB pix[i+0] = colA case sdl.PIXELFORMAT_RGB24, sdl.PIXELFORMAT_RGB888: pix[i+0] = colB pix[i+1] = colG pix[i+2] = colR case sdl.PIXELFORMAT_BGR24, sdl.PIXELFORMAT_BGR888: pix[i+2] = colR pix[i+1] = colG pix[i+0] = colB case sdl.PIXELFORMAT_RGB444: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 4 & 0x0F g := uint32(colG) >> 4 & 0x0F b := uint32(colB) >> 4 & 0x0F *buf = r<<8 | g<<4 | b case sdl.PIXELFORMAT_RGB332: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 5 & 0x0F g := uint32(colG) >> 5 & 0x0F b := uint32(colB) >> 6 & 0x0F *buf = r<<5 | g<<2 | b case sdl.PIXELFORMAT_RGB565: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 2 & 0xFF b := uint32(colB) >> 3 & 0xFF *buf = r<<11 | g<<5 | b case sdl.PIXELFORMAT_RGB555: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF *buf = r<<10 | g<<5 | b case sdl.PIXELFORMAT_BGR565: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 2 & 0xFF b := uint32(colB) >> 3 & 0xFF *buf = b<<11 | g<<5 | r case sdl.PIXELFORMAT_BGR555: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF *buf = b<<10 | g<<5 | r case sdl.PIXELFORMAT_ARGB4444: buf := (*uint32)(unsafe.Pointer(&pix[i])) a := uint32(colA) >> 4 & 0x0F r := uint32(colR) >> 4 & 0x0F g := uint32(colG) >> 4 & 0x0F b := uint32(colB) >> 4 & 0x0F *buf = a<<12 | r<<8 | g<<4 | b case sdl.PIXELFORMAT_ABGR4444: buf := (*uint32)(unsafe.Pointer(&pix[i])) a := uint32(colA) >> 4 & 0x0F r := uint32(colR) >> 4 & 0x0F g := uint32(colG) >> 4 & 0x0F b := uint32(colB) >> 4 & 0x0F *buf = a<<12 | b<<8 | g<<4 | r case sdl.PIXELFORMAT_RGBA4444: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 4 & 0x0F g := uint32(colG) >> 4 & 0x0F b := uint32(colB) >> 4 & 0x0F a := uint32(colA) >> 4 & 0x0F *buf = r<<12 | g<<8 | b<<4 | a case sdl.PIXELFORMAT_BGRA4444: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 4 & 0x0F g := uint32(colG) >> 4 & 0x0F b := uint32(colB) >> 4 & 0x0F a := uint32(colA) >> 4 & 0x0F *buf = b<<12 | g<<8 | r<<4 | a case sdl.PIXELFORMAT_ARGB1555: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF a := uint32(0) if colA > 0 { a = 1 } *buf = a<<15 | r<<10 | g<<5 | b case sdl.PIXELFORMAT_RGBA5551: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF a := uint32(0) if colA > 0 { a = 1 } *buf = r<<11 | g<<6 | b<<1 | a case sdl.PIXELFORMAT_ABGR1555: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF a := uint32(0) if colA > 0 { a = 1 } *buf = a<<15 | b<<10 | g<<5 | r case sdl.PIXELFORMAT_BGRA5551: buf := (*uint32)(unsafe.Pointer(&pix[i])) r := uint32(colR) >> 3 & 0xFF g := uint32(colG) >> 3 & 0xFF b := uint32(colB) >> 3 & 0xFF a := uint32(0) if colA > 0 { a = 1 } *buf = b<<11 | g<<6 | r<<1 | a case sdl.PIXELFORMAT_RGBA8888: pix[i+3] = colR pix[i+2] = colG pix[i+1] = colB pix[i+0] = colA case sdl.PIXELFORMAT_BGRA8888: pix[i+3] = colB pix[i+2] = colG pix[i+1] = colR pix[i+0] = colA default: panic("Unknown pixel format!") } } ```

Not only does it not panic anymore, but it also now fixes an issue I had with transparent colors. Please tell me if I could open a PR for this.

[veeableful]

Hi @mokiat, thanks for opening up an issue and looking into it! Of course, please open a pull request and I will review and merge it.