Re: Modern font features, hacky patch

On Fri, Sep 07, 2012 at 06:43:26PM +0200, Khaled Hosny wrote:
> On Fri, Sep 07, 2012 at 05:18:46PM +0100, Caolán McNamara wrote:
> > On Sat, 2012-07-21 at 18:20 +0200, Khaled Hosny wrote:
> > > Here is a very crude, WIP patch. It adds --enable-harfbuzz configure
> > > option, plus some (very broken) harfbuzz layout code, but I can't get
> > > the ENABLE_HARFBUZZ to propagate in and thus can't do any testing
> > > because the code is never compiled. I appreciate any insights on how to
> > > get this to work.
> >
> > You need to put harfbuzz in the gb_Library_use_externals section of
> > Library_vcl.mk (or whichever is the right .mk file in vcl)
> >
> > It would be nice to get this feature in as a compile time option for
> > experimenting with.
>
> I’ve some code that more or less works (as in displaying some text), but
> it crashes like hell, I'm yet to figure out what is going on (I think
> I've to override more members of ServerFontLayout to make things saner).

Realizing that I might not get around fixing basic issues with HarfBuzz
integration anytime soon, I'm attaching preliminary patches I've for any
one interested in them. So far LTR text works fine, RTL horizontal
offsets (e.g. kerning) seems to be applied in the wrong direction,
probably the same old issue with ICU that we "band aided" a while ago,
which seems to be because of (Generic)SalLayout::AdjustLayout and
::Justify doing tricks with glyph advances and offsets (I've hard time
wrapping my head around all this SalLayout, GenericSalLayout,
MultiSalLayout etc. we really should consolidate this stuff now that we
have only one shaping path on *nix).

Regards,
 Khaled

From 6a05198fcf86af5d8898430077705920b71ff64b Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Sun, 28 Oct 2012 01:31:11 +0200
Subject: [PATCH 1/2] Add HarfBuzz support to the build system

Not used yet, and no support for local build either.

Change-Id: I7121fbe66e57eaa6be0e58451acf9a2ea7b50f59
---
 RepositoryExternal.mk |   23 +++++++++++++++++++++++
 config_host.mk.in     |    3 +++
 configure.ac          |   27 +++++++++++++++++++++++++++
 vcl/Library_vcl.mk    |    9 +++++++++
 4 files changed, 62 insertions(+)

diff --git a/RepositoryExternal.mk b/RepositoryExternal.mk
index e08beec..22aa2a0 100644
--- a/RepositoryExternal.mk
+++ b/RepositoryExternal.mk
@@ -1498,6 +1498,29 @@ gb_LinkTarget__use_pixbuf :=
 
 endif # SYSTEM_GDKPIXBUF
 
+ifeq ($(ENABLE_HARFBUZZ),TRUE)
+
+define gb_LinkTarget__use_harfbuzz
+$(call gb_LinkTarget_set_include,$(1),\
+       $$(INCLUDE) \
+       $(HARFBUZZ_CFLAGS) \
+)
+
+$(call gb_LinkTarget_add_libs,$(1),\
+       $(HARFBUZZ_LIBS) \
+)
+
+endef
+
+else # !ENABLE_HARFBUZZ
+
+define gb_LinkTarget__use_harfbuzz
+
+endef
+
+endif # ENABLE_HARFBUZZ
+
+
 ifeq ($(SYSTEM_DB),YES)
 
 define gb_LinkTarget__use_berkeleydb
diff --git a/config_host.mk.in b/config_host.mk.in
index 9a5fbfd..8322624 100644
--- a/config_host.mk.in
+++ b/config_host.mk.in
@@ -145,6 +145,7 @@ export ENABLE_GSTREAMER_0_10=@ENABLE_GSTREAMER_0_10@
 export ENABLE_GTK3=@ENABLE_GTK3@
 export ENABLE_GTK=@ENABLE_GTK@
 export ENABLE_GTK_PRINT=@ENABLE_GTK_PRINT@
+export ENABLE_HARFBUZZ=@ENABLE_HARFBUZZ@
 export ENABLE_HEADLESS=@ENABLE_HEADLESS@
 export ENABLE_TDEAB=@ENABLE_TDEAB@
 export ENABLE_TDE=@ENABLE_TDE@
@@ -240,6 +241,8 @@ export GUIBASE=@GUIBASE@
 export GUIBASE_FOR_BUILD=@GUIBASE_FOR_BUILD@
 export GUI_FOR_BUILD=@GUI_FOR_BUILD@
 export GXX_INCLUDE_PATH=@GXX_INCLUDE_PATH@
+export HARFBUZZ_CFLAGS=@HARFBUZZ_CFLAGS@
+export HARFBUZZ_LIBS=@HARFBUZZ_LIBS@
 export HAVE_CXX0X=@HAVE_CXX0X@
 export HAVE_GCC_AVX=@HAVE_GCC_AVX@
 export HAVE_GCC_BUILTIN_ATOMIC=@HAVE_GCC_BUILTIN_ATOMIC@
diff --git a/configure.ac b/configure.ac
index 6f654a2..a260cc4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -886,6 +886,11 @@ AC_ARG_ENABLE(gio,
         [Determines whether to use the GIO support.]),
 ,enable_gio=no)
 
+AC_ARG_ENABLE(harfbuzz,
+    AS_HELP_STRING([--enable-harfbuzz],
+        [Determines whether to use HarfBuzz text layout engine.]),
+,enable_harfbuzz=no)
+
 AC_ARG_ENABLE(telepathy,
     AS_HELP_STRING([--enable-telepathy],
         [Determines whether to enable Telepathy for collaboration.]),
@@ -9582,6 +9587,28 @@ AC_SUBST([GTK_PRINT_LIBS])
 
 
 dnl ===================================================================
+dnl Check whether the HarfBuzz libraries are available.
+dnl ===================================================================
+
+ENABLE_HARFBUZZ=""
+HARFBUZZ_CFLAGS=""
+HARFBUZZ_LIBS=""
+
+AC_MSG_CHECKING([whether to enable HarfBuzz support])
+if test "$_os" != "WINNT" -a "$_os" != "Darwin" -a "$enable_harfbuzz" = "yes"; then
+    ENABLE_HARFBUZZ="TRUE"
+    AC_MSG_RESULT([yes])
+    PKG_CHECK_MODULES( HARFBUZZ, harfbuzz >= 0.9.3 )
+else
+    AC_MSG_RESULT([no])
+fi
+
+AC_SUBST(ENABLE_HARFBUZZ)
+AC_SUBST(HARFBUZZ_CFLAGS)
+AC_SUBST(HARFBUZZ_LIBS)
+
+
+dnl ===================================================================
 dnl Check whether the Telepathy libraries are available.
 dnl ===================================================================
 
diff --git a/vcl/Library_vcl.mk b/vcl/Library_vcl.mk
index 1ff3339..a47ba9d 100644
--- a/vcl/Library_vcl.mk
+++ b/vcl/Library_vcl.mk
@@ -315,6 +315,15 @@ $(eval $(call gb_Library_use_external,vcl,graphite))
 
 endif
 
+ifneq ($(ENABLE_HARFBUZZ),NO)
+$(eval $(call gb_Library_add_defs,vcl,\
+    -DENABLE_HARFBUZZ \
+))
+
+$(eval $(call gb_Library_use_externals,vcl,harfbuzz))
+
+endif
+
 ifneq ($(ENABLE_LIBRSVG),NO)
 $(eval $(call gb_Library_add_exception_objects,vcl,\
     vcl/source/components/rasterizer_rsvg \
-- 
1.7.9.5

From 8533457b0f3705ef6dee9a1478d4b6371ef77ac0 Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Sun, 28 Oct 2012 01:32:06 +0200
Subject: [PATCH 2/2] Add support for using HarfBuzz instead of ICU LE

Seems to work fine but RTL kerning seems to be applied in the wrong
direction, seems like SalLayout::AdjustLayout and
GenericSalLayout::Justify are doing some tricks recalculating glyph
advances and offsets.

Change-Id: Ib7a3b178e108856f538fa8b36187f9500c658248
---
 vcl/generic/glyphs/gcach_layout.cxx |  138 +++++++++++++++++++++++++++++++++--
 1 file changed, 130 insertions(+), 8 deletions(-)

diff --git a/vcl/generic/glyphs/gcach_layout.cxx b/vcl/generic/glyphs/gcach_layout.cxx
index 7d0cd9e..e2a3986 100644
--- a/vcl/generic/glyphs/gcach_layout.cxx
+++ b/vcl/generic/glyphs/gcach_layout.cxx
@@ -35,9 +35,14 @@
 #include <sal/alloca.h>
 #include <rtl/instance.hxx>
 
+#ifdef ENABLE_HARFBUZZ
+#include <hb-ft.h>
+#include <hb-icu.h>
+#else
 #include <layout/LayoutEngine.h>
 #include <layout/LEFontInstance.h>
 #include <layout/LEScripts.h>
+#endif // ENABLE_HARFBUZZ
 
 #include <unicode/uscript.h>
 #include <unicode/ubidi.h>
@@ -69,7 +74,9 @@ bool ServerFontLayout::LayoutText( ImplLayoutArgs& rArgs )
 
 void ServerFontLayout::AdjustLayout( ImplLayoutArgs& rArgs )
 {
-    GenericSalLayout::AdjustLayout( rArgs );
+    SalLayout::AdjustLayout( rArgs );
+    if( rArgs.mnLayoutWidth )
+        GenericSalLayout::Justify( rArgs.mnLayoutWidth );
 
     // apply asian kerning if the glyphs are not already formatted
     if( (rArgs.mnFlags & SAL_LAYOUT_KERNING_ASIAN)
@@ -91,6 +98,121 @@ void ServerFontLayout::AdjustLayout( ImplLayoutArgs& rArgs )
 }
 
 // =======================================================================
+
+static bool lcl_CharIsJoiner(sal_Unicode cChar)
+{
+    return ((cChar == 0x200C) || (cChar == 0x200D));
+}
+
+#ifdef ENABLE_HARFBUZZ
+class HbLayoutEngine : public ServerFontLayoutEngine
+{
+public:
+                            HbLayoutEngine( ServerFont& ){};
+    virtual                 ~HbLayoutEngine(){};
+
+    virtual bool            layout( ServerFontLayout&, ImplLayoutArgs& );
+};
+
+bool HbLayoutEngine::layout( ServerFontLayout& rLayout, ImplLayoutArgs& rArgs )
+{
+    ServerFont& rFont = rLayout.GetServerFont();
+    FT_Face aFace = rFont.GetFtFace();
+
+    // allocate temporary arrays, note: round to even
+    int nGlyphCapacity = (3 * (rArgs.mnEndCharPos - rArgs.mnMinCharPos ) | 15) + 1;
+
+    rLayout.Reserve(nGlyphCapacity);
+
+    Point aNewPos(0, 0);
+    while( true )
+    {
+        int nMinRunPos, nEndRunPos;
+        bool bRightToLeft;
+        if( !rArgs.GetNextRun( &nMinRunPos, &nEndRunPos, &bRightToLeft ) )
+            break;
+
+        int nRunLen = nEndRunPos - nMinRunPos;
+
+        hb_font_t *aHbFont = hb_ft_font_create (aFace, NULL);
+        hb_buffer_t *aHbBuffer = hb_buffer_create ();
+        hb_buffer_set_direction (aHbBuffer, bRightToLeft ? HB_DIRECTION_RTL: HB_DIRECTION_LTR);
+
+        // XXX: do we need to set script.language or should HB guessing be enough?
+        //hb_buffer_set_script (aHbBuffer, hb_script_from_string ("arab"));
+        //hb_buffer_set_language (aHbBuffer, hb_language_from_string ("ar"));
+        hb_buffer_add_utf16 (aHbBuffer, rArgs.mpStr, nRunLen, nMinRunPos, nRunLen);
+        hb_shape (aHbFont, aHbBuffer, NULL, 0);
+
+        int nRunGlyphCount = hb_buffer_get_length (aHbBuffer);
+        hb_glyph_info_t *aHbGlyph = hb_buffer_get_glyph_infos (aHbBuffer, NULL);
+        hb_glyph_position_t *aHbPosition = hb_buffer_get_glyph_positions (aHbBuffer, NULL);
+
+        int32_t nLastCluster = -1;
+        for( int i = 0; i < nRunGlyphCount; ++i, ++aHbGlyph, ++aHbPosition ) {
+            int32_t nGlyphIndex = aHbGlyph->codepoint;
+            int32_t nCluster = aHbGlyph->cluster;
+            int32_t nCharPos = nCluster;
+
+            // if needed request glyph fallback by updating LayoutArgs
+            if( !nGlyphIndex )
+            {
+                if( nCharPos >= 0 )
+                {
+                    rArgs.NeedFallback( nCharPos, bRightToLeft );
+                    // XXX: ???
+                    if ( (nCharPos > 0) && lcl_CharIsJoiner(rArgs.mpStr[nCharPos-1]) )
+                        rArgs.NeedFallback( nCharPos-1, bRightToLeft );
+                    else if ( (nCharPos + 1 < nEndRunPos) && 
lcl_CharIsJoiner(rArgs.mpStr[nCharPos+1]) )
+                        rArgs.NeedFallback( nCharPos+1, bRightToLeft );
+                }
+
+                if( SAL_LAYOUT_FOR_FALLBACK & rArgs.mnFlags )
+                    continue;
+            }
+
+            long nGlyphFlags = 0;
+            if( bRightToLeft )
+                nGlyphFlags |= GlyphItem::IS_RTL_GLYPH;
+
+            // what is this for?
+            // XXX: rtl clusters
+            bool bInCluster = false;
+            if( nCluster == nLastCluster )
+                bInCluster = true;
+            nLastCluster = nCluster;
+            if( bInCluster )
+                nGlyphFlags |= GlyphItem::IS_IN_CLUSTER;
+
+            // XXX: query GDEF glyph class? Do we even need this?
+            bool bDiacritic = false;
+            if( bDiacritic )
+                nGlyphFlags |= GlyphItem::IS_DIACRITIC;
+
+            aHbPosition->x_offset /= 64;
+            aHbPosition->y_offset /= 64;
+            aHbPosition->x_advance /= 64;
+            aHbPosition->y_advance /= 64;
+
+            aNewPos = Point( aNewPos.X() + aHbPosition->x_offset, aNewPos.Y() - 
aHbPosition->y_offset );
+
+            GlyphItem aGI( nCharPos, nGlyphIndex, aNewPos, nGlyphFlags, aHbPosition->x_advance );
+            aGI.mnNewWidth = aHbPosition->y_offset + aHbPosition->x_advance;
+
+            rLayout.AppendGlyph( aGI );
+
+            aNewPos.X() += aHbPosition->x_advance;
+            aNewPos.Y() += aHbPosition->y_advance;
+        }
+
+        hb_buffer_destroy (aHbBuffer);
+        hb_font_destroy (aHbFont);
+    }
+
+    return true;
+}
+#else
+// =======================================================================
 // bridge to ICU LayoutEngine
 // =======================================================================
 
@@ -299,13 +421,6 @@ IcuLayoutEngine::~IcuLayoutEngine()
     delete mpIcuLE;
 }
 
-// -----------------------------------------------------------------------
-
-static bool lcl_CharIsJoiner(sal_Unicode cChar)
-{
-    return ((cChar == 0x200C) || (cChar == 0x200D));
-}
-
 //See https://bugs.freedesktop.org/show_bug.cgi?id=31016
 #define ARABIC_BANDAID
 
@@ -584,13 +699,20 @@ bool IcuLayoutEngine::layout(ServerFontLayout& rLayout, ImplLayoutArgs& rArgs)
     return true;
 }
 
+#endif // ENABLE_HARFBUZZ
+
 // =======================================================================
 
 ServerFontLayoutEngine* ServerFont::GetLayoutEngine()
 {
     // find best layout engine for font, platform, script and language
+#ifdef ENABLE_HARFBUZZ
+    if (!mpLayoutEngine)
+        mpLayoutEngine = new HbLayoutEngine(*this);
+#else
     if (!mpLayoutEngine)
         mpLayoutEngine = new IcuLayoutEngine(*this);
+#endif // ENABLE_HARFBUZZ
     return mpLayoutEngine;
 }
 
-- 
1.7.9.5