Merge pull request #762 from zed-industries/safer-atlas-allocation

Max Brunsfeld created

Safer Atlas Allocation

Change summary

crates/gpui/src/platform/mac/atlas.rs        | 34 ++++++++++++---------
crates/gpui/src/platform/mac/image_cache.rs  |  5 ++
crates/gpui/src/platform/mac/renderer.rs     | 14 ++++++++
crates/gpui/src/platform/mac/sprite_cache.rs | 33 +++++++++++---------
4 files changed, 54 insertions(+), 32 deletions(-)

Detailed changes

crates/gpui/src/platform/mac/atlas.rs 🔗

@@ -2,9 +2,9 @@ use crate::geometry::{
     rect::RectI,
     vector::{vec2i, Vector2I},
 };
-use anyhow::anyhow;
 use etagere::BucketedAtlasAllocator;
 use foreign_types::ForeignType;
+use log::warn;
 use metal::{Device, TextureDescriptor};
 use objc::{msg_send, sel, sel_impl};
 
@@ -41,36 +41,40 @@ impl AtlasAllocator {
         )
     }
 
-    pub fn allocate(&mut self, requested_size: Vector2I) -> (AllocId, Vector2I) {
-        let (alloc_id, origin) = self
+    pub fn allocate(&mut self, requested_size: Vector2I) -> Option<(AllocId, Vector2I)> {
+        let allocation = self
             .atlases
             .last_mut()
             .unwrap()
             .allocate(requested_size)
-            .unwrap_or_else(|| {
+            .or_else(|| {
                 let mut atlas = self.new_atlas(requested_size);
-                let (id, origin) = atlas
-                    .allocate(requested_size)
-                    .ok_or_else(|| {
-                        anyhow!("could not allocate requested size {:?}", requested_size)
-                    })
-                    .unwrap();
+                let (id, origin) = atlas.allocate(requested_size)?;
                 self.atlases.push(atlas);
-                (id, origin)
+                Some((id, origin))
             });
 
+        if allocation.is_none() {
+            warn!(
+                "allocation of size {:?} could not be created",
+                requested_size,
+            );
+        }
+
+        let (alloc_id, origin) = allocation?;
+
         let id = AllocId {
             atlas_id: self.atlases.len() - 1,
             alloc_id,
         };
-        (id, origin)
+        Some((id, origin))
     }
 
-    pub fn upload(&mut self, size: Vector2I, bytes: &[u8]) -> (AllocId, RectI) {
-        let (alloc_id, origin) = self.allocate(size);
+    pub fn upload(&mut self, size: Vector2I, bytes: &[u8]) -> Option<(AllocId, RectI)> {
+        let (alloc_id, origin) = self.allocate(size)?;
         let bounds = RectI::new(origin, size);
         self.atlases[alloc_id.atlas_id].upload(bounds, bytes);
-        (alloc_id, bounds)
+        Some((alloc_id, bounds))
     }
 
     pub fn deallocate(&mut self, id: AllocId) {

crates/gpui/src/platform/mac/image_cache.rs 🔗

@@ -1,3 +1,4 @@
+use anyhow::anyhow;
 use metal::{MTLPixelFormat, TextureDescriptor, TextureRef};
 
 use super::atlas::{AllocId, AtlasAllocator};
@@ -31,7 +32,9 @@ impl ImageCache {
             .prev_frame
             .remove(&image.id)
             .or_else(|| self.curr_frame.get(&image.id).copied())
-            .unwrap_or_else(|| self.atlases.upload(image.size(), image.as_bytes()));
+            .or_else(|| self.atlases.upload(image.size(), image.as_bytes()))
+            .ok_or_else(|| anyhow!("could not upload image of size {:?}", image.size()))
+            .unwrap();
         self.curr_frame.insert(image.id, (alloc_id, atlas_bounds));
         (alloc_id, atlas_bounds)
     }

crates/gpui/src/platform/mac/renderer.rs 🔗

@@ -9,6 +9,7 @@ use crate::{
     scene::{Glyph, Icon, Image, Layer, Quad, Scene, Shadow, Underline},
 };
 use cocoa::foundation::NSUInteger;
+use log::warn;
 use metal::{MTLPixelFormat, MTLResourceOptions, NSRange};
 use shaders::ToFloat2 as _;
 use std::{collections::HashMap, ffi::c_void, iter::Peekable, mem, sync::Arc, vec};
@@ -172,7 +173,14 @@ impl Renderer {
             for path in layer.paths() {
                 let origin = path.bounds.origin() * scene.scale_factor();
                 let size = (path.bounds.size() * scene.scale_factor()).ceil();
-                let (alloc_id, atlas_origin) = self.path_atlases.allocate(size.to_i32());
+
+                let path_allocation = self.path_atlases.allocate(size.to_i32());
+                if path_allocation.is_none() {
+                    // Path size was likely zero.
+                    warn!("could not allocate path texture of size {:?}", size);
+                    continue;
+                }
+                let (alloc_id, atlas_origin) = path_allocation.unwrap();
                 let atlas_origin = atlas_origin.to_f32();
                 sprites.push(PathSprite {
                     layer_id,
@@ -569,6 +577,10 @@ impl Renderer {
             let sprite =
                 self.sprite_cache
                     .render_icon(source_size, icon.path.clone(), icon.svg.clone());
+            if sprite.is_none() {
+                continue;
+            }
+            let sprite = sprite.unwrap();
 
             sprites_by_atlas
                 .entry(sprite.atlas_id)

crates/gpui/src/platform/mac/sprite_cache.rs 🔗

@@ -4,6 +4,7 @@ use crate::{
     geometry::vector::{vec2f, Vector2F, Vector2I},
     platform,
 };
+use collections::hash_map::Entry;
 use metal::{MTLPixelFormat, TextureDescriptor};
 use ordered_float::OrderedFloat;
 use std::{borrow::Cow, collections::HashMap, sync::Arc};
@@ -114,7 +115,9 @@ impl SpriteCache {
                     scale_factor,
                 )?;
 
-                let (alloc_id, atlas_bounds) = atlases.upload(glyph_bounds.size(), &mask);
+                let (alloc_id, atlas_bounds) = atlases
+                    .upload(glyph_bounds.size(), &mask)
+                    .expect("could not upload glyph");
                 Some(GlyphSprite {
                     atlas_id: alloc_id.atlas_id,
                     atlas_origin: atlas_bounds.origin(),
@@ -130,15 +133,15 @@ impl SpriteCache {
         size: Vector2I,
         path: Cow<'static, str>,
         svg: usvg::Tree,
-    ) -> IconSprite {
+    ) -> Option<IconSprite> {
         let atlases = &mut self.atlases;
-        self.icons
-            .entry(IconDescriptor {
-                path,
-                width: size.x(),
-                height: size.y(),
-            })
-            .or_insert_with(|| {
+        match self.icons.entry(IconDescriptor {
+            path,
+            width: size.x(),
+            height: size.y(),
+        }) {
+            Entry::Occupied(entry) => Some(entry.get().clone()),
+            Entry::Vacant(entry) => {
                 let mut pixmap = tiny_skia::Pixmap::new(size.x() as u32, size.y() as u32).unwrap();
                 resvg::render(&svg, usvg::FitTo::Width(size.x() as u32), pixmap.as_mut());
                 let mask = pixmap
@@ -146,15 +149,15 @@ impl SpriteCache {
                     .iter()
                     .map(|a| a.alpha())
                     .collect::<Vec<_>>();
-
-                let (alloc_id, atlas_bounds) = atlases.upload(size, &mask);
-                IconSprite {
+                let (alloc_id, atlas_bounds) = atlases.upload(size, &mask)?;
+                let icon_sprite = IconSprite {
                     atlas_id: alloc_id.atlas_id,
                     atlas_origin: atlas_bounds.origin(),
                     size,
-                }
-            })
-            .clone()
+                };
+                Some(entry.insert(icon_sprite).clone())
+            }
+        }
     }
 
     pub fn atlas_texture(&self, atlas_id: usize) -> Option<&metal::TextureRef> {