[drape] Integrate Harfbuzz text shaping #8473

biodranik · 2024-06-16T22:25:28Z

What should be done:

Split longer lines of text into two

There are many TODOs in the code which can be optimized later. Proper profiling and caching may increase the rendering speed.

drape/glyph.hpp

drape_frontend/gui/gui_text.cpp

vng · 2024-06-17T15:55:23Z

drape_frontend/text_layout.cpp

-    delimIndexes.push_back(visibleText.size());
+  // if (visibleText == text && !forceNoWrap)
+  //   SplitText(visibleText, delimIndexes);
+  // else


What is the problem with keeping an existing SplitText logic? How does it break HB?

It does not break, just a bit more complex computation. I will implement it.

@vng can you please review newly added commits?

biodranik · 2024-06-23T13:54:57Z

Some statistics about the length and count of rendered strings in displayed data after zooming in/out in China and Switzerland (longer strings are NOT split into two):

Dumping stats:
1 13
2 516
3 4145
4 2094
5 2049
6 2140
7 2314
8 1920
9 1723
10 1429
11 1302
12 1258
13 1262
14 1086
15 1139
16 1140
17 1001
18 789
19 679
20 497
21 402
22 297
23 257
24 189
25 188
26 131
27 114
28 139
29 201
30 212
31 170
32 139
33 113
34 81
35 78
36 66
37 66
38 65
39 63
40 45
41 29
42 17
43 15
44 12
45 9
46 11
47 9
48 11
49 12
50 506
51 9
52 1
53 9
54 2
55 5
56 1
57 5
58 2
59 2
60 1
61 2
62 2
64 2
66 3
68 2
69 1
70 2
72 1
74 1
75 1
78 1
84 1
93 1

Conclusion: using buffer_vector with size 50 should cover most of the cases.

Signed-off-by: Alexander Borsuk <[email protected]>

…he first match fails Signed-off-by: Alexander Borsuk <[email protected]>

Signed-off-by: Alexander Borsuk <[email protected]>

Rendering without a cache is several times slower than without Harfbuzz. Experiments showed that the hit rate for the same rendered strings is very high. Signed-off-by: Alexander Borsuk <[email protected]>

Signed-off-by: Alexander Borsuk <[email protected]>

vng · 2024-06-25T22:03:40Z

drape/glyph_manager.cpp

+#ifdef DEBUG
+  static int const fontSize = fontPixelHeight;
+  ASSERT_EQUAL(fontSize, fontPixelHeight,
+      ("Cache relies on the same font height/metrics for each glyph", fontSize, fontPixelHeight));


Didn't get here. If fontPixelHeight should be always the same, why not set it once into GlyphManager and don't pass into ShapeText?

But is it true that font height should always be the same during the app's lifetime?

vng · 2024-06-25T22:04:49Z

drape/glyph_manager.cpp

+  {
+    LOG(LINFO, ("Clearing text metrics cache"));
+    // TODO(AB): Is there a better way? E.g. clear a half of the cache?
+    m_impl->m_textMetricsCache.clear();


vng · 2024-06-25T22:09:55Z

drape/glyph_manager.cpp

+    hb_buffer_set_language(m_impl->m_harfbuzzBuffer, hbLanguage);
+
+    auto u32CharacterIter{text.begin() + substring.m_start};
+    auto const end{text.begin() + substring.m_start + substring.m_length};


Do you really like this {} notation instead of a natural language wise assignment?
auto const end = u32CharacterIter + substring.m_length;

vng · 2024-06-25T22:11:32Z

drape/glyph_manager.hpp

@@ -34,7 +35,12 @@ struct TextMetrics

  void AddGlyphMetrics(int16_t font, uint16_t glyphId, int32_t xOffset, int32_t yOffset, int32_t xAdvance, int32_t height)
  {
-    m_glyphs.push_back({font, glyphId, xOffset, yOffset, xAdvance});
+    m_glyphs.push_back({{font, glyphId}, xOffset, yOffset, xAdvance});


nit: emplace_back(GlyphFontAndId(font, glyphId), xOffset, yOffset, xAdvance)

vng · 2024-06-25T22:14:54Z

drape/glyph_manager.cpp

+        LOG(LWARNING, ("No font was found for character", NumToHex(u32Character)));
+      else
+      {
+        // TODO(AB): Mapping font only by the first character in a string may fail in theory in some cases.


Ok, why TODO then? :)

vng · 2024-06-25T22:28:19Z

drape_frontend/text_layout.cpp

@@ -26,7 +25,7 @@ class TextGeometryGenerator

  void SetPenPosition(glsl::vec2 const & penOffset) {}

-  void operator() (dp::TextureManager::GlyphRegion const & glyph)
+  void operator() (dp::TextureManager::GlyphRegion const & glyph, dp::text::GlyphMetrics const & metrics)


dp::text::GlyphMetrics const & because unused?

vng · 2024-06-25T22:28:58Z

drape_frontend/text_layout.cpp

@@ -107,7 +107,7 @@ class TextOutlinedGeometryGenerator

  void SetPenPosition(glsl::vec2 const & penOffset) {}

-  void operator() (dp::TextureManager::GlyphRegion const & glyph)
+  void operator() (dp::TextureManager::GlyphRegion const & glyph, dp::text::GlyphMetrics const & metrics)


dp::text::GlyphMetrics const & because unused?

vng · 2024-06-25T22:33:08Z

drape/glyph.hpp

+
+//using TGlyph = std::pair<int16_t /* fontIndex */, uint16_t /* glyphId */>;
+// TODO(AB): Measure if 32 is the best value here.
+using TGlyphs = buffer_vector<GlyphFontAndId, 32>;


Note that GetGlyphs functions return by value -> so using move ctor -> the longer in-place buffer -> stranger moving :)

Use regular vector, especially with reserve
or

Use GetGlyphs(TGlyphs & res) notation.

biodranik requested a review from vng June 16, 2024 22:25

biodranik force-pushed the ab-harfbuzz branch 2 times, most recently from b1b21dd to e80bcb9 Compare June 16, 2024 23:28

vng reviewed Jun 17, 2024

View reviewed changes

biodranik force-pushed the ab-harfbuzz branch 2 times, most recently from ff02cb7 to 142d093 Compare June 21, 2024 23:45

biodranik added this to the Ready for alpha testing milestone Jun 21, 2024

biodranik force-pushed the ab-harfbuzz branch from 27ef838 to a00f082 Compare June 22, 2024 00:02

biodranik added 10 commits June 23, 2024 19:13

[drape] Harfbuzz integration

41d17a0

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Introduce GlyphFontAndId

ee1521d

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Use a struct for font and glyph id

af43ec8

[3party] Fixed Harfbuzz clang warning

75b8001

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Fixed vulkan warnings

b323fc7

Signed-off-by: Alexander Borsuk <[email protected]>

[qt] Properly call base class QOpenGLWidget::event

6c466a4

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] A bit faster code

b3e7814

[drape] Try to match other characters in a string to a font even if t…

49aa841

…he first match fails Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Minor fix

fc5b540

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Simple cache for text metrics to speed up the rendering

eb6e677

Rendering without a cache is several times slower than without Harfbuzz. Experiments showed that the hit rate for the same rendered strings is very high. Signed-off-by: Alexander Borsuk <[email protected]>

biodranik force-pushed the ab-harfbuzz branch from a00f082 to eb6e677 Compare June 23, 2024 18:10

biodranik added 3 commits June 23, 2024 22:02

[drape] Reuse the same harfbuzz buffer

e870a4f

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Fixed font rendering tests

33bb40a

Signed-off-by: Alexander Borsuk <[email protected]>

[drape] Get rid of unnecessary layouts

7189ff0

Signed-off-by: Alexander Borsuk <[email protected]>

vng reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[drape] Integrate Harfbuzz text shaping #8473

[drape] Integrate Harfbuzz text shaping #8473

biodranik commented Jun 16, 2024 •

edited

Loading

vng Jun 17, 2024

biodranik Jun 23, 2024

biodranik commented Jun 23, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024

vng Jun 25, 2024 •

edited

Loading

[drape] Integrate Harfbuzz text shaping #8473

Are you sure you want to change the base?

[drape] Integrate Harfbuzz text shaping #8473

Conversation

biodranik commented Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biodranik commented Jun 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vng Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

biodranik commented Jun 16, 2024 •

edited

Loading

vng Jun 25, 2024 •

edited

Loading