Pixel/image buffers

[aykevl]

This PR refactors pixel buffer handling.

Summary

I'd like to add a new pixel package for efficiently working with image buffers for displays, and add a DrawBitmap(x, y, bitmap) method to send these image buffers directly to a SPI display. This is intended as preparation for DMA support in displays (StartDrawBitmap) and should replace DrawRGBBitmap8. I've already done lots of tests with this design outside the drivers package and I think it works well.

Details

I've added a new package to efficiently work with pixel buffers in various formats: tinygo.org/x/drivers/pixel. This isn't new code: I've been using it for a while in this package: https://pkg.go.dev/github.com/aykevl/tinygl/pixel. It's pretty fast, see for example the demos in this thread.

The basic idea is as follows:

• Use the native pixel format of a particular display, so that pixel data can be streamed out via SPI/DMA directly. This means not using color.RGBA for example, which would be very wasteful (often, half the bits are unused).
• Support less common pixel formats like RGB444. They don't fit neatly on byte boundaries and so needed a radical new design for pixel buffers: no []T but pixel.Image[T].
• Generics are used to select the pixel format. This allows adding multiple formats to a driver package without hurting performance. Backwards compatibility is kept by using the previous default (RGB565) with the old functions (e.g. st7789.New vs st7789.NewOf).
  The advantage of generics is that it is trivially easy to use multiple pixel formats in a single program for different displays if needed, something that LVGL doesn't support (the pixel format is a compile time constant).
• Graphics operations should be done on pixel buffers first and then sent as a whole. This means no SetPixel: it was probably a mistake (it's too slow). One such package is https://github.com/aykevl/tinygl/ which we might eventually want to move to tinygo-org. Another example is the refactored pyportal_boing example (which actually got faster with the refactor!).

Comparisons with other approaches:

• The Adafruit GFX library hardcodes RGB565 little endian. This works for a large number of screens, but has a few problems:
  • When sending 8-bit color data to displays, most displays need the bytes swapped before transmission. This can be done by setting the SPI bus to 16-bit, but not all hardware supports this. It seems much easier to me to work with big endian 16-bit values throughout, as I've done with pixel.RGB565BE.
  • No other color format is supported, like RGB444 (that I use for the PineTime to speed up graphics) or black/white displays (like e-ink). I guess they all need to convert from RGB565 to a different format when drawing.
• LVGL (commonly used open source graphics library) only has a fixed list of color formats. It doesn't support more niche color formats like RGB444 or RGB555 while the design of the pixel package can easily support this. (It does however support swapping the byte order through LV_COLOR_16_SWAP).

The main reason I'm doing this is because I would like to add DMA support to most displays using a very similar method: StartDrawBitmap (and IsAsync / Wait). I figured it would be much easier to add this as a synchronous API and only do the async one once the synchronous one is agreed upon. I didn't want to use StartDrawRGBBitmap8 with a byte array - I think that's just not a great API and too easy to make mistakes with.

Next up:

• I'd like to add a display driver for the GameBoy Advance, which uses yet another custom color format: RGB555 (already in tinygl, not yet in this PR).
• I might add a pixel.Gray1 (black/white) pixel format, for e-ink screens.
• Once this is merged, I'll make a draft PR for StartDrawBitmap (see machine: add SPI DMA support for the rp2040 and samd51 tinygo#3985).

Anyway, looking forward to feedback on this. Questions? Comments? Is this the direction we want to take with TinyGo graphics? I know generics might complicate things but I really think it's the best tool for the job here.

[soypat]

A suggestion based on personal preference when working with generics:

Generic data structures can get tedious to write out and decrease legibility. I've found that using type aliases to declare shorthand common generic types is quite idiomatic:

type Image = pixel.Image[pixel.RGB565BE]

You then use the Image type anywhere you'd use pixel.Image[pixel.RGB565BE]. A caveat (or benefit even) is that users will see the Image type if you export it, so you can even provide the convenience ili9341.Image type along with your device library so that generics don't pollute user facing APIs.

The greatest benefit of type aliases for generics I think comes from not having to add type parameters to structures which contain the type as one of the fields, thus preventing viral spreading of type parameters in a codebase.

[aykevl]

I guess I could add it, if needed. Though I don't see the need for it? I haven't needed anything like this for the three small projects I used the pixel.Image API with.

The greatest benefit of type aliases for generics I think comes from not having to add type parameters to structures which contain the type as one of the fields, thus preventing viral spreading of type parameters in a codebase.

Yes, generics can get quite viral. See for example my tinygl package, which has the type parameter on almost every type/struct/method: https://pkg.go.dev/github.com/aykevl/tinygl
However, I don't really see a way around this. For a specific application (where you are fine with hardcoding the pixel format) you can certainly make a type alias like this. But when you do that, you lose the ability to switch the pixel format. (For example: for my watch I use RGB444 instead of RGB565 for improved speed, and the simulator uses RGB888).

So in short, I could add ili9341.Image, but I'm not sure it's really necessary/useful. And users can define it themselves if they really want to.

---

EDIT: oh, apparently the ili9341 display only supports RGB565 and RGB666 (and I don't think RGB666 is used much in practice). In that case, I guess it makes sense to define ili9341.Image.

[soypat]

Yes! agreed 100%, including the last EDIT note! 🧡.

[bgould]

This looks really nice.

I might add a pixel.Gray1 (black/white) pixel format, for e-ink screens.

FWIW the sh1106 probably could use the above format as well: https://github.com/tinygo-org/drivers/blob/release/sh1106/sh1106.go#L203-L226

One thing that comes to mind for me is the thought that pixel.Image could possibly be "windowed" to provide sort of a virtual scroll buffer. I've thought such a thing might be useful for instance in tinyterm

For example, if defining a pixel.Image that has equivalent width but greater height than the display, and then tracking a "scroll offset" value, then it might be possible to allowing drawing off-screen and scroll it into view by adjusting the offset.

[aykevl]

FWIW the sh1106 probably could use the above format as well: https://github.com/tinygo-org/drivers/blob/release/sh1106/sh1106.go#L203-L226

I don't think I have this display, but yeah that looks like a place where it would be useful!

One thing that comes to mind for me is the thought that pixel.Image could possibly be "windowed" to provide sort of a virtual scroll buffer. I've thought such a thing might be useful for instance in tinyterm

You mean storing the entire contents of the scroll buffer in-memory?
Usually there isn't nearly enough memory for that sort of thing. In fact, most boards don't even have enough memory for a single framebuffer, hence why SPI displays typically have a built-in framebuffer.
(The reason why using a framebuffer works for a display like the sh1106 is because it's small and each pixel is only 1 bit, instead of 16 like for most LCD displays).

For TinyGl I have a slightly different approach: https://pkg.go.dev/github.com/aykevl/tinygl#ScrollBox
Basically I will call the Update method of the contents (basically, the redraw function) with a separate (x, y) for the display and for the contents. That way, it'll redraw exactly the part of the contents that is needed. And it only needs small buffers, not the entire screen.

Related: https://hachyderm.io/@ayke/110554855872552126 (but with some patches I still haven't pushed due to bugs)

In short, I don't think adding a scroll offset is a good way to do this. You'll quickly run into memory issues on common MCU/display combinations and there are alternatives for getting the same scroll effect.

[bgould]

Ok thank you for explaining. The TinyGl mehod you mentioned is close to what I what thinking, but it makes sense that it at a different layer of the "stack".

[aykevl]

Made a few small changes, see the force push: 0135459...db73aad.

• Added the ili9341.Image type.
• Clarified the pixel format for RGB565BE and RGB444BE, with a rationale as to why they're encoded the way they are.

[soypat]

After a closer look there are some things I think are worth a second look at, mainly unsafe memory access.

[aykevl]

@soypat found a pretty big oversight on my part: bounds checking for the Set/Get methods. I've added the bounds checks, and checked the performance difference:

• Rendering the home screen of https://github.com/aykevl/things/tree/master/badge got slightly faster, I don't know how but I'll take the win.
• Rendering the first frame of https://github.com/aykevl/things/tree/master/games/brick-breaker got a bit slower, about 1ms (21.7ms -> 22.7ms). I believe this is because I use a naive implementation for drawing rectangles: it simply sets each individual pixel while there could be a faster implementation that draws a rectangle like the FillSolidColor. I'll leave this as a future improvement. For now, there could be a slight decrease in performance but it's necessary.

[soypat]

Idea: might it be worth passing in the desired frame buffer to NewImage as a third argument? A couple of benefits I've come up with:

• Code readability improvements in generics department: Since the buffer has the color type there is no need to pass in a type parameter.
• Ease of use for people who don't want allocations: Makes it easier for users who want to reuse buffers and possible make images within images (given they use a new strided Image type for subimages)

If the user passes in a nil buffer then NewImage could automatically assign it a size. NewImage should however calculate the minimum size as it does now and make sure the argument buffer size meets the minimum size requirements.

[aykevl]

Interesting idea. Right now the buffer is an implementation detail, and I like to keep it that way (at least for now): calculating the size of the buffer is somewhat non-trivial for pixel sizes that don't neatly fit on byte boundaries (like RGB444 and perhaps others in the future like black&white with one bit per pixel). In fact RGB444 uses []byte which wouldn't help with generics.

If there is a practical need for this (not just a theoretical one), we can always add a NewImageWithBuffer (name TBD) that can reuse an existing buffer.

[soypat]

Fair enough, yeah RGB444 would make it hard for users to use and NewImageWithBuffer idea is very sound

[deadprogram]

Thanks @aykevl for this set of features and thank you @soypat for the indepth reviews/suggestions! Now merging.

[sparques]

@aykevl Apologies if this is not the right place to ask, but I was curious why do colors in the pixel package have an RGBA() color.RGBA method rather than RGBA() (r, g, b, a uint32) method? With the latter, they implement color.Color and it's much easier to use these memory efficient pixel formats with the standard library.