pixel: add NewImageFromBytes function #713
Conversation
…existing slice Signed-off-by: deadprogram <[email protected]>
Signed-off-by: deadprogram <[email protected]>
| @@ -43,6 +43,40 @@ func NewImage[T Color](width, height int) Image[T] { | |||
| } | |||
| } | |||
|
|
|||
| // NewImageFromBytes creates a new image of the given size using an existing data slice of bytes. | |||
| func NewImageFromBytes[T Color](width, height int, buf []byte) Image[T] { | |||
There was a problem hiding this comment.
Should this return (Image[T], error) instead of panicking?
There was a problem hiding this comment.
That is the current interface, which I was not trying to change in this PR.
|
Thanks, merged. In a later PR we might change the interface as proposed, but merge this now to keep things simple. |
This reverts commit 4fb5d7a.
|
I undo this with the revert because it makes the test fails even thought the build succeeded before I merged it 🤔 |
| if len(buf) != width*height*int(unsafe.Sizeof(zeroColor)) { | ||
| panic("NewImageFromBytes: data slice size mismatch") | ||
| } | ||
| data = unsafe.Pointer(&buf[0]) |
There was a problem hiding this comment.
This isn't safe. If you create this from a plain old byte slice ([]byte) and the alignment of the new slice is different, this can lead to crashes on simpler cores like Cortex-M0 and I think even miscompiles (undefined behavior) which can result in any behavior and will be difficult to debug.
This applies to RGB565BE and RGB555 which have an underlying type of []uint16.
To check for this, you can do something like this:
if zeroColor.BitsPerPixel() % 8 == 0 && uintptr(data) % unsafe.Alignof(zeroColor) != 0 {
panic("NewImageFromBytes: byte buffer is not aligned")
}But really, if you allocate the slice using any normal Go way (global variable, make([]byte, ...), etc) there is no guarantee the byte slice is correctly aligned. If you want to be absolutely sure and don't want to rely on this behavior (and therefore exclude formats like RGB565BE and RGB555), you can also use a check like this:
if zeroColor.BitsPerPixel() % 8 == 0 && unsafe.Alignof(zeroColor) > 1 {
panic("NewImageFromBytes: cannot convert byte buffer to image (different alignment)")
}There was a problem hiding this comment.
I will add a new PR with that additional check, since this code is specifically for mono.
| *((*byte)(ptr)) |= 1 << uint8(y%8) | ||
| *((*byte)(ptr)) |= (1 << (7 - uint8(x%8))) | ||
| } else { | ||
| *((*byte)(ptr)) &^= 1 << uint8(y%8) | ||
| *((*byte)(ptr)) &^= (1 << (7 - uint8(x%8))) |
There was a problem hiding this comment.
You are changing the format here! Specifically, this reverses the order of the bits in the Monochrome color type, thus breaking programs that were using pixel.Monochrome before with the old format.
If you want to add a new format, please do so by adding a new pixel type instead.
There was a problem hiding this comment.
I think I was the only one trying to use this format, and I did not complete that work (on Thumby) due to this code being incorrect in the current form. So I am pretty sure this is fixing something broken, not introducing a new format.
There was a problem hiding this comment.
Ok, seems reasonable.
Though we might want to add a test for this: just a small monochrome image (5 x 3 pixels or so - intentionally with a weird size) and check whether it is equivalent to a PNG image for example.
There was a problem hiding this comment.
Good suggestion, I will do that in the same PR in which I add the alignment check.
| y := index / int(img.width) | ||
| offset := x + (y/8)*int(img.width) | ||
| offset := index / 8 |
There was a problem hiding this comment.
You are also changing the format here! The offset calculation changed and now outputs different values.
This PR adds a new function
NewImageFromBytes()to create apixel.Imagefrom an existing slice of bytes.It is already being used as part of some refactoring that @conejoninja and I are doing on the badger2040 repo.
Thanks to @conejoninja for helping me debug these changes!