Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up SKTextBlobBuilder and SKRunBuffer APIs #2775

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Mar 2, 2024

Description of Change

This is sort of inspired by #2664 but also the SKRunBuffer API is a bit weird. In 2.x the skia comments say the text part is not meant to be sued, but it is actually required in some cases.

I had cleaned up the API a bit before, but I see I still needed some work.

Then #2664 was asking for less allocations, so I am thinking instead of spans, we just return the struct directly (almost) and then it is up to the caller to do the right thing.

Copy link
Contributor Author

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to maybe just replace entirely with structs, but that will break everything. The new overloads are less break-y, but are the old ones actually useful?

I see some code out there already that does use the classes, so that will all have to be migrated.

But is this overload bad? Is it good? Should we rename the method and return the struct directly instead of using out?

binding/SkiaSharp/SKTextBlob.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see this merged!

@Gillibald
Copy link
Contributor

Gillibald commented Mar 2, 2024

I am fine with breaking changes in this area as long as it improves things. This is a hot path for Avalonia. Less allocation/copies is always better. Being able to work with the buffer directly is preferred.

@Gillibald
Copy link
Contributor

Gillibald commented Mar 5, 2024

This is our current usage: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Skia/Avalonia.Skia/GlyphRunImpl.cs#L117

We share glyph indices and glyph positions for different edging modes. Ideally we could replace the buffer spans with our data. Sadly run allocation is tied to a specific SkFont.

@mattleibow
Copy link
Contributor Author

mattleibow commented Mar 10, 2024

Thinking next to the box here... Google has decided that the api needs a font. SkiaSharp just copied and unfortunately we have a bit of overhead...

However, typically what do you need from a font? If it is things like typeface and text size, maybe we can create an overload for this. Might be a basic/internal struct that I pass into the pinvoke.

Maybe I can revalidate that the font needs to go through all the registration logic. It may not.

However, what is the main issue? The allocations of an object just to pass it to the allocation of a run? If that was gone, would it be better?

I control the api for the most part and typically just add bindings as people ask. Once there are numbers with perf, we can add overloads or improve. This is v3 after all, 😅

@Gillibald
Copy link
Contributor

Gillibald commented Mar 10, 2024

This is mainly a limitation of Skia's API. Theoretically the SKFont is only neded when the SKTextBlob is build.

Avalonia supports different edging modes for text and edging is backed into the SKTextBlob when it is created you can't draw one blob with different edging.

In real world applications you always shape text into runs of glyphs and this data needs to be stored somewhere. We can't use a SKRunBuffer because of above mentioned reasons.

So all we can do is exposing easy excess to positions and glyphs spans. So a copy is as fast as possible.

Avoiding a copy is not possible. At least not for Avalonia.

@mattleibow
Copy link
Contributor Author

I am getting close to merging a few PRs, is this new API going to meet your expectations? Is the shape good and are you able to confirm there are no/minimal allocations?

@Gillibald
Copy link
Contributor

Y API looks good 👍

@mattleibow mattleibow merged commit 99a2607 into main Mar 28, 2024
69 of 72 checks passed
@mattleibow mattleibow deleted the dev/textblobbuilder branch March 28, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide allocation-free overloads of methods in SKTextBlobBuilder
3 participants