From e8aa5451c3ee2745c9d1864fdb9456fccfe4b13b Mon Sep 17 00:00:00 2001 From: Sophie Herold Date: Mon, 10 Apr 2023 17:53:15 +0200 Subject: [PATCH] decoder: Avoid unwraps and report issues to UI Those are mostly cases that should never occur. But especially for rare erros it can be helpful to directly display them in the UI. --- src/decoder/formats/heif.rs | 21 ++++++++------- src/decoder/formats/image_rs_other.rs | 35 +++++++++++++++--------- src/decoder/formats/mod.rs | 2 +- src/decoder/formats/svg.rs | 38 ++++++++++++++++++++------- src/decoder/mod.rs | 27 +++++++++++++++---- 5 files changed, 86 insertions(+), 37 deletions(-) diff --git a/src/decoder/formats/heif.rs b/src/decoder/formats/heif.rs index d9bea2f..ac41425 100644 --- a/src/decoder/formats/heif.rs +++ b/src/decoder/formats/heif.rs @@ -89,7 +89,7 @@ impl Heif { position: (0, 0), zoom_level: tiling::zoom_to_level(1.), bleed: 0, - texture: decoded.into_texture(), + texture: decoded.into_texture()?, }; tiles.push(tile); @@ -109,18 +109,21 @@ pub struct Decoded<'a> { } impl<'a> Decoded<'a> { - pub fn into_texture(self) -> gdk::Texture { + pub fn into_texture(self) -> anyhow::Result { let memory_format = match self.rgb_chroma { RgbChroma::HdrRgbBe | RgbChroma::HdrRgbaBe | RgbChroma::HdrRgbLe | RgbChroma::HdrRgbaLe => { - let transmuted: &mut [u16] = - safe_transmute::transmute_many_pedantic_mut(self.plane.data).unwrap(); - - // Scale HDR pixels to 16bit (they are usually 10bit or 12bit) - for pixel in transmuted.iter_mut() { - *pixel <<= 16 - self.plane.bits_per_pixel; + if let Ok(transmuted) = + safe_transmute::transmute_many_pedantic_mut::(self.plane.data) + { + // Scale HDR pixels to 16bit (they are usually 10bit or 12bit) + for pixel in transmuted.iter_mut() { + *pixel <<= 16 - self.plane.bits_per_pixel; + } + } else { + log::error!("Could not transform HDR (16bit) data to u16"); } if self.has_alpha_channel { @@ -157,6 +160,6 @@ impl<'a> Decoded<'a> { self.plane.stride, ); - tex.upcast() + Ok(tex.upcast()) } } diff --git a/src/decoder/formats/image_rs_other.rs b/src/decoder/formats/image_rs_other.rs index 5abe86d..987d047 100644 --- a/src/decoder/formats/image_rs_other.rs +++ b/src/decoder/formats/image_rs_other.rs @@ -5,7 +5,7 @@ use crate::deps::*; use crate::util::gettext::*; use crate::util::{BufReadSeek, ToBufRead}; -use anyhow::Context; +use anyhow::{bail, Context}; use arc_swap::ArcSwap; use gtk::prelude::*; use image_rs::AnimationDecoder; @@ -21,8 +21,12 @@ pub struct ImageRsOther { impl Drop for ImageRsOther { fn drop(&mut self) { if let Some(handle) = &self.thread_handle { - let mut abort = self.abort.write().unwrap(); - *abort = true; + if let Ok(mut abort) = self.abort.write() { + *abort = true; + } else { + // Don't use log in drop because not sure if always still initialized + eprintln!("Unable to write abort signal to decoding thread."); + } handle.thread().unpark(); } } @@ -95,7 +99,7 @@ impl ImageRsOther { position: (0, 0), zoom_level: tiling::zoom_to_level(1.), bleed: 0, - texture: decoded_image.into_texture(), + texture: decoded_image.into_texture()?, }; tiles.push(tile); @@ -179,7 +183,7 @@ impl ImageRsOther { position, zoom_level: tiling::zoom_to_level(1.), bleed: 0, - texture: decoded_image.into_texture(), + texture: decoded_image.into_texture()?, }; tiles.push_frame(tile, dimensions, delay); @@ -196,9 +200,13 @@ impl ImageRsOther { std::thread::park(); } - if *abort.read().unwrap() { - log::debug!("Terminating decoder thread."); - return Ok(()); + if let Ok(abort_state) = abort.read().as_deref() { + if *abort_state { + log::debug!("Terminating decoder thread."); + return Ok(()); + } + } else { + bail!("Cannot read decoder thread abort instructions."); } } } @@ -219,7 +227,7 @@ pub struct Decoded { } impl Decoded { - pub fn into_texture(self) -> gdk::Texture { + pub fn into_texture(self) -> anyhow::Result { let (n_bytes, memory_format, layout, data) = match self.dynamic_image { img @ image_rs::DynamicImage::ImageLuma8(_) => { let buffer = img.to_rgb8(); @@ -299,20 +307,23 @@ impl Decoded { 2, gdk::MemoryFormat::R16g16b16a16, buffer.sample_layout(), - safe_transmute::transmute_vec(buffer.into_raw()).unwrap(), + safe_transmute::transmute_vec(buffer.into_raw()) + .context("Failed to transform into 16 bit texture")?, ) } }; let bytes = glib::Bytes::from_owned(data); - gdk::MemoryTexture::new( + let texture = gdk::MemoryTexture::new( layout.width as i32, layout.height as i32, memory_format, &bytes, layout.height_stride * n_bytes, ) - .upcast() + .upcast(); + + Ok(texture) } } diff --git a/src/decoder/formats/mod.rs b/src/decoder/formats/mod.rs index 5f63ddc..0630ef0 100644 --- a/src/decoder/formats/mod.rs +++ b/src/decoder/formats/mod.rs @@ -6,7 +6,7 @@ pub use heif::Heif; pub use image_rs_other::ImageRsOther; pub use svg::{Svg, RSVG_MAX_SIZE}; -use super::{Decoder, DecoderUpdate, UpdateSender}; +use super::{DecoderUpdate, UpdateSender}; use crate::util::gettext::*; #[derive(Clone, Copy, Debug)] diff --git a/src/decoder/formats/svg.rs b/src/decoder/formats/svg.rs index e11502c..2424efb 100644 --- a/src/decoder/formats/svg.rs +++ b/src/decoder/formats/svg.rs @@ -36,8 +36,12 @@ impl Request { impl Drop for Svg { fn drop(&mut self) { - let mut request = self.current_request.write().unwrap(); - *request = Request::Exit; + if let Ok(mut request) = self.current_request.write() { + *request = Request::Exit; + } else { + // Don't use log in drop because not sure if always still initialized + eprintln!("Unable to write exit request: RwLock is poisoned."); + } self.thread_handle.thread().unpark(); } } @@ -65,7 +69,7 @@ impl Svg { loop { let tile_request = { - let mut request = request_store.write().unwrap(); + let mut request = request_store.write().ok().context("RwLock is poisoned")?; let value = request.clone(); *request = Request::None; value @@ -90,7 +94,12 @@ impl Svg { let relevant_tiles = tiling.relevant_tiles(&tile_request.viewport); for tile_instructions in relevant_tiles { - if request_store.read().unwrap().is_waiting() { + if request_store + .read() + .ok() + .context("RwLock is poisoned")? + .is_waiting() + { break; } @@ -139,7 +148,7 @@ impl Svg { ), zoom_level: tiling::zoom_to_level(tile_instructions.tiling.zoom), bleed: 2, - texture: decoded_image.into_texture(), + texture: decoded_image.into_texture()?, }; tiles.push(tile.clone()); @@ -157,8 +166,12 @@ impl Svg { } } - pub fn request(&self, request: TileRequest, _abstract_decoder: &Decoder) -> anyhow::Result<()> { - let mut current_request = self.current_request.write().unwrap(); + pub fn request(&self, request: TileRequest) -> anyhow::Result<()> { + let mut current_request = self + .current_request + .write() + .ok() + .context("RwLock is poisoned")?; *current_request = Request::Tile(request); self.thread_handle.thread().unpark(); @@ -200,7 +213,7 @@ struct Decoded { } impl Decoded { - pub fn into_texture(self) -> gdk::Texture { + pub fn into_texture(self) -> anyhow::Result { let memory_format = { #[cfg(target_endian = "little")] { @@ -217,8 +230,13 @@ impl Decoded { let height = self.surface.height(); let stride = self.surface.stride() as usize; - let bytes = glib::Bytes::from_owned(self.surface.take_data().unwrap().to_vec()); + let bytes = glib::Bytes::from_owned( + self.surface + .take_data() + .context("Cairo surface already taken")? + .to_vec(), + ); - gdk::MemoryTexture::new(width, height, memory_format, &bytes, stride).upcast() + Ok(gdk::MemoryTexture::new(width, height, memory_format, &bytes, stride).upcast()) } } diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index 82fa480..8ac148d 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -77,6 +77,7 @@ impl UpdateSender { #[derive(Debug)] pub struct Decoder { decoder: FormatDecoder, + update_sender: UpdateSender, } #[derive(Debug)] @@ -100,11 +101,23 @@ impl Decoder { let update_sender = UpdateSender { sender }; tiles.set_update_sender(update_sender.clone()); - let decoder = gio::spawn_blocking(move || Self::format_decoder(update_sender, file, tiles)) - .await - .map_err(|_| anyhow!("Constructing the FormatDecoder failed unexpectedly"))??; + let decoder = gio::spawn_blocking( + glib::clone!(@strong update_sender => move || Self::format_decoder( + update_sender, + file, + tiles + )), + ) + .await + .map_err(|_| anyhow!("Constructing the FormatDecoder failed unexpectedly"))??; - Ok((Self { decoder }, receiver)) + Ok(( + Self { + decoder, + update_sender, + }, + receiver, + )) } fn format_decoder( @@ -171,7 +184,11 @@ impl Decoder { /// Request missing tiles pub fn request(&self, tile_request: TileRequest) { match &self.decoder { - FormatDecoder::Svg(svg) => svg.request(tile_request, self).unwrap(), + FormatDecoder::Svg(svg) => { + if let Err(err) = svg.request(tile_request) { + self.update_sender.send(DecoderUpdate::Error(err)); + } + } FormatDecoder::Heif(_) => {} _ => {} };