-
-
Notifications
You must be signed in to change notification settings - Fork 862
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
[drape] Use built-in Freetype SDF rendering #7992
Conversation
97c83e7
to
e0cd4c7
Compare
e0cd4c7
to
7e71afa
Compare
e9e72ed
to
1ad22f7
Compare
on my desktop fonts look a little more clear/crisp, nice! |
drape/font_texture.cpp
Outdated
|
||
auto res = m_index.emplace(key, GlyphInfo(m_packer.MapTextureCoords(r), glyph.m_metrics)); | ||
ASSERT(res.second, ()); | ||
|
||
m_pendingNodes.emplace_back(r, std::move(glyph)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is used in GlyphIndex::UploadResources to upload rendered glyphs to the texture. Without this call all SDF glyph rendering is useless )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean all m_pendingNodes routine.
It was needed with previous logic when glyph rendering was delayed (pending).
You did immediate glyph rendering on creation.
So the next logical step is to remove m_pendingNodes at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vng that's not feasible at the moment. Glyphs are uploaded from another DrapeFronend thread which uses stored graphics context to upload data to textures. That context is not available here in this thread. So m_pendingNodes is just a queue to pass glyphs to another thread.
1ad22f7
to
5d21f0e
Compare
FREETYPE_CHECK(FT_Load_Glyph(m_fontFace, FT_Get_Char_Index(m_fontFace, unicodePoint), FT_LOAD_RENDER)); | ||
FREETYPE_CHECK(FT_Render_Glyph(m_fontFace->glyph, FT_RENDER_MODE_SDF)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, we had delayed async glyph rendering logic before. Not to hang the rendering routine.
Now it is rendered in place. Did you visually check how it works? Especially on low-end Androids :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explicitly mentioned it in the commit and in the PR description. I have tested it on low-end Androids, and didn't see a negative difference (there the most time is taken by slow feature reads and rendering of roads/areas, texts appear almost immediately).
A positive difference is that texts do not blink now as before.
The current performance is comparable to the previous threaded approach, because:
- There is no threading overhead to pass glyphs in many smaller chunks to 3 different glyph rendering threads.
- All symbols are now uploaded in a few larger batches by the frontend thread, instead of uploading a few symbols per each rendered frame.
- Freetype's SDF implementation is faster than the removed one and provides a better visual quality result.
IMO the current state is good enough to release, but it's better to beta-test it first, of course.
The next step will introduce additional HB shaping before rendering glyphs. That step may create a sensible delay in text rendering, in this case, we may need to shape/render different strings on different threads. That is another story, I have some ideas on how to do it cleaner, via a (system) thread pool with callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should test on low-end devices.
256eeb8
to
f492153
Compare
* Set the same SDF spread/border (4) as it was before * Removed the threaded glyph generator, SDF glyphs are created now on a single thread. Before, the bitmap was rendered and then copied on the same single thread. By removing unnecessary overhead and by using bsdf renderer which is faster than sdf, visually glyphs appear on the screen at comparable or even faster time. * Removed GetGlyphSdfScale, it will be hardcoded or set in a different way if necessary * Fixed some minor tidy warnings Signed-off-by: Alexander Borsuk <me@alex.bio>
Signed-off-by: Alexander Borsuk <me@alex.bio>
Signed-off-by: Alexander Borsuk <me@alex.bio>
f492153
to
8efdebb
Compare
g
letter) on Android 9 that should be investigated.Read more about SDF fonts here.
This PR is a step towards #4281
Any help with Android and iOS testing is appreciated.