From bc0575287e5eb59531e355a1311108ade1a5d5b9 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 5 Mar 2026 13:21:41 +0100 Subject: [PATCH 1/2] Use triple-buffering in CoreGraphics --- src/backends/cg.rs | 197 +++++++++++++++++++++++++++++++++------------ 1 file changed, 147 insertions(+), 50 deletions(-) diff --git a/src/backends/cg.rs b/src/backends/cg.rs index a84cbd7f..77280da3 100644 --- a/src/backends/cg.rs +++ b/src/backends/cg.rs @@ -20,10 +20,11 @@ use raw_window_handle::{HasDisplayHandle, HasWindowHandle, RawWindowHandle}; use std::ffi::c_void; use std::marker::PhantomData; -use std::mem::size_of; +use std::mem::{self, size_of}; use std::num::NonZeroU32; use std::ops::Deref; use std::ptr::{self, slice_from_raw_parts_mut, NonNull}; +use std::slice; define_class!( #[unsafe(super(NSObject))] @@ -104,10 +105,9 @@ pub struct CGImpl { root_layer: SendCALayer, observer: Retained, color_space: CFRetained, - /// The width of the underlying buffer. - width: usize, - /// The height of the underlying buffer. - height: usize, + front: Buffer, + middle: Option, + back: Buffer, window_handle: W, _display: PhantomData, } @@ -228,23 +228,25 @@ impl SurfaceInterface for CGImpl< // Default alpha mode is opaque. layer.setOpaque(true); - // Initialize color space here, to reduce work later on. + // The color space we're using. Initialize it here to reduce work later on. + // TODO: Allow setting this to something else? let color_space = CGColorSpace::new_device_rgb().unwrap(); // Grab initial width and height from the layer (whose properties have just been initialized // by the observer using `NSKeyValueObservingOptionInitial`). let size = layer.bounds().size; let scale_factor = layer.contentsScale(); - let width = (size.width * scale_factor) as usize; - let height = (size.height * scale_factor) as usize; + let width = (size.width * scale_factor) as u32; + let height = (size.height * scale_factor) as u32; Ok(Self { layer: SendCALayer(layer), root_layer: SendCALayer(root_layer), observer, color_space, - width, - height, + front: Buffer::new(width, height), + middle: Some(Buffer::new(width, height)), + back: Buffer::new(width, height), _display: PhantomData, window_handle: window_src, }) @@ -271,17 +273,38 @@ impl SurfaceInterface for CGImpl< // TODO: Set opaque-ness on root layer too? Is that our responsibility, or Winit's? // self.root_layer.setOpaque(opaque); - self.width = width.get() as usize; - self.height = height.get() as usize; + let width = width.get(); + let height = height.get(); + + // TODO: Is this check desirable? + if self.front.width == width && self.front.height == height { + return Ok(()); + } + + // Recreate buffers. It's fine to release the old ones, `CALayer.contents` is going to keep + // a reference if they're still in use. + self.front = Buffer::new(width, height); + self.back = Buffer::new(width, height); + Ok(()) } fn next_buffer(&mut self, alpha_mode: AlphaMode) -> Result, SoftBufferError> { - let buffer_size = util::byte_stride(self.width as u32) as usize * self.height / 4; + // Block until back buffer is no longer being used by the compositor. + // + // TODO: Allow configuring this? https://github.com/rust-windowing/softbuffer/issues/29 + // TODO: Is this actually the check we want to do? It feels overly restrictive. + // TODO: Should we instead keep a boundless queue, and use the latest available buffer? + tracing::warn!("next_buffer"); + while self.back.is_in_use() { + tracing::warn!("in use"); + std::thread::yield_now(); + } + Ok(BufferImpl { - buffer: util::PixelBuffer(vec![Pixel::default(); buffer_size]), - width: self.width, - height: self.height, + front: &mut self.front, + middle: &mut self.middle, + back: &mut self.back, color_space: &self.color_space, alpha_info: match (alpha_mode, cfg!(target_endian = "little")) { (AlphaMode::Opaque | AlphaMode::Ignored, true) => CGImageAlphaInfo::NoneSkipFirst, @@ -296,62 +319,65 @@ impl SurfaceInterface for CGImpl< } } +/// The implementation used for presenting the back buffer to the surface. +/// +/// This is triple-buffered because that's what QuartzCore / the compositor seems to require: +/// - The front buffer is what's currently assigned to `CALayer.contents`, and was submitted to the +/// compositor in the previous iteration of the run loop. +/// - The middle buffer is what the compositor is currently drawing from (assuming a 1 frame delay). +/// - The back buffer is what we'll be drawing into. +/// +/// This is especially important because `softbuffer::Surface` may be used from different threads. #[derive(Debug)] pub struct BufferImpl<'surface> { - width: usize, - height: usize, + front: &'surface mut Buffer, + middle: &'surface mut Option, + back: &'surface mut Buffer, color_space: &'surface CGColorSpace, - buffer: util::PixelBuffer, alpha_info: CGImageAlphaInfo, layer: &'surface mut SendCALayer, } impl BufferInterface for BufferImpl<'_> { fn byte_stride(&self) -> NonZeroU32 { - NonZeroU32::new(util::byte_stride(self.width as u32)).unwrap() + NonZeroU32::new(util::byte_stride(self.back.width)).unwrap() } fn width(&self) -> NonZeroU32 { - NonZeroU32::new(self.width as u32).unwrap() + NonZeroU32::new(self.back.width).unwrap() } fn height(&self) -> NonZeroU32 { - NonZeroU32::new(self.height as u32).unwrap() + NonZeroU32::new(self.back.height).unwrap() } - #[inline] fn pixels_mut(&mut self) -> &mut [Pixel] { - &mut self.buffer + // SAFETY: Called on the back buffer. + unsafe { self.back.data() } } fn age(&self) -> u8 { - 0 + self.back.age } fn present_with_damage(self, _damage: &[Rect]) -> Result<(), SoftBufferError> { - unsafe extern "C-unwind" fn release( - _info: *mut c_void, - data: NonNull, - size: usize, - ) { - let data = data.cast::(); - let slice = slice_from_raw_parts_mut(data.as_ptr(), size / size_of::()); - // SAFETY: This is the same slice that we passed to `Box::into_raw` below. - drop(unsafe { Box::from_raw(slice) }) + self.back.age = 1; + if let Some(middle) = self.middle { + if middle.age != 0 { + middle.age += 1; + } + } + if self.front.age != 0 { + self.front.age += 1; } - let data_provider = { - let len = self.buffer.len() * size_of::(); - let buffer: *mut [Pixel] = Box::into_raw(self.buffer.0.into_boxed_slice()); - // Convert slice pointer to thin pointer. - let data_ptr = buffer.cast::(); - - // SAFETY: The data pointer and length are valid. - // The info pointer can safely be NULL, we don't use it in the `release` callback. - unsafe { - CGDataProvider::with_data(ptr::null_mut(), data_ptr, len, Some(release)).unwrap() - } - }; + // Rotate buffers such that the back buffer is now the front buffer. + if let Some(middle) = self.middle { + mem::swap(self.back, middle); + mem::swap(middle, self.front); + } else { + mem::swap(self.back, self.front); + } // `CGBitmapInfo` consists of a combination of `CGImageAlphaInfo`, `CGImageComponentInfo` // `CGImageByteOrderInfo` and `CGImagePixelFormatInfo` (see e.g. `CGBitmapInfoMake`). @@ -364,16 +390,17 @@ impl BufferInterface for BufferImpl<'_> { | CGImagePixelFormatInfo::Packed.0, ); + // CGImage is immutable, so we re-create it let image = unsafe { CGImage::new( - self.width, - self.height, + self.front.width as usize, + self.front.height as usize, 8, 32, - util::byte_stride(self.width as u32) as usize, + util::byte_stride(self.front.width) as usize, Some(self.color_space), bitmap_info, - Some(&data_provider), + Some(&self.front.data_provider), ptr::null(), false, CGColorRenderingIntent::RenderingIntentDefault, @@ -395,6 +422,76 @@ impl BufferInterface for BufferImpl<'_> { } } +/// A single buffer in Softbuffer. +#[derive(Debug)] +struct Buffer { + data_provider: CFRetained, + data_ptr: *mut Pixel, + width: u32, + height: u32, + age: u8, +} + +// SAFETY: We only mutate the `CGDataProvider` when we know it's the back buffer, and only then +// behind `&mut`. +unsafe impl Send for Buffer {} +// SAFETY: Same as above. +unsafe impl Sync for Buffer {} + +impl Buffer { + fn new(width: u32, height: u32) -> Self { + unsafe extern "C-unwind" fn release( + _info: *mut c_void, + data: NonNull, + size: usize, + ) { + let data = data.cast::(); + let slice = slice_from_raw_parts_mut(data.as_ptr(), size / size_of::()); + // SAFETY: This is the same slice that we passed to `Box::into_raw` below. + drop(unsafe { Box::from_raw(slice) }) + } + + let num_bytes = util::byte_stride(width) as usize * (height as usize); + + let buffer = vec![Pixel::default(); num_bytes / size_of::()].into_boxed_slice(); + let data_ptr = Box::into_raw(buffer).cast::(); + + // SAFETY: The data pointer and length are valid. + // The info pointer can safely be NULL, we don't use it in the `release` callback. + let data_provider = unsafe { + CGDataProvider::with_data(ptr::null_mut(), data_ptr, num_bytes, Some(release)) + } + .unwrap(); + + Self { + data_provider, + data_ptr: data_ptr.cast(), + width, + height, + age: 0, + } + } + + /// Hint for whether the data provider is currently being used. + /// + /// Might return `false`, but if this returns `true`, the provider is definitely not in use. + fn is_in_use(&self) -> bool { + self.data_provider.retain_count() != 1 + } + + /// # Safety + /// + /// Must only be called on the back buffer. + unsafe fn data(&mut self) -> &mut [Pixel] { + // Check that nobody else is using the data provider. + debug_assert!(!self.is_in_use()); + + let num_bytes = util::byte_stride(self.width) as usize * (self.height as usize); + // SAFETY: The pointer is valid, and ownership rules are upheld by caller. + unsafe { slice::from_raw_parts_mut(self.data_ptr, num_bytes / size_of::()) } + } +} + #[derive(Debug)] struct SendCALayer(Retained); From e1edabd59894431191b5fbddedefe3a695ed6167 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 5 Mar 2026 13:02:55 +0100 Subject: [PATCH 2/2] Use unbounded buffering in CoreGraphics --- src/backends/cg.rs | 159 +++++++++++++++++++++++---------------------- 1 file changed, 80 insertions(+), 79 deletions(-) diff --git a/src/backends/cg.rs b/src/backends/cg.rs index 77280da3..88169630 100644 --- a/src/backends/cg.rs +++ b/src/backends/cg.rs @@ -20,7 +20,7 @@ use raw_window_handle::{HasDisplayHandle, HasWindowHandle, RawWindowHandle}; use std::ffi::c_void; use std::marker::PhantomData; -use std::mem::{self, size_of}; +use std::mem::size_of; use std::num::NonZeroU32; use std::ops::Deref; use std::ptr::{self, slice_from_raw_parts_mut, NonNull}; @@ -105,9 +105,26 @@ pub struct CGImpl { root_layer: SendCALayer, observer: Retained, color_space: CFRetained, - front: Buffer, - middle: Option, - back: Buffer, + /// The buffers that we may render into. + /// + /// This contains an unbounded number of buffers, since we don't get any feedback from + /// QuartzCore about when it's done using the buffer, other than the retain count of the data + /// provider (which is a weak signal). It shouldn't be needed (QuartzCore seems to copy the data + /// from `CGImage` once), though theoretically there might be cases where it would have a + /// multi-stage pipeline where it processes the image once, retains it, and sends it onwards to + /// be processed again later (and such things might change depending on OS version), so we do + /// this to be safe. + /// + /// Anecdotally, if the user renders 3 times within a single frame (which they probably + /// shouldn't do, but would be safe), we need 4 buffers according to the retain counts. + /// + /// Note that having more buffers here shouldn't add any presentation delay, since we still go + /// directly from drawing to the back buffer and presenting the front buffer. + buffers: Vec, + /// The width of the current buffers. + width: u32, + /// The height of the current buffers. + height: u32, window_handle: W, _display: PhantomData, } @@ -244,9 +261,11 @@ impl SurfaceInterface for CGImpl< root_layer: SendCALayer(root_layer), observer, color_space, - front: Buffer::new(width, height), - middle: Some(Buffer::new(width, height)), - back: Buffer::new(width, height), + // We'll usually do double-buffering, but might end up needing more buffers if the user + // renders multiple times per frame. + buffers: vec![Buffer::new(width, height), Buffer::new(width, height)], + width, + height, _display: PhantomData, window_handle: window_src, }) @@ -277,34 +296,34 @@ impl SurfaceInterface for CGImpl< let height = height.get(); // TODO: Is this check desirable? - if self.front.width == width && self.front.height == height { + if self.width == width && self.height == height { return Ok(()); } // Recreate buffers. It's fine to release the old ones, `CALayer.contents` is going to keep // a reference if they're still in use. - self.front = Buffer::new(width, height); - self.back = Buffer::new(width, height); + self.buffers = vec![Buffer::new(width, height), Buffer::new(width, height)]; + self.width = width; + self.height = height; Ok(()) } fn next_buffer(&mut self, alpha_mode: AlphaMode) -> Result, SoftBufferError> { - // Block until back buffer is no longer being used by the compositor. + // If the backmost buffer might be in use, allocate a new buffer. // - // TODO: Allow configuring this? https://github.com/rust-windowing/softbuffer/issues/29 - // TODO: Is this actually the check we want to do? It feels overly restrictive. - // TODO: Should we instead keep a boundless queue, and use the latest available buffer? - tracing::warn!("next_buffer"); - while self.back.is_in_use() { - tracing::warn!("in use"); - std::thread::yield_now(); + // TODO: Add an `unsafe` option to disable this, and always assume 2 buffers? + if self.buffers.last().unwrap().might_be_in_use() { + self.buffers.push(Buffer::new(self.width, self.height)); + // This should have no effect on latency, but it will affect the `buffer.age()` that + // users see, and unbounded allocation is undesirable too, so we should try to avoid it. + tracing::warn!("had to allocate extra buffer in `next_buffer`, you might be rendering faster than the display rate?"); } Ok(BufferImpl { - front: &mut self.front, - middle: &mut self.middle, - back: &mut self.back, + buffers: &mut self.buffers, + width: self.width, + height: self.height, color_space: &self.color_space, alpha_info: match (alpha_mode, cfg!(target_endian = "little")) { (AlphaMode::Opaque | AlphaMode::Ignored, true) => CGImageAlphaInfo::NoneSkipFirst, @@ -320,19 +339,11 @@ impl SurfaceInterface for CGImpl< } /// The implementation used for presenting the back buffer to the surface. -/// -/// This is triple-buffered because that's what QuartzCore / the compositor seems to require: -/// - The front buffer is what's currently assigned to `CALayer.contents`, and was submitted to the -/// compositor in the previous iteration of the run loop. -/// - The middle buffer is what the compositor is currently drawing from (assuming a 1 frame delay). -/// - The back buffer is what we'll be drawing into. -/// -/// This is especially important because `softbuffer::Surface` may be used from different threads. #[derive(Debug)] pub struct BufferImpl<'surface> { - front: &'surface mut Buffer, - middle: &'surface mut Option, - back: &'surface mut Buffer, + buffers: &'surface mut Vec, + width: u32, + height: u32, color_space: &'surface CGColorSpace, alpha_info: CGImageAlphaInfo, layer: &'surface mut SendCALayer, @@ -340,43 +351,47 @@ pub struct BufferImpl<'surface> { impl BufferInterface for BufferImpl<'_> { fn byte_stride(&self) -> NonZeroU32 { - NonZeroU32::new(util::byte_stride(self.back.width)).unwrap() + NonZeroU32::new(util::byte_stride(self.width)).unwrap() } fn width(&self) -> NonZeroU32 { - NonZeroU32::new(self.back.width).unwrap() + NonZeroU32::new(self.width).unwrap() } fn height(&self) -> NonZeroU32 { - NonZeroU32::new(self.back.height).unwrap() + NonZeroU32::new(self.height).unwrap() } fn pixels_mut(&mut self) -> &mut [Pixel] { - // SAFETY: Called on the back buffer. - unsafe { self.back.data() } + let back = self.buffers.last_mut().unwrap(); + + // Should've been verified by `next_buffer` above. + debug_assert!(!back.might_be_in_use()); + + let num_bytes = util::byte_stride(self.width) as usize * (self.height as usize); + // SAFETY: The pointer is valid, and we know that we're the only owners of the back buffer's + // data provider. This, combined with taking `&mut self` in this function, means that we can + // safely write to the buffer. + unsafe { slice::from_raw_parts_mut(back.data_ptr, num_bytes / size_of::()) } } fn age(&self) -> u8 { - self.back.age + let back = self.buffers.last().unwrap(); + back.age } fn present_with_damage(self, _damage: &[Rect]) -> Result<(), SoftBufferError> { - self.back.age = 1; - if let Some(middle) = self.middle { - if middle.age != 0 { - middle.age += 1; - } - } - if self.front.age != 0 { - self.front.age += 1; - } - // Rotate buffers such that the back buffer is now the front buffer. - if let Some(middle) = self.middle { - mem::swap(self.back, middle); - mem::swap(middle, self.front); - } else { - mem::swap(self.back, self.front); + self.buffers.rotate_right(1); + + let (front, rest) = self.buffers.split_first_mut().unwrap(); + front.age = 1; // This buffer (previously the back buffer) was just rendered into. + + // Bump the age of the other buffers. + for buffer in rest { + if buffer.age != 0 { + buffer.age += 1; + } } // `CGBitmapInfo` consists of a combination of `CGImageAlphaInfo`, `CGImageComponentInfo` @@ -390,17 +405,18 @@ impl BufferInterface for BufferImpl<'_> { | CGImagePixelFormatInfo::Packed.0, ); - // CGImage is immutable, so we re-create it + // CGImage is (intended to be) immutable, so we re-create it on each present. + // SAFETY: The `decode` pointer is NULL. let image = unsafe { CGImage::new( - self.front.width as usize, - self.front.height as usize, + self.width as usize, + self.height as usize, 8, 32, - util::byte_stride(self.front.width) as usize, + util::byte_stride(self.width) as usize, Some(self.color_space), bitmap_info, - Some(&self.front.data_provider), + Some(&front.data_provider), ptr::null(), false, CGColorRenderingIntent::RenderingIntentDefault, @@ -427,13 +443,11 @@ impl BufferInterface for BufferImpl<'_> { struct Buffer { data_provider: CFRetained, data_ptr: *mut Pixel, - width: u32, - height: u32, age: u8, } -// SAFETY: We only mutate the `CGDataProvider` when we know it's the back buffer, and only then -// behind `&mut`. +// SAFETY: We only mutate the `CGDataProvider` when we know it's not referenced by anything else, +// and only then behind `&mut`. unsafe impl Send for Buffer {} // SAFETY: Same as above. unsafe impl Sync for Buffer {} @@ -466,30 +480,17 @@ impl Buffer { Self { data_provider, data_ptr: data_ptr.cast(), - width, - height, age: 0, } } - /// Hint for whether the data provider is currently being used. + /// Whether the buffer might currently be in use. /// - /// Might return `false`, but if this returns `true`, the provider is definitely not in use. - fn is_in_use(&self) -> bool { + /// Might return `false` even if the buffer is unused (such as if it ended up in an autorelease + /// pool), but if this returns `true`, the provider is definitely not in use. + fn might_be_in_use(&self) -> bool { self.data_provider.retain_count() != 1 } - - /// # Safety - /// - /// Must only be called on the back buffer. - unsafe fn data(&mut self) -> &mut [Pixel] { - // Check that nobody else is using the data provider. - debug_assert!(!self.is_in_use()); - - let num_bytes = util::byte_stride(self.width) as usize * (self.height as usize); - // SAFETY: The pointer is valid, and ownership rules are upheld by caller. - unsafe { slice::from_raw_parts_mut(self.data_ptr, num_bytes / size_of::()) } - } } #[derive(Debug)]