From 5d1401ef4ffb811b3f6d311e0824d5ea922bbc6f Mon Sep 17 00:00:00 2001 From: obrusvit Date: Mon, 9 Dec 2024 19:39:26 +0100 Subject: [PATCH] refactor(core): safe iface for get_glyph_data [no changelog] --- core/embed/gfx/fonts/fonts.h | 3 +- core/embed/rust/src/translations/blob.rs | 16 +- core/embed/rust/src/translations/mod.rs | 25 +- core/embed/rust/src/ui/display/font.rs | 343 ++++++++++++++--------- core/embed/rust/src/ui/shape/text.rs | 36 +-- 5 files changed, 250 insertions(+), 173 deletions(-) diff --git a/core/embed/gfx/fonts/fonts.h b/core/embed/gfx/fonts/fonts.h index 4e3aced1ad..37eff857a2 100644 --- a/core/embed/gfx/fonts/fonts.h +++ b/core/embed/gfx/fonts/fonts.h @@ -38,7 +38,8 @@ typedef struct { const uint8_t *glyph_nonprintable; } font_info_t; -/// Font identifiers +/// Font identifiers. Keep in sync with `enum font` definition in +/// `core/embed/rust/src/ui/display/font.rs`. typedef enum { FONT_NORMAL = -1, FONT_BOLD = -2, diff --git a/core/embed/rust/src/translations/blob.rs b/core/embed/rust/src/translations/blob.rs index 422259359c..03ed46b92b 100644 --- a/core/embed/rust/src/translations/blob.rs +++ b/core/embed/rust/src/translations/blob.rs @@ -240,7 +240,7 @@ impl<'a> Translations<'a> { /// translations object. This is to facilitate safe interface to /// flash-based translations. See docs for `flash::get` for details. #[allow(clippy::needless_lifetimes)] - pub fn font<'b>(&'b self, index: u16) -> Option> { + fn font<'b>(&'b self, index: u16) -> Option> { self.fonts .get(index) .and_then(|data| Table::new(InputStream::new(data)).ok()) @@ -258,6 +258,20 @@ impl<'a> Translations<'a> { pub fn header<'b>(&'b self) -> &'b TranslationsHeader<'b> { &self.header } + + /// Returns a byte slice of the glyph data for the given UTF-8 codepoint in + /// the specified font. + /// + /// SAFETY: Do not mess with the lifetimes in this signature. + /// + /// The lifetimes are a useful lie that bind the lifetime of the returned + /// string not to the underlying data, but to the _reference_ to the + /// translations object. This is to facilitate safe interface to + /// flash-based translations. See docs for `flash::get` for details. + #[allow(clippy::needless_lifetimes)] + pub fn get_utf8_glyph<'b>(&'b self, codepoint: u16, font_index: u16) -> Option<&'b [u8]> { + self.font(font_index).and_then(|t| t.get(codepoint)) + } } pub struct TranslationsHeader<'a> { diff --git a/core/embed/rust/src/translations/mod.rs b/core/embed/rust/src/translations/mod.rs index 17b9544cd7..0be1e46e41 100644 --- a/core/embed/rust/src/translations/mod.rs +++ b/core/embed/rust/src/translations/mod.rs @@ -1,33 +1,12 @@ mod blob; -mod flash; +pub mod flash; mod generated; #[cfg(feature = "micropython")] mod obj; mod public_keys; mod translated_string; +pub use blob::Translations; pub use translated_string::TranslatedString as TR; -use crate::ui::display::Font; pub const DEFAULT_LANGUAGE: &str = "en-US"; - -/// # Safety -/// -/// Returned pointer will only point to valid font data for as long as -/// the flash content is not invalidated by `erase()` or `write()`. -pub unsafe fn get_utf8_glyph(codepoint: u16, font: Font) -> *const u8 { - // SAFETY: Reference is discarded at the end of the function. - // We do return a _pointer_ to the same memory location, but the pointer is - // always valid. - let Ok(translations) = flash::get() else { - return core::ptr::null(); - }; - let Some(tr) = translations.as_ref() else { - return core::ptr::null(); - }; - if let Some(glyph) = tr.font(font as u16).and_then(|t| t.get(codepoint)) { - glyph.as_ptr() - } else { - core::ptr::null() - } -} diff --git a/core/embed/rust/src/ui/display/font.rs b/core/embed/rust/src/ui/display/font.rs index 0e55786c9b..ffe085269d 100644 --- a/core/embed/rust/src/ui/display/font.rs +++ b/core/embed/rust/src/ui/display/font.rs @@ -1,5 +1,7 @@ +use spin::RwLockReadGuard; + use crate::{ - trezorhal::display, + trezorhal::display::{self}, ui::{ constant, geometry::Offset, @@ -9,12 +11,14 @@ use crate::{ use core::slice; #[cfg(feature = "translations")] -use crate::translations::get_utf8_glyph; +use crate::translations::flash; +#[cfg(feature = "translations")] +use crate::translations::Translations; /// Representation of a single glyph. /// We use standard typographic terms. For a nice explanation, see, e.g., /// the FreeType docs at https://www.freetype.org/freetype2/docs/glyphs/glyphs-3.html -pub struct Glyph { +pub struct Glyph<'a> { /// Total width of the glyph itself pub width: i16, /// Total height of the glyph itself @@ -25,40 +29,32 @@ pub struct Glyph { pub bearing_x: i16, /// Top-side vertical bearing pub bearing_y: i16, - data: &'static [u8], + data: &'a [u8], } -impl Glyph { - /// Construct a `Glyph` from a raw pointer. +impl<'a> Glyph<'a> { + /// Creates a new `Glyph` from a byte slice containing font data. /// - /// # Safety - /// - /// This function is unsafe because the caller has to guarantee that `data` - /// is pointing to a memory containing a valid glyph data, that is: - /// - contains valid glyph metadata - /// - data has appropriate size - /// - data must have static lifetime - pub unsafe fn load(data: *const u8) -> Self { - unsafe { - let width = *data.offset(0) as i16; - let height = *data.offset(1) as i16; + /// Expected data format (bytes): + /// - 0: glyph width + /// - 1: glyph height + /// - 2: advance width + /// - 3: x-bearing + /// - 4: y-bearing + /// - 5...: bitmap data, packed according to FONT_BPP (bits per pixel) + pub fn load(data: &'a [u8]) -> Self { + let width = data[0] as i16; + let height = data[1] as i16; - let data_bytes = match constant::FONT_BPP { - 1 => (width * height + 7) / 8, // packed bits - 2 => (width * height + 3) / 4, // packed bits - 4 => (width + 1) / 2 * height, // row aligned to bytes - 8 => width * height, - _ => fatal_error!("Unsupported font bpp"), - }; - - Glyph { - width, - height, - adv: *data.offset(2) as i16, - bearing_x: *data.offset(3) as i16, - bearing_y: *data.offset(4) as i16, - data: slice::from_raw_parts(data.offset(5), data_bytes as usize), - } + let size = calculate_glyph_size(data); + ensure!(data.len() == size, "Invalid glyph data size"); + Glyph { + width, + height, + adv: data[2] as i16, + bearing_x: data[3] as i16, + bearing_y: data[4] as i16, + data: &data[5..], } } @@ -88,7 +84,7 @@ impl Glyph { c_data >> 4 } - pub fn bitmap(&self) -> Bitmap<'static> { + pub fn bitmap(&self) -> Bitmap<'a> { match constant::FONT_BPP { 1 => unwrap!(Bitmap::new( BitmapFormat::MONO1P, @@ -109,6 +105,114 @@ impl Glyph { } } +/// A provider of font glyphs and their metadata. +/// +/// Manages access to font resources and handles UTF-8 character glyphs +/// +/// The provider holds necessary lock for accessing translation data +/// and is typically used through the `Font::with_glyph_data` method +/// to ensure proper resource cleanup. +/// +/// # Example +/// ``` +/// let font = Font::NORMAL; +/// font.with_glyph_data(|data| { +/// let glyph = data.get_glyph('A'); +/// // use glyph... +/// }); +/// ``` +pub struct GlyphData { + font: Font, + #[cfg(feature = "translations")] + translations_guard: Option>>>, +} + +impl GlyphData { + fn new(font: Font) -> Self { + #[cfg(feature = "translations")] + let translations_guard = flash::get().ok(); + + Self { + font, + #[cfg(feature = "translations")] + translations_guard, + } + } + + pub fn get_glyph(&self, ch: char) -> Glyph<'_> { + let ch = match ch { + '\u{00a0}' => '\u{0020}', + c => c, + }; + let gl_data = self.get_glyph_data(ch as u16); + + Glyph::load(unwrap!(gl_data, "Failed to load glyph")) + } + + fn get_glyph_data(&self, codepoint: u16) -> Option<&[u8]> { + display::get_font_info(self.font.into()).map(|font_info| { + if codepoint >= ' ' as u16 && codepoint < 0x7F { + // ASCII character + let offset = codepoint - ' ' as u16; + unsafe { + let ptr = *font_info.glyph_data.offset(offset as isize); + self.load_glyph_from_ptr(ptr) + } + } else { + #[cfg(feature = "translations")] + { + if codepoint >= 0x7F { + // UTF8 character from embedded blob + if let Some(glyph) = self + .translations_guard + .as_ref() + .and_then(|guard| guard.as_ref()) + .and_then(|translations| { + translations.get_utf8_glyph(codepoint, self.font as u16) + }) + { + return glyph; + } + } + } + self.glyph_nonprintable() + } + }) + } + + /// Returns glyph data slice from a raw pointer by reading the header and + /// calculating full size. + unsafe fn load_glyph_from_ptr(&self, ptr: *const u8) -> &[u8] { + unsafe { + let header = slice::from_raw_parts(ptr, 2); + let full_size = calculate_glyph_size(header); + slice::from_raw_parts(ptr, full_size) + } + } + + /// Returns glyph data slize for non-printable characters. + fn glyph_nonprintable(&self) -> &[u8] { + display::get_font_info(self.font.into()) + .map(|font_info| unsafe { self.load_glyph_from_ptr(font_info.glyph_nonprintable) }) + .unwrap() + } +} + +fn calculate_glyph_size(header: &[u8]) -> usize { + let width = header[0] as i16; + let height = header[1] as i16; + + let data_bytes = match constant::FONT_BPP { + 1 => (width * height + 7) / 8, // packed bits + 2 => (width * height + 3) / 4, // packed bits + 4 => (width + 1) / 2 * height, // row aligned to bytes + 8 => width * height, + _ => fatal_error!("Unsupported font bpp"), + }; + + 5 + data_bytes as usize // header (5 bytes) + bitmap data +} + /// Font constants. Keep in sync with `font_id_t` definition in /// `core/embed/gfx/fonts/fonts.h`. #[derive(Copy, Clone, PartialEq, Eq, FromPrimitive)] @@ -134,17 +238,12 @@ impl From for i32 { impl Font { /// Supports UTF8 characters pub fn text_width(self, text: &str) -> i16 { - text.chars().fold(0, |acc, c| acc + self.char_width(c)) - } - - /// Supports UTF8 characters - fn get_first_glyph_from_text(self, text: &str) -> Option { - text.chars().next().map(|c| self.get_glyph(c)) - } - - /// Supports UTF8 characters - fn get_last_glyph_from_text(self, text: &str) -> Option { - text.chars().next_back().map(|c| self.get_glyph(c)) + self.with_glyph_data(|data| { + text.chars().fold(0, |acc, c| { + let char_width = data.get_glyph(c).adv; + acc + char_width + }) + }) } /// Width of the text that is visible. @@ -155,17 +254,17 @@ impl Font { return 0; } - let first_char_bearing = if let Some(glyph) = self.get_first_glyph_from_text(text) { - glyph.bearing_x - } else { - 0 - }; - - let last_char_bearing = if let Some(glyph) = self.get_last_glyph_from_text(text) { - glyph.right_side_bearing() - } else { - 0 - }; + let (first_char_bearing, last_char_bearing) = self.with_glyph_data(|data| { + let first = text + .chars() + .next() + .map_or(0, |c| data.get_glyph(c).bearing_x); + let last = text + .chars() + .next_back() + .map_or(0, |c| data.get_glyph(c).right_side_bearing()); + (first, last) + }); // Strip leftmost and rightmost spaces/bearings/margins. self.text_width(text) - first_char_bearing - last_char_bearing @@ -178,11 +277,13 @@ impl Font { /// the glyphs representing the characters in the provided text. pub fn visible_text_height(self, text: &str) -> i16 { let (mut ascent, mut descent) = (0, 0); - for c in text.chars() { - let glyph = self.get_glyph(c); - ascent = ascent.max(glyph.bearing_y); - descent = descent.max(glyph.height - glyph.bearing_y); - } + self.with_glyph_data(|data| { + for c in text.chars() { + let glyph = data.get_glyph(c); + ascent = ascent.max(glyph.bearing_y); + descent = descent.max(glyph.height - glyph.bearing_y); + } + }); ascent + descent } @@ -202,26 +303,25 @@ impl Font { return 0; } - self.get_first_glyph_from_text(text) - .map_or(0, |glyph| glyph.bearing_x) + text.chars().next().map_or(0, |c| { + self.with_glyph_data(|data| data.get_glyph(c).bearing_x) + }) } pub fn char_width(self, ch: char) -> i16 { - self.get_glyph(ch).adv + self.with_glyph_data(|data| data.get_glyph(ch).adv) } pub fn text_height(self) -> i16 { - display::get_font_info(self.into()).map_or(i16::MAX, |font| font.height.try_into().unwrap()) + unwrap!(display::get_font_info(self.into())).height as i16 } pub fn text_max_height(self) -> i16 { - display::get_font_info(self.into()) - .map_or(i16::MAX, |font| font.max_height.try_into().unwrap()) + unwrap!(display::get_font_info(self.into())).max_height as i16 } pub fn text_baseline(self) -> i16 { - display::get_font_info(self.into()) - .map_or(i16::MAX, |font| font.baseline.try_into().unwrap()) + unwrap!(display::get_font_info(self.into())).baseline as i16 } pub fn line_height(self) -> i16 { @@ -247,41 +347,16 @@ impl Font { (start + end + self.visible_text_height(text)) / 2 } - fn get_glyph_data(&self, codepoint: u16) -> *const u8 { - display::get_font_info((*self).into()).map_or(core::ptr::null(), |font_info| { - #[cfg(feature = "translations")] - { - if codepoint >= 0x7F { - // UTF8 character from embedded blob - return unsafe { get_utf8_glyph(codepoint, *self) }; - } - } - - if codepoint >= ' ' as u16 && codepoint < 0x7F { - // ASCII character - unsafe { - *font_info - .glyph_data - .offset((codepoint - ' ' as u16) as isize) - } - } else { - font_info.glyph_nonprintable - } - }) - } - - pub fn get_glyph(self, ch: char) -> Glyph { - /* have the non-breaking space counted for width but not counted as a - * breaking point */ - let ch = match ch { - '\u{00a0}' => '\u{0020}', - c => c, - }; - let gl_data = self.get_glyph_data(ch as u16); - - ensure!(!gl_data.is_null(), "Failed to load glyph"); - // SAFETY: Glyph::load is valid for data returned by get_char_glyph - unsafe { Glyph::load(gl_data) } + /// Safely manages temporary access to glyph data without risking + /// translation lock deadlocks. See `GlyphData` for more details. + pub fn with_glyph_data(&self, f: F) -> T + where + F: FnOnce(&GlyphData) -> T, + { + // Create a new GlyphData instance that will be dropped at the end of this + // function, releasing any translations lock + let glyph_data = GlyphData::new(*self); + f(&glyph_data) } /// Get the longest prefix of a given `text` (breaking at word boundaries) @@ -289,45 +364,51 @@ impl Font { pub fn longest_prefix(self, width: i16, text: &str) -> &str { let mut prev_word_boundary = 0; let mut text_width = 0; - for (i, c) in text.char_indices() { - let c_width = self.char_width(c); - if text_width + c_width > width { - // Another character would not fit => split at the previous word boundary - return &text[0..prev_word_boundary]; + self.with_glyph_data(|data| { + for (i, c) in text.char_indices() { + let char_width = data.get_glyph(c).adv; + let c_width = char_width; + if text_width + c_width > width { + // Another character would not fit => split at the previous word boundary + return &text[0..prev_word_boundary]; + } + if c == ' ' { + prev_word_boundary = i; + } + text_width += c_width; } - if c == ' ' { - prev_word_boundary = i; - } - text_width += c_width; - } - - text // the whole text fits + text // the whole text fits + }) } /// Get the length of the longest suffix from a given `text` /// that will fit into the area `width` pixels wide. pub fn longest_suffix(self, width: i16, text: &str) -> usize { let mut text_width = 0; - for (chars_from_right, c) in text.chars().rev().enumerate() { - let c_width = self.char_width(c); - if text_width + c_width > width { - // Another character cannot be fitted, we're done. - return chars_from_right; - } - text_width += c_width; - } - text.len() // it fits in its entirety + self.with_glyph_data(|data| { + for (chars_from_right, c) in text.chars().rev().enumerate() { + let char_width = data.get_glyph(c).adv; + if text_width + char_width > width { + // Another character cannot be fitted, we're done. + return chars_from_right; + } + text_width += char_width; + } + text.len() // it fits in its entirety + }) } pub fn visible_text_height_ex(&self, text: &str) -> (i16, i16) { let (mut ascent, mut descent) = (0, 0); - for c in text.chars() { - let glyph = self.get_glyph(c); - ascent = ascent.max(glyph.bearing_y); - descent = descent.max(glyph.height - glyph.bearing_y); - } - (ascent, descent) + self.with_glyph_data(|data| { + for c in text.chars() { + let glyph = data.get_glyph(c); + ascent = ascent.max(glyph.bearing_y); + descent = descent.max(glyph.height - glyph.bearing_y); + } + (ascent, descent) + }) } } diff --git a/core/embed/rust/src/ui/shape/text.rs b/core/embed/rust/src/ui/shape/text.rs index 130f935feb..7707920f02 100644 --- a/core/embed/rust/src/ui/shape/text.rs +++ b/core/embed/rust/src/ui/shape/text.rs @@ -97,24 +97,26 @@ impl<'a> Shape<'_> for Text<'a> { // TODO: optimize text clipping, use canvas.viewport() - for ch in self.text.chars() { - if r.x0 >= r.x1 { - break; + self.font.with_glyph_data(|glyph_data| { + for ch in self.text.chars() { + if r.x0 >= r.x1 { + break; + } + + let glyph = glyph_data.get_glyph(ch); + let glyph_bitmap = glyph.bitmap(); + let glyph_view = BitmapView::new(&glyph_bitmap) + .with_alpha(self.alpha) + .with_fg(self.color) + .with_offset(Offset::new( + -glyph.bearing_x, + -(max_ascent - glyph.bearing_y), + )); + + canvas.blend_bitmap(r, glyph_view); + r.x0 += glyph.adv; } - - let glyph = self.font.get_glyph(ch); - let glyph_bitmap = glyph.bitmap(); - let glyph_view = BitmapView::new(&glyph_bitmap) - .with_alpha(self.alpha) - .with_fg(self.color) - .with_offset(Offset::new( - -glyph.bearing_x, - -(max_ascent - glyph.bearing_y), - )); - - canvas.blend_bitmap(r, glyph_view); - r.x0 += glyph.adv; - } + }); } }