From 350f3a02ce225e5d78db8ac96de1351ff9f96dd5 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Sun, 28 Dec 2014 17:44:26 -0800 Subject: [PATCH] [ft] Add hb_ft_face_create_referenced() and hb_ft_font_create_referenced() When I originally wrote hb-ft, FreeType objects did not support reference counting. As such, hb_ft_face_create() and hb_ft_font_create() had a "destroy" callback and client was responsible for making sure FT_Face is kept around as long as the hb-font/face are alive. However, since this was not clearly documented, some clienets didn't correctly did that. In particular, some clients assumed that it's safe to destroy FT_Face and then hb_face_t. This, indeed, used to work, until 45fd9424c723f115ca98995b8f8a25185a6fc71d, which make face destroy access font tables. Now, I fixed that issue in 395b35903e052aecc97d0807e4f813c64c0d2b0b since the access was not needed, but the problem remains that not all clients handle this correctly. See: https://bugs.freedesktop.org/show_bug.cgi?id=86300 Fortunately, FT_Reference_Face() was added to FreeType in 2010, and so we can use it now. Originally I wanted to change hb_ft_face_create() and hb_ft_font_create() to reference the face if destroy==NULL was passed in. That would improve pretty much all clients, with little undesired effects. Except that FreeType itself, when compiled with HarfBuzz support, calls hb_ft_font_create() with destroy==NULL and saves the resulting hb-font on the ft-face (why does it not free it immediately?). Making hb-face reference ft-face causes a cycling reference there. At least, that's my current understanding. At any rate, a cleaner approach, even if it means all clients will need a change, is to introduce brand new API. Which this commit does. Some comments added to hb-ft.h, hoping to make future clients make better choices. Fixes https://bugs.freedesktop.org/show_bug.cgi?id=75299 --- configure.ac | 4 ++-- src/hb-ft.cc | 32 ++++++++++++++++++++++++++++ src/hb-ft.h | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 8f894a8d3..4dc562c48 100644 --- a/configure.ac +++ b/configure.ac @@ -297,8 +297,8 @@ AC_ARG_WITH(freetype, [with_freetype=auto]) have_freetype=false if test "x$with_freetype" = "xyes" -o "x$with_freetype" = "xauto"; then - # See freetype/docs/VERSION.DLL; 9.19.13 means freetype-2.3.8 - PKG_CHECK_MODULES(FREETYPE, freetype2 >= 9.19.3, have_freetype=true, :) + # See freetype/docs/VERSION.DLL; 12.0.6 means freetype-2.4.2 + PKG_CHECK_MODULES(FREETYPE, freetype2 >= 12.0.6, have_freetype=true, :) fi if test "x$with_freetype" = "xyes" -a "x$have_freetype" != "xtrue"; then AC_MSG_ERROR([FreeType support requested but libfreetype2 not found]) diff --git a/src/hb-ft.cc b/src/hb-ft.cc index 0e21573e7..9b2a90844 100644 --- a/src/hb-ft.cc +++ b/src/hb-ft.cc @@ -352,6 +352,22 @@ hb_ft_face_create (FT_Face ft_face, return face; } +/** + * hb_ft_face_create_referenced: + * @ft_face: + * + * + * + * Return value: (transfer full): + * Since: 1.0 + **/ +hb_face_t * +hb_ft_face_create_referenced (FT_Face ft_face) +{ + FT_Reference_Face (ft_face); + return hb_ft_face_create (ft_face, (hb_destroy_func_t) FT_Done_Face); +} + static void hb_ft_face_finalize (FT_Face ft_face) { @@ -421,6 +437,22 @@ hb_ft_font_create (FT_Face ft_face, return font; } +/** + * hb_ft_font_create_referenced: + * @ft_face: + * + * + * + * Return value: (transfer full): + * Since: 1.0 + **/ +hb_font_t * +hb_ft_font_create_referenced (FT_Face ft_face) +{ + FT_Reference_Face (ft_face); + return hb_ft_font_create (ft_face, (hb_destroy_func_t) FT_Done_Face); +} + /* Thread-safe, lock-free, FT_Library */ diff --git a/src/hb-ft.h b/src/hb-ft.h index 696251e14..92f4b3614 100644 --- a/src/hb-ft.h +++ b/src/hb-ft.h @@ -34,19 +34,76 @@ HB_BEGIN_DECLS -/* Note: FreeType is not thread-safe. Hence, these functions are not either. */ +/* + * Note: FreeType is not thread-safe. + * Hence, these functions are not either. + */ +/* + * hb-face from ft-face. + */ + +/* This one creates a new hb-face for given ft-face. + * When the returned hb-face is destroyed, the destroy + * callback is called (if not NULL), with the ft-face passed + * to it. + * + * The client is responsible to make sure that ft-face is + * destroyed after hb-face is destroyed. + * + * Most often you don't want this function. You should use either + * hb_ft_face_create_cached(), or hb_ft_face_create_referenced(). + * In particular, if you are going to pass NULL as destroy, you + * probably should use (the more recent) hb_ft_face_create_referenced() + * instead. + */ hb_face_t * hb_ft_face_create (FT_Face ft_face, hb_destroy_func_t destroy); +/* This version is like hb_ft_face_create(), except that it caches + * the hb-face using the generic pointer of the ft-face. This means + * that subsequent calls to this function with the same ft-face will + * return the same hb-face (correctly referenced). + * + * Client is still responsible for making sure that ft-face is destroyed + * after hb-face is. + */ hb_face_t * hb_ft_face_create_cached (FT_Face ft_face); +/* This version is like hb_ft_face_create(), except that it calls + * FT_Reference_Face() on ft-face, as such keeping ft-face alive + * as long as the hb-face is. + * + * This is the most convenient version to use. Use it unless you have + * very good reasons not to. + */ +hb_face_t * +hb_ft_face_create_referenced (FT_Face ft_face); + + +/* + * hb-font from ft-face. + */ + +/* + * Note: + * + * Set face size on ft-face before creating hb-font from it. + * Otherwise hb-ft would NOT pick up the font size correctly. + */ + +/* See notes on hb_ft_face_create(). Same issues re lifecycle-management + * apply here. Use hb_ft_font_create_referenced() if you can. */ hb_font_t * hb_ft_font_create (FT_Face ft_face, hb_destroy_func_t destroy); +/* See notes on hb_ft_face_create_referenced() re lifecycle-management + * issues. */ +hb_font_t * +hb_ft_font_create_referenced (FT_Face ft_face); /* Makes an hb_font_t use FreeType internally to implement font functions. */