Skip to content

android: Make the backend zero-copy #331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MarijnS95 wants to merge 2 commits into
base: master
Choose a base branch
from
+62 −26
Open

android: Make the backend zero-copy #331

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7911980
android: Make the backend zero-copy
MarijnS95 Feb 1, 2026
b9f5218
android: Hold the locked buffer in `AndroidImpl` until it is presente…
MarijnS95 Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 62 additions & 26 deletions src/backends/android.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Implementation of software buffering for Android.

use std::marker::PhantomData;
use std::mem::MaybeUninit;
use std::num::{NonZeroI32, NonZeroU32};

use ndk::{
Expand All @@ -12,16 +13,23 @@
use raw_window_handle::{HasDisplayHandle, HasWindowHandle, RawWindowHandle};

use crate::error::InitError;
use crate::{util, BufferInterface, Pixel, Rect, SoftBufferError, SurfaceInterface};
use crate::{BufferInterface, Pixel, Rect, SoftBufferError, SurfaceInterface};

const PIXEL_SIZE: usize = size_of::<Pixel>();

Check failure on line 18 in src/backends/android.rs

View workflow job for this annotation

GitHub Actions / MSRV (aarch64-linux-android) 

cannot find function `size_of` in this scope

/// The handle to a window for software buffering.
#[derive(Debug)]
pub struct AndroidImpl<D, W> {
// Must be first in the struct to guarantee being dropped and unlocked before the `NativeWindow` reference
in_progress_buffer: Option<NativeWindowBufferLockGuard<'static>>,
Copy link
Member

@madsmtm madsmtm Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

native_window: NativeWindow,
window: W,
_display: PhantomData<D>,
}

// TODO: Move to NativeWindowBufferLockGuard?
unsafe impl<D, W> Send for AndroidImpl<D, W> {}

impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for AndroidImpl<D, W> {
type Context = D;
type Buffer<'surface>
Expand All @@ -41,6 +49,7 @@
let native_window = unsafe { NativeWindow::clone_from_ptr(a.a_native_window.cast()) };

Ok(Self {
in_progress_buffer: None,
native_window,
_display: PhantomData,
window,
Expand All @@ -52,7 +61,7 @@
&self.window
}

/// Also changes the pixel format to [`HardwareBufferFormat::R8G8B8A8_UNORM`].
/// Also changes the pixel format to [`HardwareBufferFormat::R8G8B8X8_UNORM`].
fn resize(&mut self, width: NonZeroU32, height: NonZeroU32) -> Result<(), SoftBufferError> {
let (width, height) = (|| {
let width = NonZeroI32::try_from(width).ok()?;
Expand All @@ -77,7 +86,13 @@
}

fn next_buffer(&mut self) -> Result<BufferImpl<'_>, SoftBufferError> {
let native_window_buffer = self.native_window.lock(None).map_err(|err| {
if self.in_progress_buffer.is_some() {
return Ok(BufferImpl {
native_window_buffer: &mut self.in_progress_buffer,
});
}

let mut native_window_buffer = self.native_window.lock(None).map_err(|err| {
SoftBufferError::PlatformError(
Some("Failed to lock ANativeWindow".to_owned()),
Some(Box::new(err)),
Expand All @@ -99,12 +114,33 @@
));
}

let buffer =
vec![Pixel::default(); native_window_buffer.stride() * native_window_buffer.height()];
assert_eq!(
native_window_buffer.format().bytes_per_pixel(),
Some(PIXEL_SIZE)
);
let native_buffer = native_window_buffer.bytes().unwrap();

// Zero-initialize the buffer, allowing it to be cast away from MaybeUninit and mapped as a Pixel slice
native_buffer.fill(MaybeUninit::new(0));

assert_eq!(
native_buffer.len(),
native_window_buffer.stride() * native_window_buffer.height() * PIXEL_SIZE
);

// SAFETY: We guarantee that the guard isn't actually held longer than this owned handle of
// the `NativeWindow` (which is trivially cloneable), by means of having BufferImpl take a
// mutable borrow on AndroidImpl which owns the NativeWindow and LockGuard.
Comment on lines +131 to +133
Copy link
Member

@madsmtm madsmtm Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@madsmtm madsmtm Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let native_window_buffer = unsafe {
std::mem::transmute::<
NativeWindowBufferLockGuard<'_>,
NativeWindowBufferLockGuard<'static>,
>(native_window_buffer)
};
self.in_progress_buffer = Some(native_window_buffer);

Ok(BufferImpl {
native_window_buffer,
buffer: util::PixelBuffer(buffer),
native_window_buffer: &mut self.in_progress_buffer,
})
}

Expand All @@ -116,29 +152,38 @@

#[derive(Debug)]
pub struct BufferImpl<'surface> {
native_window_buffer: NativeWindowBufferLockGuard<'surface>,
buffer: util::PixelBuffer,
// This Option will always be Some until present_with_damage() is called
native_window_buffer: &'surface mut Option<NativeWindowBufferLockGuard<'static>>,
}

// TODO: Move to NativeWindowBufferLockGuard?
unsafe impl Send for BufferImpl<'_> {}

impl BufferInterface for BufferImpl<'_> {
fn byte_stride(&self) -> NonZeroU32 {
NonZeroU32::new(self.native_window_buffer.stride() as u32 * 4).unwrap()
NonZeroU32::new((self.native_window_buffer.as_ref().unwrap().stride() * PIXEL_SIZE) as u32)
.unwrap()
}

fn width(&self) -> NonZeroU32 {
NonZeroU32::new(self.native_window_buffer.width() as u32).unwrap()
NonZeroU32::new(self.native_window_buffer.as_ref().unwrap().width() as u32).unwrap()
}

fn height(&self) -> NonZeroU32 {
NonZeroU32::new(self.native_window_buffer.height() as u32).unwrap()
NonZeroU32::new(self.native_window_buffer.as_ref().unwrap().height() as u32).unwrap()
}

#[inline]
fn pixels_mut(&mut self) -> &mut [Pixel] {
&mut self.buffer
let native_buffer = self.native_window_buffer.as_mut().unwrap().bytes().unwrap();
// SAFETY: The buffer was zero-initialized and its length is always a multiple of the pixel
// size (4 bytes), even when stride is applied
unsafe {
std::slice::from_raw_parts_mut(
native_buffer.as_mut_ptr().cast(),
native_buffer.len() / PIXEL_SIZE,
)
}
}

#[inline]
Expand All @@ -147,7 +192,7 @@
}

// TODO: This function is pretty slow this way
fn present_with_damage(mut self, damage: &[Rect]) -> Result<(), SoftBufferError> {
fn present_with_damage(self, damage: &[Rect]) -> Result<(), SoftBufferError> {
// TODO: Android requires the damage rect _at lock time_
// Since we're faking the backing buffer _anyway_, we could even fake the surface lock
// and lock it here (if it doesn't influence timings).
Expand All @@ -158,18 +203,9 @@
// when the enlarged damage region is not re-rendered?
let _ = damage;

// Unreachable as we checked before that this is a valid, mappable format
let native_buffer = self.native_window_buffer.bytes().unwrap();

// Write RGB(A) to the output.
// TODO: Use `slice::write_copy_of_slice` once stable and in MSRV.
// TODO(madsmtm): Verify that this compiles down to an efficient copy.
for (pixel, output) in self.buffer.iter().zip(native_buffer.chunks_mut(4)) {
output[0].write(pixel.r);
output[1].write(pixel.g);
output[2].write(pixel.b);
output[3].write(pixel.a);
}
// The surface will be presented when it is unlocked, which happens when the owned guard
// is dropped.
self.native_window_buffer.take();

Ok(())
}
Expand Down
Loading