From d4385280ebe7fac0aae6fa51d47d28ce779ec2a6 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Fri, 28 May 2021 09:11:03 +0200 Subject: [PATCH] Fix disappearing images or panics when hiding and showing a window with wayland Properly release GL resource when unmapping a window and detect stale cache indices using a generation count. This fixes the disappearing images (GL textures would be invalid and need to be re-created) as well as the panics when an item's cache index ended up being re-used because some other item was drawn first and allocated that indices. The latter is fixed by using a generational counter on the cache that's bumped when clearing, instead of a single "cache_ok" bool. --- sixtyfps_runtime/corelib/graphics.rs | 45 ++++++++++++++++++- sixtyfps_runtime/corelib/item_rendering.rs | 17 ++++--- .../rendering_backends/gl/graphics_window.rs | 2 + sixtyfps_runtime/rendering_backends/gl/lib.rs | 10 +++++ 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/sixtyfps_runtime/corelib/graphics.rs b/sixtyfps_runtime/corelib/graphics.rs index aca590add..8c6b812ae 100644 --- a/sixtyfps_runtime/corelib/graphics.rs +++ b/sixtyfps_runtime/corelib/graphics.rs @@ -93,8 +93,51 @@ impl CachedGraphicsData { /// The RenderingCache, in combination with CachedGraphicsData, allows backends to store data that's either /// intensive to compute or has bad CPU locality. Backends typically keep a RenderingCache instance and use /// the item's cached_rendering_data() integer as index in the vec_arena::Arena. -pub type RenderingCache = slab::Slab>; +pub struct RenderingCache { + slab: slab::Slab>, + generation: usize, +} +impl Default for RenderingCache { + fn default() -> Self { + Self { slab: Default::default(), generation: 1 } + } +} + +impl RenderingCache { + /// Returns the generation of the cache. The generation starts at 1 and is increased + /// whenever the cache is cleared, for example when the GL context is lost. + pub fn generation(&self) -> usize { + self.generation + } + + /// Retrieves a mutable reference to the cached graphics data at index. + pub fn get_mut(&mut self, index: usize) -> Option<&mut CachedGraphicsData> { + self.slab.get_mut(index) + } + + /// Inserts data into the cache and returns the index for retrieval later. + pub fn insert(&mut self, data: CachedGraphicsData) -> usize { + self.slab.insert(data) + } + + /// Retrieves an immutable reference to the cached graphics data at index. + pub fn get(&self, index: usize) -> Option<&CachedGraphicsData> { + self.slab.get(index) + } + + /// Removes the cached graphics data at the given index. + pub fn remove(&mut self, index: usize) -> CachedGraphicsData { + self.slab.remove(index) + } + + /// Removes all entries from the cache and increases the cache's generation count, so + /// that stale index access can be avoided. + pub fn clear(&mut self) { + self.slab.clear(); + self.generation += 1; + } +} /// FontRequest collects all the developer-configurable properties for fonts, such as family, weight, etc. /// It is submitted as a request to the platform font system (i.e. CoreText on macOS) and in exchange the /// backend returns a Box. diff --git a/sixtyfps_runtime/corelib/item_rendering.rs b/sixtyfps_runtime/corelib/item_rendering.rs index c48d93357..23a5e511e 100644 --- a/sixtyfps_runtime/corelib/item_rendering.rs +++ b/sixtyfps_runtime/corelib/item_rendering.rs @@ -25,8 +25,10 @@ use std::cell::{Cell, RefCell}; pub struct CachedRenderingData { /// Used and modified by the backend, should be initialized to 0 by the user code pub(crate) cache_index: Cell, - /// Set to false initially and when changes happen that require updating the cache - pub(crate) cache_ok: Cell, + /// Used and modified by the backend, should be initilized to 0 by the user code. + /// The backend compares this generation against the one of the cache to verify + /// the validity of the cache_index field. + pub(crate) cache_generation: Cell, } impl CachedRenderingData { @@ -38,7 +40,7 @@ impl CachedRenderingData { cache: &mut RenderingCache, update_fn: impl FnOnce() -> T, ) -> T { - if self.cache_ok.get() { + if self.cache_generation.get() == cache.generation() { let index = self.cache_index.get(); if let Some(existing_entry) = cache.get_mut(index) { if let Some(data) = @@ -48,12 +50,9 @@ impl CachedRenderingData { } return existing_entry.data.clone(); } - // We may see cache_ok == true but get() on the cache failed with our index. That can - // only happen when the entire cache was destroyed in the meantime (through show and hide), - // in which case we re-allocate. } self.cache_index.set(cache.insert(crate::graphics::CachedGraphicsData::new(update_fn))); - self.cache_ok.set(true); + self.cache_generation.set(cache.generation()); cache.get(self.cache_index.get()).unwrap().data.clone() } @@ -61,10 +60,10 @@ impl CachedRenderingData { /// exists, i.e. if any data was ever cached. This is typically called by the graphics backend's /// implementation of the release_item_graphics_cache function. pub fn release(&self, cache: &mut RenderingCache) { - if self.cache_ok.get() { + if self.cache_generation.get() == cache.generation() { let index = self.cache_index.get(); cache.remove(index); - self.cache_ok.set(false); + self.cache_generation.set(0); } } } diff --git a/sixtyfps_runtime/rendering_backends/gl/graphics_window.rs b/sixtyfps_runtime/rendering_backends/gl/graphics_window.rs index abad25d69..3a52507d7 100644 --- a/sixtyfps_runtime/rendering_backends/gl/graphics_window.rs +++ b/sixtyfps_runtime/rendering_backends/gl/graphics_window.rs @@ -209,6 +209,8 @@ impl GraphicsWindow { /// Removes the window from the screen. The window is not destroyed though, it can be show (mapped) again later /// by calling [`PlatformWindow::map_window`]. fn unmap_window(self: Rc) { + // Release GL textures and other GPU bound resources. + self.graphics_cache.borrow_mut().clear(); self.map_state.replace(GraphicsWindowBackendState::Unmapped); /* FIXME: if let Some(existing_blinker) = self.cursor_blinker.borrow().upgrade() { diff --git a/sixtyfps_runtime/rendering_backends/gl/lib.rs b/sixtyfps_runtime/rendering_backends/gl/lib.rs index 52e98a96f..a7dcd62eb 100644 --- a/sixtyfps_runtime/rendering_backends/gl/lib.rs +++ b/sixtyfps_runtime/rendering_backends/gl/lib.rs @@ -112,10 +112,20 @@ impl ItemGraphicsCacheEntry { struct ItemGraphicsCache(RenderingCache>); impl ItemGraphicsCache { + /// Convenience method for releasing an item's cached graphics data. fn release(&mut self, item_data: &CachedRenderingData) { item_data.release(&mut self.0); } + /// Clears the entire graphics cache. This is needed when for example loosing + /// the GL context (when unmapping a window) and destroying the canvas. + fn clear(&mut self) { + self.0.clear(); + } + + /// Convenience method that will return what's in the item's graphics cache + /// and call update_fn if the cache is outdated and needs refreshing. If + /// update_fn is called, the data is persisted in the cache. fn ensure_up_to_date( &mut self, item_data: &CachedRenderingData,