Is it unsound to perform text shaping while an OutlineBuilder exists?

[LoganDark]

In order to get an OutlineBuilder, you have to do some parsing using the font's FontTableProvider. This means the OutlineBuilder borrows from the FontTableProvider, which means it borrows from the Font.

Then, if you need to shape some text, you need to... call Font::shape which takes &mut self.

This is aliasing a mutable reference with preexisting shared references.

That means the most obvious low-hanging-fruit for caching (obtaining the OutlineBuilder up-front at load time) is unsound.

I did not notice this because I have to use piles of unsafe code to store the font and related data in a pinned struct. Therefore the borrow checker is significantly inhibited. see #52

Now that I think about it, it seems like a huge API issue, as well as a performance issue. Parsing tables to get an OutlineBuilder each frame in a game for example (required not to violate the borrowing rules), sounds like a huge problem.

[wezm]

OutlineBuilder is just a trait so it has no state of it's own. It only borrows the underlying table (mutably) when visit is called. Unless I'm missing a detail?

[LoganDark]

OutlineBuilder is just a trait so it has no state of it's own. It only borrows the underlying table (mutably) when visit is called. Unless I'm missing a detail?

The actual implementations of OutlineBuilder require borrowing a bunch of data obtained from the DynamicFontTableProvider

[wezm]

Right it seems that I was misunderstanding your question. Normally you would not be able to call shape while holding an OutlineBuilder impl because the borrow checker would not allow it. However if you do manage to achieve this via unsafe are you asking if it's unsound?

If so, shaping currently mutates the gsub_cache, gpos_cache, glyph_cache, and the lazy loaded GDEF table (if it's not already loaded) on Font. So it might be ok to hold onto the OutlineBuilder but I can't guarantee this will always be the case.

Now that I think about it, it seems like a huge API issue, as well as a performance issue. Parsing tables to get an OutlineBuilder each frame in a game for example (required not to violate the borrowing rules), sounds like a huge problem.

As you pointed out in #52 the library is not currently well structured for this type of use. The API evolved for our use case which is load a bunch of fonts, shape the text, and then generate a PDF. I'm not sure if/when we'll be able to dedicate the time to remedying that.

[LoganDark]

If so, shaping currently mutates the gsub_cache, gpos_cache, glyph_cache, and the lazy loaded GDEF table (if it's not already loaded) on Font. So it might be ok to hold onto the OutlineBuilder but I can't guarantee this will always be the case.

I'm saying that it's definitely unsound because there are aliasing mutable references which is forbidden.

As you pointed out in #52 the library is not currently well structured for this type of use. The API evolved for our use case which is load a bunch of fonts, shape the text, and then generate a PDF. I'm not sure if/when we'll be able to dedicate the time to remedying that.

This is kind of sad, because allsorts is actually kind of fast and extremely featureful. I'm having no problems shaping/rasterizing text every single frame in software (the rasterizer is ab_glyph_rasterizer). It's just that the code currently doing that is invoking Slightly Undefined Behavior. :(

[wezm]

This is kind of sad, because allsorts is actually kind of fast and extremely featureful. I'm having no problems shaping/rasterizing text every single frame in software (the rasterizer is ab_glyph_rasterizer). It's just that the code currently doing that is invoking Slightly Undefined Behavior. :(

Perhaps a silly idea: what if you parsed the font twice and used one instance for the OutlineBuilder and one for shaping?

[LoganDark]

This is kind of sad, because allsorts is actually kind of fast and extremely featureful. I'm having no problems shaping/rasterizing text every single frame in software (the rasterizer is ab_glyph_rasterizer). It's just that the code currently doing that is invoking Slightly Undefined Behavior. :(

Perhaps a silly idea: what if you parsed the font twice and used one instance for the OutlineBuilder and one for shaping?

I had this idea once but I actually forgot to try it. This will probably work. In fact, I don't see any reason why it wouldn't. The data structures returned by a call to shape do not contain any references to the font, and therefore can just be... transferred. However you want.

This can be used to cause panics and crashes and unexpected behavior... or it can be used for performance.

I'm definitely going to try implementing that now, thank you!

[LoganDark]

@wezm Is it possible for Font to implement Clone so that it can share some of the reference-counted data structures? Or would that break safety somehow?

[wezm]

If the compiler allows it then there shouldn't be any safety issues.

[LoganDark]

dang, DynamicFontTableProvider and its darn Box. probably should get rid of that one day