From 6cafdcfa45e186cf3edf9e6fed8637068d03319a Mon Sep 17 00:00:00 2001
From: Victoria Fischer <tdfischer@hackerbots.net>
Date: Mon, 2 Dec 2024 19:36:17 +0100
Subject: [PATCH] render: remove getters from Surface, reimplement buffer
 sharing with Send+Sync

---
 Cargo.toml            |   2 -
 src/buffers.rs        | 327 ++++++++++++++++++++++++++++++------------
 src/main.rs           |   4 +
 src/platform/esp32.rs |  24 ++--
 src/render.rs         |   4 -
 5 files changed, 247 insertions(+), 114 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 4fe0e15..11385c2 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -29,8 +29,6 @@ pio = ["esp-idf-svc/pio"]
 std = ["alloc", "esp-idf-svc/binstart", "esp-idf-svc/std"]
 alloc = ["esp-idf-svc/alloc"]
 
-threads = []
-
 [dependencies]
 log = { version = "0.4", default-features = false }
 esp-idf-svc = { version = "0.49", default-features = false }
diff --git a/src/buffers.rs b/src/buffers.rs
index e259597..a58119d 100644
--- a/src/buffers.rs
+++ b/src/buffers.rs
@@ -2,88 +2,259 @@ use crate::geometry::*;
 use crate::lib8::interpolate::Fract8Ops;
 use crate::power::AsMilliwatts;
 use crate::render::{PixelView, Sample, Shader, Surface, Surfaces, HardwarePixel};
+use crate::task::Task;
 
+use std::borrow::BorrowMut;
 use std::fmt::Debug;
 use std::rc::Rc;
 use std::cell::RefCell;
 use std::io;
 use std::ops::IndexMut;
 
-#[cfg(feature="threads")]
+use std::sync::atomic::AtomicBool;
+use std::sync::RwLock;
 use std::sync::{Arc, Mutex};
 
 #[derive(Debug)]
-pub struct ShaderBinding {
+struct ShaderBinding {
     shader: Option<Box<dyn Shader>>,
     rect: Rectangle<Virtual>,
     opacity: u8
 }
 
-#[derive(Clone)]
-pub struct BoundSurface<T> {
-    pub binding: T
+#[derive(Debug)]
+struct SurfaceUpdate {
+    shader: Option<Option<Box<dyn Shader>>>,
+    rect: Option<Rectangle<Virtual>>,
+    opacity: Option<u8>,
+    slot: usize
 }
 
-impl Debug for BoundSurface<Rc<RefCell<ShaderBinding>>> {
+impl SurfaceUpdate {
+    fn merge(&mut self, mut other: Self) {
+        if other.shader.is_some() {
+            self.shader = other.shader.take()
+        }
+        if other.rect.is_some() {
+            self.rect = other.rect.take()
+        }
+        if other.opacity.is_some() {
+            self.opacity = other.opacity.take()
+        }
+    }
+}
+
+impl Debug for Box<dyn Shader> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.debug_struct("BoundSurface")
-        .field("shader", &self.binding.borrow().shader)
-        .field("opacity", &self.binding.borrow().opacity)
-        .finish()
+        f.debug_struct("Shader").finish()
     }
 }
 
-pub type SimpleSurface = BoundSurface<Rc<RefCell<ShaderBinding>>>;
-
-impl Default for BoundSurface<Rc<RefCell<ShaderBinding>>>{
+impl Default for SurfaceUpdate {
     fn default() -> Self {
-        Self {
-            binding: Rc::new(RefCell::new(ShaderBinding {
-                shader: None,
-                rect: Rectangle::everything(),
-                opacity: 255
-            })),
+        SurfaceUpdate {
+            shader: None,
+            rect: None,
+            opacity: None,
+            slot: usize::MAX
         }
     }
 }
 
-impl Surface for BoundSurface<Rc<RefCell<ShaderBinding>>> {
-    fn rect(&self) -> Rectangle<Virtual> {
-        self.binding.borrow().rect
-    }
-
-    fn with_shader<F: FnMut(&dyn Shader)>(&self, mut f: F) {
-        if let Some(ref shader) = self.binding.borrow().shader {
-            f(shader.as_ref());
-        }
-    }
-
-    fn set_shader(&mut self, shader: Box<dyn Shader>) {
-        self.binding.borrow_mut().shader = Some(shader);
-    }
+#[derive(Debug)]
+pub struct BufferedSurface {
+    updater: Arc<UpdateQueue>,
+    slot: usize
+}
 
+impl Surface for BufferedSurface {
     fn clear_shader(&mut self) {
-        self.binding.borrow_mut().shader = None;
-    }
-    
-    fn set_rect(&mut self, rect: &Rectangle<Virtual>) {
-        self.binding.borrow_mut().rect = *rect;
-    }
-
-    fn opacity(&self) -> u8 {
-        self.binding.borrow().opacity
+        self.updater.push(SurfaceUpdate {
+            shader: None,
+            slot: self.slot,
+            ..Default::default()
+        });
     }
 
     fn set_opacity(&mut self, opacity: u8) {
-        self.binding.borrow_mut().opacity = opacity
+        self.updater.push(SurfaceUpdate {
+            opacity: Some(opacity),
+            slot: self.slot,
+            ..Default::default()
+        });
+    }
+
+    fn set_rect(&mut self, rect: &Rectangle<Virtual>) {
+        self.updater.push(SurfaceUpdate {
+            rect: Some(rect.clone()),
+            slot: self.slot,
+            ..Default::default()
+        });
+    }
+
+    fn set_shader(&mut self, shader: Box<dyn Shader>) {
+        self.updater.push(SurfaceUpdate {
+            shader: Some(Some(shader)),
+            slot: self.slot,
+            ..Default::default()
+        });
     }
 }
 
-#[cfg(feature="threads")]
-pub type SharedSurface = BoundSurface<Arc<Mutex<ShaderBinding>>>;
+#[derive(Debug)]
+struct UpdateQueue {
+    pending: Mutex<Vec<SurfaceUpdate>>,
+    damaged: AtomicBool
+}
 
-#[cfg(feature="threads")]
-impl Default for BoundSurface<Arc<Mutex<ShaderBinding>>> {
+impl UpdateQueue {
+    fn new() -> Self {
+        UpdateQueue {
+            pending: Mutex::new(Vec::new()),
+            damaged: AtomicBool::new(false)
+        }
+    }
+
+    fn push(&self, update: SurfaceUpdate) {
+        let mut locked = self.pending.lock().unwrap();
+        let mut existing_slot = None;
+        for existing in locked.iter_mut() {
+            if existing.slot == update.slot {
+                existing_slot = Some(existing);
+                break
+            }
+        }
+        match existing_slot {
+            Some(tgt) => {
+                log::debug!("Updating existing shader update");
+                tgt.merge(update);
+            }
+            _ => {
+                log::debug!("Pushing new shader update");
+                locked.push(update);
+                self.damaged.store(true, std::sync::atomic::Ordering::Relaxed);
+            }
+        }
+    }
+}
+
+#[derive(Debug)]
+pub struct ShaderChain {
+    bindings: Vec<ShaderBinding>,
+    updates: Arc<UpdateQueue>
+}
+
+impl ShaderChain {
+    pub fn new() -> Self {
+        ShaderChain {
+            bindings: Vec::new(),
+            updates: Arc::new(UpdateQueue::new())
+        }
+    }
+
+    pub fn is_dirty(&self) -> bool {
+        self.updates.damaged.load(std::sync::atomic::Ordering::Relaxed)
+    }
+
+    pub fn commit(&mut self) {
+        log::info!("commit");
+        let mut queue: Vec<SurfaceUpdate> = {
+            let mut updates = self.updates.pending.lock().unwrap();
+            std::mem::take(updates.as_mut())
+        };
+        log::info!("queue={:?}", queue);
+        for update in queue.iter_mut() {
+            let target_slot = &mut self.bindings[update.slot];
+            log::info!("orig={:?} update={:?}", target_slot, update);
+            if let Some(shader) = update.shader.take() {
+                target_slot.shader = shader;
+            }
+            if let Some(opacity) = update.opacity.take() {
+                target_slot.opacity = opacity;
+            }
+            if let Some(rect) = update.rect.take() {
+                target_slot.rect = rect;
+            }
+        }
+        self.updates.damaged.store(false, std::sync::atomic::Ordering::Relaxed);
+    }
+}
+
+impl Surfaces for ShaderChain {
+    type Error = ();
+    type Surface = BufferedSurface;
+
+    fn new_surface(&mut self, area: &Rectangle<Virtual>) -> Result<Self::Surface, Self::Error> {
+        let next_slot = self.bindings.len();
+        self.bindings.push(ShaderBinding {
+            opacity: 255,
+            shader: None,
+            rect: area.clone()
+        });
+
+        Ok(BufferedSurface {
+            updater: Arc::clone(&self.updates),
+            slot: next_slot
+        })
+    }
+
+    fn render_to<S: Sample>(&self, output: &mut S, frame: usize) {
+        for surface in self.bindings.iter() {
+            let opacity = surface.opacity;
+            if opacity > 0 {
+                let rect = surface.rect;
+                let mut sample = output.sample(&rect);
+                if let Some(ref shader) = surface.shader {
+                    while let Some((virt_coords, pixel)) = sample.next() {
+                        *pixel = pixel.blend8(shader.draw(&virt_coords, frame).into(), opacity);
+                    }
+                }
+            }
+        }
+    }
+}
+
+#[derive(Clone)]
+pub struct BufferedSurfacePool {
+    pool: Arc<RwLock<ShaderChain>>
+}
+
+impl BufferedSurfacePool {
+    pub fn new() -> Self {
+        BufferedSurfacePool {
+            pool: Arc::new(RwLock::new(ShaderChain::new()))
+        }
+    }
+}
+
+impl Surfaces for BufferedSurfacePool {
+    type Error = ();
+    type Surface = <ShaderChain as Surfaces>::Surface;
+    fn new_surface(&mut self, area: &crate::geometry::Rectangle<crate::geometry::Virtual>) -> Result<Self::Surface, Self::Error> {
+        self.pool.write().unwrap().new_surface(area)
+    }
+
+    fn render_to<S: crate::render::Sample>(&self, output: &mut S, frame: usize) {
+        self.pool.read().unwrap().render_to(output, frame);
+    }
+}
+
+
+impl Task for BufferedSurfacePool {
+    fn tick(&mut self) {
+        if self.pool.read().unwrap().is_dirty() {
+            self.pool.write().unwrap().commit();
+        }
+    }
+}
+
+#[derive(Clone)]
+pub struct SharedSurface {
+    binding: Arc<Mutex<ShaderBinding>>
+}
+
+
+impl Default for SharedSurface {
     fn default() -> Self {
         Self {
             binding: Arc::new(Mutex::new(ShaderBinding {
@@ -95,19 +266,7 @@ impl Default for BoundSurface<Arc<Mutex<ShaderBinding>>> {
     }
 }
 
-#[cfg(feature="threads")]
-impl Surface for BoundSurface<Arc<Mutex<ShaderBinding>>> {
-    fn rect(&self) -> Rectangle<Virtual> {
-        let r = self.binding.lock().unwrap();
-        r.rect.clone()
-    }
-
-    fn with_shader<F: FnMut(&dyn Shader)>(&self, mut f: F) {
-        if let Some(ref shader) = self.binding.lock().unwrap().shader {
-            f(shader.as_ref());
-        }
-    }
-
+impl Surface for SharedSurface {
     fn set_shader(&mut self, shader: Box<dyn Shader>) {
         self.binding.lock().unwrap().shader = Some(shader);
     }
@@ -120,68 +279,48 @@ impl Surface for BoundSurface<Arc<Mutex<ShaderBinding>>> {
         self.binding.lock().unwrap().rect = rect.clone();
     }
 
-    fn opacity(&self) -> u8 {
-        self.binding.lock().unwrap().opacity
-    }
-
     fn set_opacity(&mut self, opacity: u8) {
         self.binding.lock().unwrap().opacity = opacity
     }
 }
 
-#[cfg(feature="threads")]
-impl Debug for BoundSurface<Arc<Mutex<ShaderBinding>>> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.debug_struct("BoundSurface")
-        .field("shader", &self.binding.lock().unwrap().shader)
-        .finish()
-    }
-}
 
 #[derive(Clone)]
-pub struct SurfacePool<S: Surface + Default> {
-    surfaces: Vec<S>
+pub struct SurfacePool {
+    surfaces: Vec<SharedSurface>
 }
 
-impl<S: Surface + Debug> Debug for SurfacePool<S> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.surfaces.fmt(f)
-    }
-}
-
-impl<S: Surface + Default> SurfacePool<S> {
+impl SurfacePool {
     pub const fn new() -> Self {
         Self {
             surfaces: Vec::new()
         }
     }
-
-    pub fn iter(&self) -> std::slice::Iter<S> {
-        self.surfaces.iter()
-    }
 }
 
-impl<S: Surface + Default> Surfaces for SurfacePool<S> {
-    type Surface = S;
+impl Surfaces for SurfacePool {
+    type Surface = SharedSurface;
     type Error = io::Error;
-    fn new_surface(&mut self, area: &Rectangle<Virtual>) -> Result<S, Self::Error> {
-        let mut surface = S::default();
+    fn new_surface(&mut self, area: &Rectangle<Virtual>) -> Result<Self::Surface, Self::Error> {
+        let mut surface = SharedSurface::default();
         surface.set_rect(area);
         self.surfaces.push(surface.clone());
         return Ok(surface);
     }
 
     fn render_to<Sampler: Sample>(&self, output: &mut Sampler, frame: usize) {
-        for surface in self.iter() {
-            let opacity = surface.opacity();
+        for surface in self.surfaces.iter() {
+            let binding = surface.binding.lock().unwrap();
+            let opacity = binding.opacity;
             if opacity > 0 {
-                let rect = surface.rect();
+                let rect = binding.rect;
                 let mut sample = output.sample(&rect);
-                surface.with_shader(|shader| {
+
+                if let Some(ref shader) = binding.shader {
                     while let Some((virt_coords, pixel)) = sample.next() {
                         *pixel = pixel.blend8(shader.draw(&virt_coords, frame).into(), opacity);
                     }
-                })
+                }
             }
         }
     }
diff --git a/src/main.rs b/src/main.rs
index af45fb6..2b21fb9 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -19,6 +19,10 @@ fn main() {
 
     log::info!("Board: {}", core::any::type_name_of_val(&board));
 
+    log::info!("Creating tasks");
+    let mut system = board.system_tasks();
+    log::info!("System scheduler: {}", core::any::type_name_of_val(&system));
+
     let output = board.output();
     log::info!("Output: {}", core::any::type_name_of_val(&output));
 
diff --git a/src/platform/esp32.rs b/src/platform/esp32.rs
index 655f95d..9e15fdf 100644
--- a/src/platform/esp32.rs
+++ b/src/platform/esp32.rs
@@ -18,30 +18,25 @@ use rgb::Rgb;
 
 use super::Board;
 
+use crate::buffers::BufferedSurfacePool;
 use crate::task::FixedSizeScheduler;
 use crate::task::Task;
-use crate::buffers::{Pixbuf, SurfacePool};
+use crate::buffers::Pixbuf;
 use crate::mappings::StrideMapping;
 use crate::platform::smart_leds_lib::StrideOutput;
 use crate::platform::smart_leds_lib::rmt::FastWs2812Esp32Rmt;
 use crate::time::Periodically;
 
-#[cfg(feature="threads")]
-use crate::buffers::SharedSurface as SurfaceType;
-
-#[cfg(not(feature="threads"))]
-use crate::buffers::SimpleSurface as SurfaceType;
-
-pub struct Esp32Board<'a> {
+pub struct Esp32Board {
     output: Option<<Self as Board>::Output>,
-    surfaces: Option<SurfacePool<SurfaceType>>,
     sys_loop: EspSystemEventLoop,
     modem: Option<Modem>,
+    surfaces: BufferedSurfacePool
 }
 
-impl<'a> Board for Esp32Board<'a> {
-    type Output = StrideOutput<[Rgb<u8>; 310], FastWs2812Esp32Rmt<'a>>;
-    type Surfaces = SurfacePool<SurfaceType>;
+impl Board for Esp32Board {
+    type Output = StrideOutput<[Rgb<u8>; 310], FastWs2812Esp32Rmt<'static>>;
+    type Surfaces = BufferedSurfacePool;
     type Scheduler = FixedSizeScheduler<2>;
     
     fn take() -> Self {
@@ -88,10 +83,10 @@ impl<'a> Board for Esp32Board<'a> {
         );
 
         Esp32Board {
-            surfaces: Some(SurfacePool::new()),
             output: Some(output),
             modem: Some(peripherals.modem),
             sys_loop: sys_loop.clone(),
+            surfaces: BufferedSurfacePool::new()
         }
     }
 
@@ -100,13 +95,14 @@ impl<'a> Board for Esp32Board<'a> {
     }
 
     fn surfaces(&mut self) -> Self::Surfaces {
-        self.surfaces.take().unwrap()
+        self.surfaces.clone()
     }
 
     fn system_tasks(&mut self) -> Self::Scheduler {
         let nvs = EspDefaultNvsPartition::take().unwrap();
         FixedSizeScheduler::new([
             Box::new(WifiTask::new(self.modem.take().unwrap(), self.sys_loop.clone(), &nvs)),
+            Box::new(self.surfaces.clone())
         ])
     }
 }
diff --git a/src/render.rs b/src/render.rs
index a77cad0..658a1c8 100644
--- a/src/render.rs
+++ b/src/render.rs
@@ -1,4 +1,3 @@
-use std::io;
 use rgb::Rgb;
 
 use crate::geometry::*;
@@ -35,13 +34,10 @@ pub trait Surfaces: Send + Sync {
 }
 
 pub trait Surface: Send + Sync {
-    fn with_shader<F: FnMut(&dyn Shader)>(&self, f: F);
     fn set_shader(&mut self, shader: Box<dyn Shader>);
     fn clear_shader(&mut self);
 
-    fn rect(&self) -> Rectangle<Virtual>;
     fn set_rect(&mut self, rect: &Rectangle<Virtual>);
-    fn opacity(&self) -> u8;
     fn set_opacity(&mut self, opacity: u8);
 }