Implement go's image and image/draw interfaces

[akiross]

Hello, I would like to propose to implement the image.Image and https://golang.org/pkg/image/draw/#Image for sdl.Surface.

Actually, I am a bit surprised they are not already implemented: it is not so common among programming languages to have a standardized package for managing images and color, but go does and I think you should take advantage of it.

The reason for this proposal is that it should not take much effort, but it should instantly give benefit and access to a wide range of packages that already use such interfaces. For instance, the go packages themselves, as well as rendering libraries such as https://github.com/llgcode/draw2d (given that SDL rendering is quite limited).

I think that one might object that this is not part of the original SDL, but I think that it is important for a library to be well integrated in the language's ecosystem.

[veeableful]

Hi @akiross!

I definitely agree that we should provide such interfaces if it means the binding feels more natural for the programming language. There is actually another branch where I was working on that but unfortunately hasn't been continued for awhile.

Are you proposing to implement them yourself? If not, I can work on that. Thanks for the reminder either way!

[akiross]

Hello @veeableful, it's great to know there's a branch, I just took a peek at it. I would love to contribute, but I think I miss some important knowledge on SDL image formats to work on it, so I would need careful review.

I must say that I tried to work on it by myself, implementing a new type Surface sdl.Surface, but I kinda failed/given up: I give you here some thoughts I had while working on it.

First, I noticed that SDL_Surface must be locked before accessing raw data: this means there is a risk that data gets corrupted if surfaces are not properly locked when used. This could be a next step, but maybe it would be nice to warn the user if surfaces are not being used properly.

Second, color models are of course a major issue here. I still did not understand if the existing image.Color models can be used with no modifications, or if new models must be defined for all the SDL_PixelFormatsEnums, like SDL_PIXELFORMAT_RGB332. It should not be too complicated to do so, basically we just need to provide a function that maps any given format to pre-multiplied RGBA. Maybe, some more work should be made to implement palettes. I noticed that image.Color has a https://golang.org/pkg/image/color/#Palette type which is just an array of colors, but I suspect that it is needed to implement a color model for SDL indexed. I'm still not understanding why a go Palette is a different type and not a color model. (Palette IS a color model, as there's a Convert method). Anyway, we could just start providing basic RGBA support and tackle palettes later.

Third, I am not really sure about how to access raw pixel data in a safe way, but I think an example of this is shown in the aforementioned branch :)

If you can work on it, it's great. I'll play a little bit with it this morning, to see if I can use the branch code to come up with a new type implementing image.Image and draw.Image and report later.

[veeableful]

Hi @akiross,

Yeah, it probably needs certain degree of SDL2 knowledge (which I got to admit: I'm not anywhere near an expert myself). I haven't worked on that code for awhile and probably need to set aside some time on Saturday to get into it again and properly work on this.

For the Surface locking, maybe we can change the existing Lock() method. Currently it only returns an error and the user would have to manually call Pixels() to modify it. Maybe we can make it more efficient by directly returning a type that implements the Image interface. But Go doesn't have destructor, so I guess we have to remember to use something like defer lock.Unlock() to unlock the surface. Now I really wish Go had a destructor :(

As an example:

surface, _ := window.GetSurface()
pixels, _ := surface.Lock() // pixels is a type that implements image.Image
defer surface.Unlock()
// do something to the pixels

instead of:

surface, _ := window.GetSurface()
surface.Lock()
pixels := surface.Pixels()  // pixels is an array of bytes
defer surface.Unlock()
// do something to the pixels

[akiross]

Sorry I did not report earlier. I actually tried to implement those interfaces (as a new type), but I got stuck when using them for rendering with existing libraries. I was mistaken in thinking that it could just work out of the box with existing tools like draw2d or gg, because apparently the tool might depend on the specific interface implementation. For example, draw2d relies on truetype's render, and their code do not use At() and Set(), but directly access pixels, therefore they cast the Image interface type to the actual RGBA structure. I opened an issue trying to understand why this design choice.

The code is fairly simple and likely wrong, because I get correct results on the screen when drawing (colors are correct), but in the code channels seem inverted.

About the locking, I ended up manually locking the interface just before the render and unlocking right after it, as you just shown. I don't fancy destructors, but something like a context manager could definitely be useful in this case.

[veeableful]

I think it's fine as long as we implement the interface correctly. It's up to the other libraries to make use of them. My guess is they did it that way for performance reasons.

I think the code you did looks fine! Would you like to open a pull request to merge it?

[akiross]

That's true and I suspect that as well.

Regarding my code, I'm not really happy with it. If it is no rush, I will have time to work on it later this month and I plan to review it thoroughly and have it support other formats as well (e.g. palettes).

[veeableful]

Yes, definitely! Take as much time as you need @akiross. I really appreciate you working on this!

[akiross]

I created a PR for this #369, although it does not include the support for palettes.