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

Draft
MarijnS95 wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

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
81eff2e
android: Make the backend zero-copy
MarijnS95 Feb 1, 2026
acc2bcf
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
73 changes: 48 additions & 25 deletions src/backends/android.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,21 @@ use raw_window_handle::AndroidNdkWindowHandle;
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};

/// 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>>,
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 +46,7 @@ impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for Android
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 +58,7 @@ impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for Android
&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,6 +83,12 @@ impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for Android
}

fn buffer_mut(&mut self) -> Result<BufferImpl<'_>, SoftBufferError> {
if self.in_progress_buffer.is_some() {
return Ok(BufferImpl {
native_window_buffer: &mut self.in_progress_buffer,
});
}

let native_window_buffer = self.native_window.lock(None).map_err(|err| {
SoftBufferError::PlatformError(
Some("Failed to lock ANativeWindow".to_owned()),
Expand All @@ -99,12 +111,19 @@ impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for Android
));
}

let buffer =
vec![Pixel::default(); native_window_buffer.stride() * native_window_buffer.height()];
// 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.
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 +135,42 @@ impl<D: HasDisplayHandle, W: HasWindowHandle> SurfaceInterface<D, W> for Android

#[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() as u32 * 4).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();
// assert_eq!(
// native_buffer.len(),
// self.native_window_buffer.stride() * self.native_window_buffer.height()
// );
// TODO: Validate that Android actually initializes (possibly with garbage) the buffer, or
// let softbuffer initialize it, or return MaybeUninit<Pixel>?
// i.e. consider age().
Comment on lines +165 to +167
Copy link
Member

@ids1024 ids1024 Feb 2, 2026

Choose a reason for hiding this comment

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

As long as android is using a swap chain of two or three buffers and we can get the buffer age, it probably makes the most sense for softbuffer to zero-initialize the buffers, if Android doesn't already do that.

It shouldn't really add any meaningful cost to zero each buffer once.

Copy link
Member

@madsmtm madsmtm Feb 3, 2026

Choose a reason for hiding this comment

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

Maybe we can somehow infer the amount of buffers with ANativeWindow_getBuffersDataSpace?

But I'd also be fine with zero-initializing in buffer_mut for now, and coming back to this later, this PR will be an improvement anyhow.

Copy link
Member

@ids1024 ids1024 Feb 5, 2026

Choose a reason for hiding this comment

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

Looking at https://developer.android.com/ndk/reference/group/a-native-window#anativewindow_lock, I guess it would return the whole bounds of the buffer in inOutDirtyBounds if a buffer is freshly allocated?

Though the API seems a bit backwards from how we have softbuffer work. We don't have a damage region to pass until we present.

Copy link
Member

@madsmtm madsmtm Feb 5, 2026

Choose a reason for hiding this comment

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

Though the API seems a bit backwards from how we have softbuffer work. We don't have a damage region to pass until we present.

Yeah, that's discussed in a code comment too:

softbuffer/src/backends/android.rs

Lines 151 to 158 in afa87bb

// 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).
//
// Android seems to do this because the region can be expanded by the
// system, requesting the user to actually redraw a larger region.
// It's unclear if/when this is used, or if corruption/artifacts occur
// when the enlarged damage region is not re-rendered?

Copy link
Member

@ids1024 ids1024 Feb 5, 2026

Choose a reason for hiding this comment

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

It seems the implementation of this is in Surface::lock defined in https://android.googlesource.com/platform/frameworks/native/+/main/libs/gui/Surface.cpp.

Which also has:

const bool canCopyBack = (frontBuffer != nullptr &&
        backBuffer->width  == frontBuffer->width &&
        backBuffer->height == frontBuffer->height &&
        backBuffer->format == frontBuffer->format);

And then copies the front buffer to the back buffer if it can.

Or otherwise:

// if we can't copy-back anything, modify the user's dirty
// region to make sure they redraw the whole buffer

Maybe a bit of an abuse of the API, but if we pass an empty inOutDirtyBounds region, maybe we could then see if we get an empty region in return. In which case we can treat it as age 1. Otherwise treat it as a new buffer with age 0, which we can also zero-initialize to if we think that's necessary.

Sounds like that would work? And should generally return an age of 1, keeping things very simple for any damage-tracked renderer using softbuffer, since Android is apparently copying the front buffer to the back buffer already.

Copy link
Member Author

@MarijnS95 MarijnS95 Feb 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we can somehow infer the amount of buffers with ANativeWindow_getBuffersDataSpace?

That function queries the color space and related info: https://developer.android.com/ndk/reference/group/a-data-space

But I'd also be fine with zero-initializing in buffer_mut for now, and coming back to this later, this PR will be an improvement anyhow.

Okay, we could do that...

It seems that I get the same pixel buffer back on every 3rd lock, so a typical triple-buffered swapchain.

But always tricky to infer if you don't know about internal copies that may change the context relative to what you thought it might have been given a non-zero age.

That all goes away with custom AHardwareBuffer presented through ASurfaceControl/Transaction.

EDIT: And if I remember correctly, I once asked the Android developers to create an AImageWriter synonymous to the existing AImageReader to serve this exact purpose of (de)queueing buffers from a Surface in a more controllable manner (since the underlying APIs exist in the private ANativeWindow structure).

I'm a bit skeptical with those sources since they may change across Android versions. Despite that the documentation is never comprehensive/complete and may "change" to "clarify" scenarios though.

Maybe a bit of an abuse of the API, but if we pass an empty inOutDirtyBounds region, maybe we could then see if we get an empty region in return.

In that case, isn't it expected or allowed that nothing of the buffer is presented, because nothing was communicated or expected to have changed?

Copy link
Member

@madsmtm madsmtm Feb 7, 2026

Choose a reason for hiding this comment

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

I'm a bit skeptical with those sources since they may change across Android versions. Despite that the documentation is never comprehensive/complete and may "change" to "clarify" scenarios though.

Yeah, me too. So I wouldn't expose it in age, at least not unless we can find some definitive docs for things (including the tripple-buffered statement you made above).

Copy link
Member Author

@MarijnS95 MarijnS95 Feb 7, 2026

Choose a reason for hiding this comment

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

Also unclear what would happen on a resize (which also changes the format). If the "view" size changes but resize is not called, does it reuse the same buffers with implied upscaling?

(On wgpu/glutin I saw that this format change implicitly limited EGL to only return configs for that format, so it was definitely confusing)

Copy link
Member

@ids1024 ids1024 Feb 9, 2026

Choose a reason for hiding this comment

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

In that case, isn't it expected or allowed that nothing of the buffer is presented, because nothing was communicated or expected to have changed?

I was thinking the input dirty bounds is only used to calculate the output dirty bounds, but looking again the dirty region is also passed to backBuffer->lockAsync. So I guess we do need to just pass NULL (equivalent to the whole region).

Copy link
Member Author

@MarijnS95 MarijnS95 Feb 9, 2026

Choose a reason for hiding this comment

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

Okay so the only remaining thing here is to zero-initialize it (always) to get rid of the MaybeUninit "safely", and leave the age() at 0.

unsafe {
std::slice::from_raw_parts_mut(
native_buffer.as_mut_ptr().cast(),
native_buffer.len() / 4,
)
}
}

#[inline]
Expand All @@ -147,7 +179,7 @@ impl BufferInterface for BufferImpl<'_> {
}

// 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 +190,9 @@ impl BufferInterface for BufferImpl<'_> {
// 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