android: Make the backend zero-copy

[MarijnS95]

WIP but tested change that closes #318.

Still need to discuss how to handle MaybeUninit, and the now side-effect presenting in-progress modifications to pixels_mut() if the caller ended up dropping their Buffer half-way through, which is an unfortunate caveat of having a locked buffer for the surface that unlocks and presents on drop. The turning point is that it's not allowed to lock() a surface twice before unlock()(ing) and inherently presenting it.

Are other platforms affected by such a lock-unlock kind of API? As hinted in #318 ASurfaceControl+ASurfaceTransaction+AHardwareBuffer completely obviate this issue, but that has very high Android requirements (the API's are there for a while, but I seem to have been the first one ever using it on the root Surface, the one you get from NativeActivity, and it didn't work until a bug report and fix since Android 15 (API 35)).

[madsmtm]

Are other platforms affected by such a lock-unlock kind of API?

On macOS, the locking/unlocking operation that IOSurface does is expensive, because it tells the MMU to make the region of memory available to other parts of the system. But you can do that over and over again, because nobody should be using that IOSurface yet.

So I don't think there are other platforms where this is a problem, Android is a bit special in "managing" the buffer(s) for us like this.

---

Maybe an option would be to store the locked buffer on the surface? Something like:

struct AndroidImpl {
    native_window: NativeWindow,
    in_progress_buffer: Option<NativeWindowBufferLockGuard<'static>>, // + unsafe magic
}

fn buffer_mut() {
    if let Some(native_window_buffer) = self.in_progress_buffer {
        return BufferImpl { native_window_buffer };
    }
    
    let native_window_buffer = self.native_window.lock(None)?;
    BufferImpl { native_window_buffer, surface_in_progress_buffer: &mut self.in_progress_buffer }
}

impl Drop for BufferImpl {
    fn drop(&mut self) {
        let buffer = self.native_window_buffer.take();
        *self.surface_in_progress_buffer = Some(buffer);
    }
}

That would allow the following to work the same as on other platforms:

let mut buffer = surface.buffer_mut();
buffer.pixels().fill(Pixel::rgb(0, 0, 0)); // Clear
drop(buffer);
let mut buffer = surface.buffer_mut();
draw(&mut buffer);
buffer.present();

[MarijnS95]

@madsmtm something like that would work. I dropped the Some reassignment in fn drop() since the mutable lifetime that BufferImpl has on AndroidImpl already guarantees unique ownership, so it can already access the Option directly.

Unfortunately that requires various unwraps (like your suggested self.native_window_buffer.take()), but at least should not be susceptible to std::mem::forget() scenarios (which are safe) where unlock is not called and AndroidImpl::in_progess_buffer remains None?

[madsmtm]

Unfortunately that requires various unwraps

I'm totally fine with a bit of unwrapping, especially after #313, where we don't need to make pixels_mut zero-cost.

should not be susceptible to std::mem::forget() scenarios (which are safe) where unlock is not called and AndroidImpl::in_progess_buffer remains None?

I don't think my solution would've caused unsoundness either if you forget the buffer? It'd just cause a panic because you'd try to lock the buffer twice.

Anyhow, forgetting the buffer is definitely unsupported, as long as things are sound I don't care what the behavior of it is - I'll leave that up to whatever is easiest for you to maintain.

[MarijnS95]

Unfortunately that requires various unwraps

I'm totally fine with a bit of unwrapping

I mostly wanted to perhaps have a wrapper function around this accessor, to have one clear place to document that the unwrap() is actually expected to never be reachable, because the Option is only taken on present right where the struct is also dropped (while being a borrow on AndroidImpl of course).

And again, if it wasn't for present() being directly on Buffer, or if BufferImpl took access to the entire AndroidImpl (which would still require an unwrap()), it could have possibly been a borrow of the Some() portion of the Option (though that likely has implications on multiple mutable borrows). Or with your proposed solution to own it and move it back to AndroidImpl in Drop (though see below).

should not be susceptible to std::mem::forget() scenarios (which are safe) where unlock is not called and AndroidImpl::in_progess_buffer remains None?

I don't think my solution would've caused unsoundness either if you forget the buffer? It'd just cause a panic because you'd try to lock the buffer twice.

I didn't particularly say that your suggestion is unsound, I explicitly named them "scenarios (which are safe)" but they will always error in the way you described.

Anyhow, forgetting the buffer is definitely unsupported, as long as things are sound I don't care what the behavior of it is - I'll leave that up to whatever is easiest for you to maintain.

At least with my variant - where BufferImpl does not own the NativeWindowBufferLockGuard with the disadvantage of constantly having to unwrap() the borrowed Option from the parent AndroidImpl - forgetting without Drop and re-accessing the buffer via buffer_mut() has no such issues.

[madsmtm]

Blocking on the possible soundness issue, otherwise I'm happy with this.

[madsmtm]

Could you document in a comment why we keep this in-progress buffer here?

[madsmtm]

Hmm, I think the way we actually ensure soundness is by not modifying AndroidImpl.native_window, so the reference that NativeWindowBufferLockGuard holds remains valid.

But reading that, I think this might actually be unsound? NativeWindowBufferLockGuard is defined as:

struct NativeWindowBufferLockGuard<'a> {
    window: &'a NativeWindow,
    buffer: ffi::ANativeWindow_Buffer,
}

Which means that we're pointing to the NativeWindow stored on AndroidImpl, but if the user moves Surface, then that &NativeWindow reference would no longer be valid.

(Well, I guess it can't be observed because Surface.surface_impl is currently Boxed, but that feels brittle).

[madsmtm]

Is there any chance that NativeWindowBufferLockGuard could be updated to something like this instead?

struct NativeWindowBufferLockGuard<'a> {
    window: NonNull<ffi::ANativeWindow>,
    buffer: ffi::ANativeWindow_Buffer,
    window_marker: PhantomData<&'a ()>,
}

That would remove a level of indirection, and make what we do here in this PR sound.

Alternatively, add a version of NativeWindowBufferLockGuard that contains the NativeWindow and reference-counts it, that would allow doing all this without unsafe.