Replaced Freetype with harfbuzz font support by JCash · Pull Request #199 · HOST-Oman/libraqm

JCash · 2025-07-28T09:22:31Z

Fixes: #198

It seems like it should work, however there is apparently some detail I don't understand as the direction-ttb-2.test fails on this:

@@ -54,11 +54,11 @@
 glyph [4]	x_offset: -162	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
 glyph [7]	x_offset: -235	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
 glyph [3]	x_offset: -327	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
-glyph [0]	x_offset: -500	y_offset: 0	x_advance: 0	font: Noto Serif CJK SC
-glyph [0]	x_offset: -500	y_offset: 0	x_advance: 0	font: Noto Serif CJK SC
-glyph [0]	x_offset: -500	y_offset: 0	x_advance: 0	font: Noto Serif CJK SC
-glyph [0]	x_offset: -500	y_offset: 0	x_advance: 0	font: Noto Serif CJK SC
-glyph [0]	x_offset: -500	y_offset: 0	x_advance: 0	font: Noto Serif CJK SC
+glyph [0]	x_offset: -500	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
+glyph [0]	x_offset: -500	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
+glyph [0]	x_offset: -500	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
+glyph [0]	x_offset: -500	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC
+glyph [0]	x_offset: -500	y_offset: -880	x_advance: 0	font: Noto Serif CJK SC

It is the only test that fails, and I can't seem to pinpoint the difference between the original Freetype created font.
I tried removing FT_Vector_Transform from the main branch, but that still passed all the tests on the main branch 🤔

behdad · 2025-07-28T09:28:22Z

Nice. Thanks for working on this.

JCash · 2025-07-28T09:23:32Z

src/raqm.c

-    if (run->font)
-      hb_font_destroy (run->font);
-


All font management is handled outside of the library.

We should reference the font passed to us. We can’t depend on the client not destroying the font under our feet.

While I'm happy do keep it for you, I would personally argue to keep the duties separate.
The font isn't really belonging to this library, but to harfbuzz.
As such I think it's reasonable to put that effort of resource management on the developer.

We are using the font, so it is our responsibility to keep a reference to it. HarfBuzz objects are reference counted, so the whoever using the object has to make sure to reference while using and destroy when done (destroy doe not immediately free the object, it decreases the reference count and only frees when it is zero).

JCash · 2025-07-28T09:26:45Z

src/raqm.c


 #include <assert.h>
 #include <string.h>
+#include <stdlib.h>


missing include for malloc/free

JCash · 2025-07-28T09:27:44Z

src/raqm.c


 #ifdef RAQM_TESTING
+#include <stdio.h>
+#include <hb-ot.h> // for family name


Only rely on hb-ot in testing mode, as the user may have built harbuzz without that support.

JCash · 2025-07-28T09:28:45Z

tests/raqm-test.c

+typedef struct _font_data
+{
+  hb_blob_t *blob;
+  hb_face_t *face;
+  hb_font_t *font;
+  void      *data;
+  struct _font_data* next;
+} font_data_t;


Easy structure to keep track of the created data.

Is the blob for the face? Is it needed after face creation? If not, it can be destroyed and let face manage lifetime. Same about keeping face around, can it be fetched from font as needed? Sorry, my knowledge of raqm code is non-existent.

Thanks!
You are correct, the blob is only for the face creation. and it's my knowledge about HB that is non-existent :)

More than happy to help!

All HB objects do lifecycle management, as you would expect.

If you ever need it, they also have [sg]et_user_data that allows attaching arbitrary data to them, to be destructed when the object is winding down.

JCash · 2025-07-28T09:29:18Z

tests/raqm-test.c

+static font_data_t*
+load_font(const char* path)
+{
+    size_t fontdata_size;
+    font_data_t *font = (font_data_t*)malloc(sizeof(font_data_t));
+    font->next = 0;
+    font->data = read_file(path, &fontdata_size);
+    assert (font->data != 0);
+
+    font->blob = hb_blob_create(font->data, fontdata_size, HB_MEMORY_MODE_READONLY, 0, 0);
+    assert (font->blob != 0);
+    font->face = hb_face_create(font->blob, 0);
+    assert (font->face != 0);
+    font->font = hb_font_create(font->face);
+    assert (font->font != 0);
+
+    return font;
+}


Showing users how one would create and use a hb_font_t from memory.

JCash · 2025-07-28T09:49:26Z

src/raqm.h

 * raqm_get_glyphs().
 */
 typedef struct raqm_glyph_t {
+    hb_font_t* hbfont;


structure size opt: putting the large pointer first in the structure.

This changes the API. The bigger question is how we handle this without breaking both the API and ABI? I don’t have any good ideas.

I don't think there's use case of using both api's at the same time, I would opt to use the ifdef like so:

typedef struct { #ifdef RAQM_FREETYPE FT_Face ftface; int ftloadflags; #else hb_font_t *hbfont; #endif

What do you think?

I’m fine with an approach like this.

structure size opt: putting the large pointer first in the structure.

In this case I don't think it matters, since there's an even number of ints before the pointer, so the struct size should be the same. I might be wrong.

structure size opt: putting the large pointer first in the structure.

In this case I don't think it matters, since there's an even number of ints before the pointer, so the struct size should be the same. I might be wrong.

You are correct, there is no difference (on 64-bit or 32-bit): https://godbolt.org/z/vGTood47v

khaledhosny

Thanks for working on this. But I’m afraid that this is unmergable in its current state unless we are going to break the ABI and raise the major version number, which is not something I want to do just yet.

khaledhosny · 2025-07-28T10:46:25Z

src/raqm.c

-raqm_set_freetype_face (raqm_t *rq,
-                        FT_Face face)
-{
-  return _raqm_set_freetype_face (rq, face, 0, rq->text_len);


We need keep this API.

khaledhosny · 2025-07-28T10:47:18Z

src/raqm.c

-    if (run->font)
-      hb_font_destroy (run->font);
-


We should reference the font passed to us. We can’t depend on the client not destroying the font under our feet.

khaledhosny · 2025-07-28T10:57:22Z

tests/raqm-test.c

+    font->data = read_file(path, &fontdata_size);
+    assert (font->data != 0);
+
+    blob = hb_blob_create(font->data, fontdata_size, HB_MEMORY_MODE_READONLY, 0, 0);


You can use hb_blob_create_from_file_or_fail() or even hb_face_create_from_file_or_fail()

khaledhosny · 2025-07-28T10:58:18Z

tests/raqm-test.c

+    assert (font->data != 0);
+
+    blob = hb_blob_create(font->data, fontdata_size, HB_MEMORY_MODE_READONLY, 0, 0);
+    assert (blob != 0);


hb_blob_create() never returns NULL (_or_fail group of functions do, though).

khaledhosny · 2025-07-28T11:01:21Z

src/raqm.h

 * raqm_get_glyphs().
 */
 typedef struct raqm_glyph_t {
+    hb_font_t* hbfont;


This changes the API. The bigger question is how we handle this without breaking both the API and ABI? I don’t have any good ideas.

khaledhosny · 2025-07-28T11:02:43Z

src/raqm.c

+    char family[128];
+    uint32_t family_name_len = sizeof(family);
+    hb_face_t *face = hb_font_get_face(run->font);
+    hb_ot_name_get_utf8 (face, HB_OT_NAME_ID_FONT_FAMILY, HB_LANGUAGE_INVALID, &family_name_len, family);


This deserves its own helper function.

was trying to land something like this this in libraqm with this pr: HOST-Oman#199

JCash added 6 commits July 28, 2025 10:06

Initial version using ifdef RAQM_FREETYPE

4d5a995

Removed usage of FreeType in library

90e6a59

Replaced FreeType in test with hb_font_t

f0e83a1

cleanup

c69cde6

output family name for unit tests

e3cfdb0

cleanup

f2a794c

JCash commented Jul 28, 2025

View reviewed changes

review cleanup

d27ce31

JCash commented Jul 28, 2025

View reviewed changes

khaledhosny reviewed Jul 28, 2025

View reviewed changes

Andersbakken added a commit to Andersbakken/libraqm that referenced this pull request Aug 26, 2025

Add pure harfbuzz support for libraqm. Note that someone apparently

ec40b65

was trying to land something like this this in libraqm with this pr: HOST-Oman#199

Andersbakken added a commit to Andersbakken/libraqm that referenced this pull request Oct 23, 2025

Add pure harfbuzz support for libraqm. Note that someone apparently

f25126d

was trying to land something like this this in libraqm with this pr: HOST-Oman#199

Conversation

JCash commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

behdad commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khaledhosny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JCash commented Jul 28, 2025 •

edited

Loading