Add API to access Bitmap pixel and mask data

[parasyte]

Breaking changes

• Adds a lifetime to BitmapData, so it is no longer easy to store in persistent structs. BitmapData is not allowed to outlive the Bitmap that it came from.
• Makes all BitmapData fields private so they cannot be changed by callers. This is a safety invariant for accessing the underlying pixel and mask buffers.
• Bitmap::clear() and Bitmap::load() now borrow self and the BitmapInner exclusively because these methods mutate the underlying bitmap data.

New features

• BitmapData::pixels() and BitmapData::mask() provides access to the underlying pixel and mask data as byte slices.
• BitmapData has getter methods for its fields.
• Adds Bitmap::get_data_mut() to gain mutable access to the pixel and mask data through BitmapDataMut.
• BitmapData::to_view() and BitmapDataMut::to_view() are available to create a view of the BitmapData or BitmapDataMut that is not tied to the lifetime of the Bitmap that owns the data. BitmapDataView relinquishes access to the pixel and mask data so that it can be easily persisted and outlive the Bitmap.

Oddities

• BitmapInner has a number of public methods, but this type is never made accessible publically. These methods should either be made private or pub(crate). Also, BitmapInner supports more methods than the public Bitmap type.
• Bitmap::get_data_mut() technically does not need an exclusive reference to Self, since we can rely on borrow_mut() panicking at runtime when attempting to acquire two BitmapDataMuts from the same Bitmap. But it is a better user experience to enforce the invariant at compile time.
• BitmapDataMut cannot exist more than once for the same Bitmap, as mentioned above. This would cause UB by allowing mutable aliasing by e.g. holding two references from a.pixels_mut() and b.pixels_mut(). These references will point to the same location in memory.

[parasyte]

I think this implementation is correct. The somewhat limited testing I have done hasn't revealed any showstoppers. And I believe my reasoning for the lifetimes and shared/exclusive splits in the API are sound.

As a side note, an alternative to making BitmapData fields private is storing the buffer length in a private field and allowing public access to all of the old fields (including a hasmask: bool for backward compatibility). The downside is that it will require slightly more memory and it moves the potential overflow panic to the Bitmap::get_data() and Bitmap::get_data_mut() calls.

Happy to discuss if anything here needs further improvement. I'll save the visibility weirdness mentioned in the "oddities" section for another conversation.

[parasyte]

I'm going to revert this PR back to draft while I sort out potential unsoundness that I discovered. I'll have a full analysis later.

[parasyte]

Ok, it is guaranteed Undefined Behavior that I found. The issue is that the Bitmap API surface assumes interior mutability, and that makes the new API unsound because it provides &[u8]. It cannot provide a shared slice while other APIs mutate the contents that it refers to.

A trivial example:

// Create a small bitmap
let gfx = Graphics::get();
let bitmap = gfx.new_bitmap(
    ScreenSize::new(16, 1),
    LCDColor::Solid(LCDSolidColor::kColorBlack),
)?;
let pixels = bitmap.get_data()?.pixels();

// We know that `pixels` is all black.
assert_eq!(pixels, &[0, 0, 0, 0]);

// It is unsound to mutate the memory backing the slice while we hold onto it:
bitmap.clear(LCDColor::Solid(LCDSolidColor::kColorWhite))?;

// This assert should panic!
assert_eq!(pixels, &[0xff, 0xff, 0xff, 0xff]);
log_to_console!("Whoops! UB: {pixels:#04x?}");

Running this code, we see that neither assertion panics, and the log prints:

Whoops! UB: [
    0xff,
    0xff,
    0xff,
    0xff,
]

We were able to mutate the contents behind a shared reference, violating the Shared XOR Mutable principle.

---

There are broadly three solutions that I know of for this:

First Option

First, we might consider just marking the get_data API as unsafe. Push the burden of enforcing safety invariants to the caller. We need a lot of good documentation to make this strategy viable, and even then, we aren't doing any favors for the programmer. It would remove all of the assurance of the borrow checker and put undue responsibility on the caller. Tooling is very lacking on the Playdate platform: no Miri, no ASAN, etc. IMHO, this would be a huge detriment to developer productivity.

Second Option

The second is to just accept interior mutability as an unchangeable fact and make pixels() and mask() return &[Cell<u8>] or &Cell<[u8]>. ¹ This solves the unsoundness because data behind Cell<T> is allowed to mutate. The compiler disables some optimizations to make this possible, which is one of the downsides. The other, arguably more problematic, downside is that a plain &[Cell<T>] can be harder to work with. For instance, attempting to use slice::copy_from_slice won't work because Cell: !Copy, and slice::clone_from_slice is ugly because you have to wrap the other slice's bytes in Cell. ²

The optimization tradeoff and the ergonomics tradeoff are too much for the only benefit (existing APIs with interior mutability don't need to be changed) to hold up, IMHO.

Third Option

The last alternative is going back on the original interior mutability design and require the user-facing API to take exclusive references (&mut Bitmap and RefCell::borrow_mut) for any method that mutates the underlying data (even across the FFI boundary). The reason for RefCell::borrow_mut is that the inner bitmap is behind a freely cloneable Rc.

Assuming we use this new clear implementation:

pub fn clear(&mut self, color: LCDColor) -> Result<(), Error> {
    self.inner.borrow().clear(color)
}

We can still trivially obverse UB by cloning the bitmap and clearing the clone:

// It is unsound to mutate the memory backing the slice, but we can clear it to white through a cloned `Rc`:
let mut bitmap2 = bitmap.clone();
bitmap2.clear(LCDColor::Solid(LCDSolidColor::kColorWhite))?;

To fix this, we need borrow_mut() in addition to &mut self. But even this isn't enough, because we need to keep the Ref alive from the get_data() call and hold it across the clear call to catch this kind of unsoundness. Which means that the BitmapData must hold the Ref<'bitmap, BitmapInner> internally.

This does work, but there are a few minor ergonomics issues:

1. The BitmapData needs a temporary binding for calling pixels() on it, or you will get a "temporary value dropped while borrowed" error. This extra binding also needs to be dropped explicitly before taking &mut Bitmap, or you get a Shared XOR Mutable borrow check error.

2. If you clone the Bitmap in an attempt to avoid the borrow check error at compile time and do the clear() anyway from the clone, you will get an error in the clear() call. This is expected, but because it moves borrow checks to runtime, it isn't ideal. (Edit: The previous implementation panicked in the clear() call. Now all Bitmap methods return an error if the inner field cannot be borrowed.)

Overall, I think the tradeoff between slight ergonomics issues and having a fully sound interface for accessing bitmap pixel data with low overhead is a clear winner.

---

Conclusion

My conclusion, and my personal preference, is that making this API available is a good thing, but it cannot be at the expense of soundness or safety. The existence of ref-counting adds an extra layer of concern that requires holding on to the RefCell borrows for runtime borrow checking. This is not a high cost in terms of CPU or memory resources, but it is an extra layer of complexity for designing safe abstractions.

One could argue that a fourth option is removing the reference counting. That is more work than I am willing to put in here, and it would make Bitmap clones more expensive (though, also arguably, more in line with what you might expect from cloning a bitmap; a fully independent copy).

Disregarding those issues, I will move forward with BitmapData holding onto the Ref<'bitmap, BitmapInner>, since I have been able to confirm that it does cover all of the unsoundness that I've discussed above. And the only other surface-level impact is requiring exclusive borrows on Bitmap methods with interior mutability, like clear().

Footnotes

1. See https://users.rust-lang.org/t/how-unsafe-is-mmap/19635 where a similar discussion about byte slices and interior mutability has been explored thoroughly, along with a follow up: https://users.rust-lang.org/t/is-there-no-safe-way-to-use-mmap-in-rust/70338 ↩

2. Also, transmuting &[Cell<u8>] to &[u8] is unsound because those bytes still have interior mutability! And I can't seem to find it now, but I seem to recall someone made exactly this point about type compatibility issues with existing APIs accepting only &[u8] as "byte slices", even though the &[Cell<u8>] and &[AtomicU8] are equally valid. The closest I have found was https://users.rust-lang.org/t/how-unsafe-is-mmap/19635/80#h-2-memory-can-be-changed-out-from-under-you-2 WRT their "custom buffer trait". ↩

[boozook]

Thanks you!
I've solved this problem with const-generics. I have to think more about your thoughts and this solution. 👍

And BitmapDataMuts-like - there.

[parasyte]

That looks solid. You don’t have Rc<RefCell<T>>, which this PR struggled with. And you don’t have a read-only get_bitmap_data, so the interior mutability is not a concern!

[jhbruhn]

Ah, I was in the middle of this rabbit hole myself until I discovered your PR. Nice Work!

Personally, I'd prefer Option 3, as it seems the most idiomatic. Clone should in my opinion be a full clone of the Bitmap anyways, which would then allocate a new pixel buffer, if that is possible using the playdate APIs. Otherwise, it isn't a Clone, but a (weird) reference!

[boozook]

Personally, I'd prefer Option 3, as it seems the most idiomatic. Clone should in my opinion be a full clone of the Bitmap anyways, which would then allocate a new pixel buffer, if that is possible using the playdate APIs. Otherwise, it isn't a Clone, but a (weird) reference!

I'm agree and... Ha! Here is example!

[boozook]

Another problem with my implementation that I mentioned above is that I don't allow clone for "don't free on drop" bitmaps,
I just don realise yet what to do with it. Probably nothing because we're don't need shared owned single bitmap in multiple wrappers, just use rust's normal refs.

That problem is actual for subject implementation.