API Qs/feedback

[dhardy]

Hey @RazrFalcon, just looking over your API for use in KAS-text and I have a few comments.

I notice that Face is a wrapper around ttf-parser::Face, which KAS-text already has an instance of. I can supply the data pointer and face index no problem, but I could also directly supply a &'a Face (or possibly even just store the rustybuzz::Face the embedded ttf-parser::Face could still be accessed). But this isn't really important.

More significant, KAS-text stores text-runs with a FontId (index in internal list) plus a size (DPU aka pixels per font unit). I see your Face object embeds the size. For my purposes would storing a single Face and adjusting the size before each shaping run be the best option?

Your GlyphBuffer reports codepoint and cluster. KAS-text needs the text-index of each glyph (required for editing support), and trying to reconstruct this from codepoints is not ideal.

I see you use the same UnicodeBuffer ←→ GlyphBuffer model as HarfBuzz. I'm not sure how best to cache this type since it is consumed by value in the shape method. Maybe it would be better if UnicodeBuffer and GlyphBuffer were wrappers around Box<Buffer> to avoid large copies? Or does the optimiser avoid this issue anyway?

[RazrFalcon]

Sounds good. I will add from_face method.

Whoops, I forgot to mention in the readme that unlike hb, rb doesn't support font size. You should scale the produced offsets and advances manually.

Your GlyphBuffer reports codepoint and cluster.

Same as harfbuzz. There are no changes here.

Or does the optimiser avoid this issue anyway?

UnicodeBuffer and GlyphBuffer are just safe wrappers around internal Buffer. There is not copying involved. You can store/cache any of them.

[dhardy]

You should scale the produced offsets and advances manually.

So it's not necessary to set font size in the Face object at all, if not using variation axes?

Same as harfbuzz.

Ach ja. Those names are confusing IMO. From KAS-text's HarfBuzz support:

        let index = idx_offset + info.cluster;
        assert!(info.codepoint <= u16::MAX as u32, "failed to map glyph id");
        let id = GlyphId(info.codepoint as u16);

The Buffer object is rather large. Put the below in godbolt.org (sorry, share feature isn't working!), and its clear there is a copy (compare different sizes of T):

type T = [u32; 32];
pub struct A(T);
pub struct B(T);
pub fn f(a: A) -> B {
    B(a.0)
}

[RazrFalcon]

So it's not necessary to set font size in the Face object at all, if not using variation axes?

Yes. And even with variation it's not necessary, because variation doesn't affect size, technically speaking.

Those names are confusing IMO.

Yes. codepoint == glyph after shaping.

The Buffer object is rather large.

It is, but it never copied, or rather cloned, but moved.

[dhardy]

You know that CPUs don't have a concept of moves, right? This is a Rust concept (avoiding the need to destroy one object and construct another, especially so that String and Vec don't need to reallocate). When you move a String, the 24 bytes of the String object (capacity+length+pointer) are copied, though the allocated text buffer isn't.

[RazrFalcon]

Sure, and? This is how Rust works. If you what, you can box buffers, but it would not affect the performance in any way.

[dhardy]

One would have to box the buffers inside GlyphBuffer / UnicodeBuffer. But whatever, it's not that important.

[dhardy]

Other:

• Face::from_slice returns an Option; it would be preferable IMO if it would forward the ttf-parser error

[RazrFalcon]

Yes. This are some leftovers from the old version. But the are no interesting error in ttf-parser either, since it simply skips malformed tables.

[dhardy]

Well, implementing this in KAS-text wasn't hard (mostly copy+paste from HarfBuzz code): #23

[RazrFalcon]

Nice!

[AldaronLau]

Sounds good. I will add from_face method.

For text renderers, it would be useful if rather than this method taking an owned ttf_parser::Face, it took a reference to a ttf_parser::Face. Otherwise, to do text rendering, you would still need to load the font twice because you wouldn't have access to ttf_parser::Face after calling from_face.

If that was already the plan, I think from implies movement of ownership and would suggest naming the method with_face() or simply new() instead. An alternative solution would be an additional face() method to get a reference to the ttf_parser::Face out from the rustybuzz::Face struct.

Awesome library, by the way!

[RazrFalcon]

@AldaronLau Yes, this is a common request for ttf-parser, there is even owned_ttf_parser. But the problem is that ttf-parser designed to work on a borrowed data. It doesn't do heap allocations at all.

On the other hand, ttf-parser::Face creation is basically free compared to the shaping process itself. So it shouldn't be a bottleneck.

[AldaronLau]

@RazrFalcon Thanks for the clarification. I didn't know it was basically free to create one.

[RazrFalcon]

@AldaronLau Judging by ttf-parser benchmarks, a Face creation is roughly the same as outlining of a single glyph.

[laurmaedje]

It should be noted that in contrast to ttf-parser's Face, rustybuzz's Face is a bit heaver because the parsed GSUB and GPOS tables contain heap-allocated vectors. So it probably makes sense to parse these only once. I think it's reasonable to have both rustybuzz::Face::from_face(face: ttf_parser::Face) -> Self and rustybuzz::Face::face(&self) -> &ttf_parser::Face.

[RazrFalcon]

@laurmaedje Oh, I forgot about it. I thought it allocated only before the shaping itself and not on font creation.

[dhardy]

In any case it sounds like it's not a problem to have both a ttf_parser::Face and a rustybuzz::Face.

[AldaronLau]

It should be noted that in contrast to ttf-parser's Face, rustybuzz's Face is a bit heaver because the parsed GSUB and GPOS tables contain heap-allocated vectors. So it probably makes sense to parse these only once. I think it's reasonable to have both rustybuzz::Face::from_face(face: ttf_parser::Face) -> Self and rustybuzz::Face::face(&self) -> &ttf_parser::Face.

@laurmaedje Either way, the only Face creation that's happening twice is in ttf-parser, not in rustybuzz. The API you suggest would get rid of the extra ttf-parser Face creation, which @RazrFalcon has described as a cheap operation. I'm not sure if there are any downsides to this, but if you're already going to add both of those APIs are there any reasons why not do fn rustybuzz::Face::with_face(face: &'a ttf_parser::Face<'a>) -> rustybuzz::Face<'a> instead?

[RazrFalcon]

@AldaronLau Yes, I guess from_slice should be replace with with_face or from_face. There is not need for duplication.

[laurmaedje]

Either way, the only Face creation that's happening twice is in ttf-parser

@AldaronLau That's true, but in any case I wanted to point out that rustybuzz's Face is a bit heavier.

The with_face design could maybe also work. The problem I see is that you couldn't borrow the ttf_parser Face mutably anymore (which is needed for setting font variations and also currently used in rustybuzz::Face::set_variations). In contrast, the from_face design could also have a face_mut accessor. Then we wouldn't need the set_variations function anymore in rustybuzz because you can just configure the underlying ttf-parser Face.

[ear7h]

Any thoughts on implementing this through the traits From<ttf::Face>, AsRef<ttf::Face>, and AsMut<ttf::Face>?

[RazrFalcon]

I haven't looked into this yet.