Serialization Proposal 1#52
Conversation
|
Thanks for the PR, I agree that the storage of the glyphs should be optimized to save space. But I would prefer not to implement manual serialization and deserialization if possible. My idea was to store the glyphs as a struct of arrays instead of an array of struct BdfFont<'a, Coord, Size, Index> {
top_left: &'a [Coord],
size: &'a [Size],
device_width: &'a [Size],
data_index: &'a [Index],
}
const TOP_LEFT: [i8; 256 * 2] = ...;
const SIZE: [u8; 256 * 2] = ...;
const WIDTH: [i8; 256] = ...;
const DATA_INDEX: [u16; 256] = ...;
pub const MY_FONT: BdfFont<'static, i8, u8, u16> = {
.top_left = &TOP_LEFT,
.size = &SIZE,
.width = &WIDTH,
.data_index = &DATA_INDEX,
};The advantage is that no manual serialization is necessary and that each font can use the minimum data size required, because of the type parameters in ´BdfFont But my proposal only works for fonts embedded in the source code, a serialization format would still be useful for other applications which load fonts at runtime from some external storage. If you want to continue with this PR I would suggest to look into using a serialization format like https://github.com/jamesmunns/postcard. |
|
Representing fonts in source code would make it harder to use those fonts in other languages. For example, generating custom fonts for u8g2_fonts requires compiling C source code since that's the only format u8g2's tooling generates. Using postcard, would it be possible to index serialized data without de-serializing it? For a large font, creating a whole copy of it could use up the memory of a machine. |
|
Although the serialization in this PR tries to replicate the As of now, finding a glyph is O(n), and there are 17 bytes of metadata per glyph. This makes rendering slow for CJK characters and emojis, and means that the metadata of a font can get larger than the bitmap data. Sorting the list of glyphs would allow for a binary search, but storing ranges of contiguous sections of glyphs would make the search functionally O(1), since fonts usually group glyphs together in large blocks This would also remove the need to denote the corresponding character of each glyph, making the character struct 13 bytes per glyph, potentially saving up to 200kb for a font like unifont I propose: |
|
This recent commit refactored the |
| primitives::Rectangle, | ||
| }; | ||
|
|
||
| /// * Header (12 Bytes): |
There was a problem hiding this comment.
In case this format is also used to load a font at runtime it would be a good idea to add some validation that it is a valid font file. A magic value, a version, and the file size should be enough.
You are correct that this would be an issue for a variable integer size format like postcard. It would need to be a hybrid approach, where at least the glyph index table would use fixed integer sizes for more efficient lookup. I'm OK with leaving the manual serialization/deserialization.
Using binary search for glpyh lookup was always the idea for the non serialized version. I would keep the non grouped version for this PR to keep things simpler. |
Co-authored-by: Ralf Fuest <mail@rfuest.de>
Co-authored-by: Ralf Fuest <mail@rfuest.de>
|
Suggestions applied, I will push another commit to resolve the errors shortly |
|
It is not possible to generate a reference to the bitmap data for each glyph using the function |
My comment mentioned this, but I didn't explain this very well. The |
|
start_index indexes the bit the glyph data starts at, not the byte, which makes harder to generate a slice from |
|
Padding each glyphs data to a byte boundary seems to work |
Sorry I forgot that the pixel data of the glyphs was tightly packed without any padding. Starting each glyph at a byte boundary seems like a good solution. |
| /// The raw u8 data of the serialized font | ||
| pub data: &'a [u8], | ||
| } | ||
| impl<'a> SerializedBdfFont<'a> { |
There was a problem hiding this comment.
The deserialization code contains too many magical values and it shouldn't panic in case of malformed or truncated input data.
As a first step I would add some constants and getters, like:
const HEADER_SIZE: usize = 12;
const GLYPH_SIZE: usize = 17;
impl<'a> SerializedBdfFont<'a> {
...
fn bitmap_data(&self) -> &[u8] {
&self.data[HEADER_SIZE + self.character_count() * GLYPH_SIZE..]
}
fn glyph_data(&self, index: usize) -> Option<&[u8; GLYPH_SIZE]> {
let offset = HEADER_SIZE + index * GLYPH_SIZE;
self.data
.get(offset..offset + GLYPH_SIZE)
.map(|slice| slice.try_into().unwrap())
}
fn character_table(&self, index: usize) -> Option<DisplayBdfGlyph<'_>> {
let glyph = self.glyph_data(index)?;
let corresponding_character = char::from_u32(u32::from_be_bytes(glyph[0..4].try_into().unwrap()))?;
let top_left_x = i16::from_be_bytes(glyph[4..6].try_into().unwrap());
...
}
...
}|
Verification can be done using the New Type pattern, which would make checking data easier /// Constructs a serialized font without first verifing the data
pub const fn from_unverified_data(data: &'a [u8]) -> Self {
Self { data }
}
/// Verifies data in a way that prevents panics and returns a serialized font if the data is valid
///
/// TODO: Make this const
pub fn verify_data(data: &'a [u8]) -> Option<Self> { |
|
Commit 5c1c7d6 |
| /// Constructs a serialized font without first verifing the data | ||
| pub const fn from_unverified_data(data: &'a [u8]) -> Self { | ||
| Self { data } | ||
| } |
There was a problem hiding this comment.
I would prefer not to have a constructor that doesn't check the data. It should be possible to make the other constructor const and provide two variants: one which returns a Result and one which panics to make it easier to define font constants.
There was a problem hiding this comment.
The verification process is noticeably slower than constructing it from unverified data. Even if the verification function was const, it would slow down loading fonts from non-const values (like from external storage).
The consequence of loading corrupted font data would just be a panic, there isn't any memory errors that could propagate to other parts of the program silently.
| /// Verifies data in a way that prevents panics and returns a serialized font if the data is valid | ||
| /// | ||
| /// TODO: Make this const | ||
| pub fn verify_data(data: &'a [u8]) -> Result<Self, &'static str> { |
There was a problem hiding this comment.
This is a constructor and should be named accordingly:
| pub fn verify_data(data: &'a [u8]) -> Result<Self, &'static str> { | |
| pub fn new(data: &'a [u8]) -> Result<Self, &'static str> { |
| for i in 0..font.character_count() { | ||
| let offset = Self::HEADER_SIZE + (i * Self::CHARACTER_TABLE_ENTRY_SIZE) as usize; | ||
| // Corresponding Character Invalid | ||
| char::from_u32(font.get_be_u32(offset + Self::CODEPOINT_OFFSET)) | ||
| .ok_or("Invalid character")?; | ||
|
|
||
| // Data index not within bitmap data | ||
| let idx = font.get_be_u32(offset + Self::IDX_OFFSET) as usize; | ||
|
|
||
| if idx > data.len() { | ||
| return Err("Invalid bitmap index"); | ||
| } | ||
| } |
There was a problem hiding this comment.
It should be possible to make this part work in a const fn. I would add a helper function to get the glyph data, which could also be reused in character_table. Something like:
const fn glyph_data(&self, index: usize) -> &[u8; CHARACTER_TABLE_ENTRY_SIZE] {
let (_header, data) = self
.data
.split_at(Self::HEADER_SIZE + index * CHARACTER_TABLE_ENTRY_SIZE);
data.first_chunk().unwrap()
}The get_be_... functions can be moved outside the struct impl to take data as a parameter, which makes them more flexible.
| descent: self.get_be_u16(Self::DESCENT_OFFSET) as u32, | ||
| line_height: self.get_be_u16(Self::ASCENT_OFFSET) as u32 | ||
| + self.get_be_u16(Self::DESCENT_OFFSET) as u32, | ||
| } |
There was a problem hiding this comment.
See note about as vs from above.
Co-authored-by: Ralf Fuest <mail@rfuest.de>
Co-authored-by: Ralf Fuest <mail@rfuest.de>
These additions provide a way to represent and render a parsed BDF font as a slice of
u8s, and tooling for that conversion in the font converter and previewer.The format for serialization attempts to be as close to the
BdfFontstruct as possible while saving space, using 17 bytes per character: