sdl: Improve pixel fetching api

[flga]

I think there's two parts to this problem.

1 - do not require the callers to import unsafe in ReadPixels, just take in a slice and assume it's size is correct (easy enough, but I think it would be best to do it separately)
2 - provide a version of this function that does the heavy lifting for the caller (this pr)

I think it's worthwhile keeping the original implementation and just fixing up the unsafe.Pointer.
For one, it's what the actual SDL api looks like, but it also allows the caller to manage his/her buffers.

The basic principle is to take in a rect and a pixel format and fill in the gaps appropriately.
This isn't ready yet, seems to work but it needs more tests, so I'll be marking it as draft for now.
I also need to rethink the testing methodology, this way of creating the expected pixel buffers is unwieldy.

[flga]

@veeableful quick question, why are the alias pixel formats not mapped to their c values directly?

I'm asking this because, my tests for RGB888 pass, but the tests for RGB24 do not, which is weird.
So, I tried applying the same principle as with the *32 aliases and created a standalone RGB24 and mapped it accordingly, now the tests pass.

This has me puzzled.

EDIT: The documentation and source do not match up.
The docs say "SDL_PIXELFORMAT_RGBA32 is an alias for SDL_PIXELFORMAT_RGBA8888 on big endian machines and for SDL_PIXELFORMAT_ABGR8888 on little endian machines, so you can use it to specify that your pixels are represented as RGBA byte arrays, like SDL_PIXELFORMAT_RGB24 is for RGB byte arrays", however PIXELFORMAT_RGB24 is defined as 3 bytes per pixel (as expected) but PIXELFORMAT_RGB888 is defined as 4 bytes per pixel. I'll need to take a deeper look into this.

[flga]

That was painful...
Had issues with surface pixel data not actually being in the format requested (no idea why, disabled RLE, locked the surface, nothing worked). In the end I just did the format conversions by hand, ie, calling ConvertPixels myself, and use a surface just for the convenience of not having to deal with endianess/conversions when painting.

The tests were very flaky with the default renderer (not actually returning valid pixel data sometimes), had to use a SoftwareRenderer instead. Also avoids complications with init and main.

It's working fine now for RGBA formats (except the last one, it's not convertible, gotta figure out what that means still), and it's still missing palette and planar modes.

All this trial and error, + the compilation time drove me absolutely mad 😭
It's unfortunate that when using CGO the whole package gets recompiled.

[veeableful]

The PR is looking great @flga. Thanks for the hard work!

@veeableful quick question, why are the alias pixel formats not mapped to their c values directly?

I don't remember exactly why 😅 I guess that was done to enable it to compile using older SDL2 (e.g. SDL2 2.0.0). Now that I see it, I think I should have declared the missing alias definitions in go-sdl2 for older SDL2. I have pushed the change (hopefully didn't break anything!).

All this trial and error, + the compilation time drove me absolutely mad sob
It's unfortunate that when using CGO the whole package gets recompiled.

Hm.. yeah the compilation speed is quite slow. I wonder if it is possible to split the sdl package further into many small packages and re-export the types and functions so Go can cache all the packages that weren't touched and do parallel compilation.

[flga]

Hey @veeableful, I've been looking into the indexed and yuv pixel formats and they work in a slightly different manner.

Indexed formats require a palette, which is hard to support unless we accept an actual Format instead of a format enum, and yuv seem to be very "edge casey", I've played around with implementing this, and it kinda works, the basic quadrant tests pass, but the concatenated quadrants do not. I feel like this will lead to re-implementing a lot of sdl logic and it doesn't seem all that worth it.

What do you think about reducing this new method into just RGBA (all the formats, although I still need to look into how PIXELFORMAT_ARGB2101010 works)?

Users that need more specialized handling can still use the default, no guard rails, method.
Does that seem reasonable?

In regards to code splitting, in principle yes, that would solve it, however I'm not so sure that it is feasible, seems like it would be easy to fall into circular deps.

[veeableful]

Yeah @flga, absolutely. You could add support for the ones you have implemented now and maybe set error for the ones that are not yet implemented. We could always support them in the future =)

[flga]

@veeableful sorry, seems like I've missed the notification. I'll update the PR tomorrow.

[veeableful]

@flga Don’t worry about it. Take as much time as you need!