Skip to content

Commit

Permalink
try scaling down images that don't fit in the texture atlas
Browse files Browse the repository at this point in the history
When trying to display a 4k image there is a high chance that
we'll run out of texture space and then render with no images
displayed.

This commit changes the binary yes/no allow-images into a a series
of attempts: display at natural size, scale down by 2, 4, then 8,
then give up on images.

While looking at this, I noticed that we had a TOCTOU in the blob lease
stuff in the case where we might very quickly try the handle the same
image in succession, and end up deleting a file out from under a live
lease.

I've put in place a simple bandaid for that, but it's probably worth
revisiting the concurrency model for that.
  • Loading branch information
wez committed May 19, 2023
1 parent 134bf0a commit 4ae176d
Show file tree
Hide file tree
Showing 10 changed files with 195 additions and 117 deletions.
2 changes: 1 addition & 1 deletion wezterm-blob-leases/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub enum Error {
#[error("Content with id {0} not found")]
ContentNotFound(ContentId),

#[error(transparent)]
#[error("Io error in BlobLease: {0}")]
Io(#[from] std::io::Error),

#[error("Storage has already been initialized")]
Expand Down
9 changes: 8 additions & 1 deletion wezterm-blob-leases/src/simple_tempdir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ impl SimpleTempDir {
eprintln!("Failed to remove {}: {err:#}", path.display());
}
}
*count = 0;
}
Some(count) => {
*count -= 1;
Expand All @@ -57,6 +58,8 @@ impl SimpleTempDir {

impl BlobStorage for SimpleTempDir {
fn store(&self, content_id: ContentId, data: &[u8], _lease_id: LeaseId) -> Result<(), Error> {
let mut refs = self.refs.lock().unwrap();

let path = self.path_for_content(content_id)?;
let mut file = tempfile::Builder::new()
.prefix("new-")
Expand All @@ -67,12 +70,14 @@ impl BlobStorage for SimpleTempDir {
file.persist(&path)
.map_err(|persist_err| persist_err.error)?;

self.add_ref(content_id);
*refs.entry(content_id).or_insert(0) += 1;

Ok(())
}

fn lease_by_content(&self, content_id: ContentId, _lease_id: LeaseId) -> Result<(), Error> {
let _refs = self.refs.lock().unwrap();

let path = self.path_for_content(content_id)?;
if path.exists() {
self.add_ref(content_id);
Expand All @@ -83,6 +88,8 @@ impl BlobStorage for SimpleTempDir {
}

fn get_data(&self, content_id: ContentId, _lease_id: LeaseId) -> Result<Vec<u8>, Error> {
let _refs = self.refs.lock().unwrap();

let path = self.path_for_content(content_id)?;
Ok(std::fs::read(&path)?)
}
Expand Down
27 changes: 23 additions & 4 deletions wezterm-gui/src/glyphcache.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::utilsprites::RenderMetrics;
use crate::customglyph::*;
use crate::renderstate::RenderContext;
use crate::termwindow::render::paint::AllowImage;
use ::window::bitmaps::atlas::{Atlas, OutOfTextureSpace, Sprite};
use ::window::bitmaps::{BitmapImage, Image, ImageTexture, Texture2d};
use ::window::color::SrgbaPixel;
Expand Down Expand Up @@ -885,17 +886,26 @@ impl GlyphCache {
decoded: &DecodedImage,
padding: Option<usize>,
min_frame_duration: Duration,
allow_image: AllowImage,
) -> anyhow::Result<(Sprite, Option<Instant>, LoadState)> {
let mut handle = DecodedImageHandle {
h: decoded.image.data(),
current_frame: *decoded.current_frame.borrow(),
};

let scale_down = match allow_image {
AllowImage::Scale(n) => Some(n),
_ => None,
};

match &*handle.h {
ImageDataType::Rgba8 { hash, .. } => {
if let Some(sprite) = frame_cache.get(hash) {
return Ok((sprite.clone(), None, LoadState::Loaded));
}
let sprite = atlas.allocate_with_padding(&handle, padding)?;
let sprite = atlas
.allocate_with_padding(&handle, padding, scale_down)
.context("atlas.allocate_with_padding")?;
frame_cache.insert(*hash, sprite.clone());

return Ok((sprite, None, LoadState::Loaded));
Expand Down Expand Up @@ -949,7 +959,9 @@ impl GlyphCache {
return Ok((sprite.clone(), next, LoadState::Loaded));
}

let sprite = atlas.allocate_with_padding(&handle, padding)?;
let sprite = atlas
.allocate_with_padding(&handle, padding, scale_down)
.context("atlas.allocate_with_padding")?;

frame_cache.insert(hash, sprite.clone());

Expand Down Expand Up @@ -1005,9 +1017,13 @@ impl GlyphCache {
let frame = Image::from_raw(
frames.current_frame.width,
frames.current_frame.height,
frames.current_frame.lease.get_data()?,
frames
.current_frame
.lease
.get_data()
.context("frames.current_frame.lease.get_data")?,
);
let sprite = atlas.allocate_with_padding(&frame, padding)?;
let sprite = atlas.allocate_with_padding(&frame, padding, scale_down)?;

frame_cache.insert(hash, sprite.clone());

Expand All @@ -1024,6 +1040,7 @@ impl GlyphCache {
&mut self,
image_data: &Arc<ImageData>,
padding: Option<usize>,
allow_image: AllowImage,
) -> anyhow::Result<(Sprite, Option<Instant>, LoadState)> {
let hash = image_data.hash();

Expand All @@ -1034,6 +1051,7 @@ impl GlyphCache {
decoded,
padding,
self.min_frame_duration,
allow_image,
)
} else {
let decoded = DecodedImage::load(image_data);
Expand All @@ -1043,6 +1061,7 @@ impl GlyphCache {
&decoded,
padding,
self.min_frame_duration,
allow_image,
)?;
self.image_cache.put(hash, decoded);
Ok(res)
Expand Down
9 changes: 5 additions & 4 deletions wezterm-gui/src/termwindow/background.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,11 @@ impl crate::TermWindow {

let color = bg_color.mul_alpha(layer.def.opacity);

let (sprite, next_due, load_state) = gl_state
.glyph_cache
.borrow_mut()
.cached_image(&layer.source, None)?;
let (sprite, next_due, load_state) = gl_state.glyph_cache.borrow_mut().cached_image(
&layer.source,
None,
self.allow_images,
)?;
self.update_next_frame_time(next_due);

if load_state == LoadState::Loading {
Expand Down
7 changes: 4 additions & 3 deletions wezterm-gui/src/termwindow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::termwindow::background::{
};
use crate::termwindow::keyevent::{KeyTableArgs, KeyTableState};
use crate::termwindow::modal::Modal;
use crate::termwindow::render::paint::AllowImage;
use crate::termwindow::render::{
CachedLineState, LineQuadCacheKey, LineQuadCacheValue, LineToEleShapeCacheKey,
LineToElementShapeItem,
Expand Down Expand Up @@ -73,7 +74,7 @@ mod mouseevent;
pub mod palette;
pub mod paneselect;
mod prevcursor;
mod render;
pub mod render;
pub mod resize;
mod selection;
pub mod spawn;
Expand Down Expand Up @@ -434,7 +435,7 @@ pub struct TermWindow {
has_animation: RefCell<Option<Instant>>,
/// We use this to attempt to do something reasonable
/// if we run out of texture space
allow_images: bool,
allow_images: AllowImage,
scheduled_animation: RefCell<Option<Instant>>,

created: Instant,
Expand Down Expand Up @@ -762,7 +763,7 @@ impl TermWindow {
current_event: None,
has_animation: RefCell::new(None),
scheduled_animation: RefCell::new(None),
allow_images: true,
allow_images: AllowImage::Yes,
semantic_zones: HashMap::new(),
ui_items: vec![],
dragging: None,
Expand Down
8 changes: 5 additions & 3 deletions wezterm-gui/src/termwindow/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ use crate::quad::{
TripleLayerQuadAllocatorTrait,
};
use crate::shapecache::*;
use crate::termwindow::render::paint::AllowImage;
use crate::termwindow::{BorrowedShapeCacheKey, RenderState, ShapedInfo, TermWindowNotif};
use crate::utilsprites::RenderMetrics;
use ::window::bitmaps::{TextureCoord, TextureRect, TextureSize};
use ::window::{DeadKeyStatus, PointF, RectF, SizeF, WindowOps};
use anyhow::anyhow;
use anyhow::{anyhow, Context};
use config::{BoldBrightening, ConfigHandle, DimensionContext, TextStyle, VisualBellTarget};
use euclid::num::Zero;
use mux::pane::{Pane, PaneId};
Expand Down Expand Up @@ -417,7 +418,7 @@ impl crate::TermWindow {
hsv: Option<config::HsbTransform>,
glyph_color: LinearRgba,
) -> anyhow::Result<()> {
if !self.allow_images {
if self.allow_images == AllowImage::No {
return Ok(());
}

Expand All @@ -435,7 +436,8 @@ impl crate::TermWindow {
let (sprite, next_due, _load_state) = gl_state
.glyph_cache
.borrow_mut()
.cached_image(image.image_data(), Some(padding))?;
.cached_image(image.image_data(), Some(padding), self.allow_images)
.context("cached_image")?;
self.update_next_frame_time(next_due);
let width = sprite.coords.size.width;
let height = sprite.coords.size.height;
Expand Down
47 changes: 30 additions & 17 deletions wezterm-gui/src/termwindow/render/paint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,21 @@ use smol::Timer;
use std::time::{Duration, Instant};
use wezterm_font::ClearShapeCache;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum AllowImage {
Yes,
Scale(usize),
No,
}

impl crate::TermWindow {
pub fn paint_impl(&mut self, frame: &mut RenderFrame) {
self.num_frames += 1;
// If nothing on screen needs animating, then we can avoid
// invalidating as frequently
*self.has_animation.borrow_mut() = None;
// Start with the assumption that we should allow images to render
self.allow_images = true;
self.allow_images = AllowImage::Yes;

let start = Instant::now();

Expand Down Expand Up @@ -61,21 +68,27 @@ impl crate::TermWindow {
self.invalidate_modal();

if let Err(err) = result {
if self.allow_images {
self.allow_images = false;
log::info!(
"Not enough texture space ({:#}); \
will retry render with images disabled",
err
);
} else {
log::error!(
"Failed to {} texture: {}",
if pass == 0 { "clear" } else { "resize" },
err
);
break 'pass;
}
self.allow_images = match self.allow_images {
AllowImage::Yes => AllowImage::Scale(2),
AllowImage::Scale(2) => AllowImage::Scale(4),
AllowImage::Scale(4) => AllowImage::Scale(8),
AllowImage::Scale(8) => AllowImage::No,
AllowImage::No | _ => {
log::error!(
"Failed to {} texture: {}",
if pass == 0 { "clear" } else { "resize" },
err
);
break 'pass;
}
};

log::info!(
"Not enough texture space ({:#}); \
will retry render with {:?}",
err,
self.allow_images,
);
}
} else if err.root_cause().downcast_ref::<ClearShapeCache>().is_some() {
self.invalidate_fancy_tab_bar();
Expand Down Expand Up @@ -175,7 +188,7 @@ impl crate::TermWindow {

// Render the full window background
match (self.window_background.is_empty(), self.allow_images) {
(false, true) => {
(false, AllowImage::Yes) => {
let bg_color = self.palette().background.to_linear();

let top = panes
Expand Down
Loading

0 comments on commit 4ae176d

Please sign in to comment.