Implement kern (and may kerx?) cross-stream to support GeezaPro Arabic diacritics

[ebraminio]

From https://chromium-review.googlesource.com/c/chromium/src/+/1275945/33/third_party/WebKit/LayoutTests/platform/mac/fast/text/font-fallback-expected.png

util/hb-view /System/Library/Fonts/GeezaPro.ttc الأَبْجَدِيَّةـالعَرَبِيَّة --shaper coretext

util/hb-view /System/Library/Fonts/GeezaPro.ttc الأَبْجَدِيَّةـالعَرَبِيَّة --shaper ot or (with or without --font-funcs=ot)

On 10.13 machine,

CT:

OT:

3	en	Geeza Pro Regular; 14.0d1e4; 2017-12-05

bash-3.2$ shasum /System/Library/Fonts/GeezaPro.ttc 
ab26ea45dcaa5e1c5a958e42af10e10d330e7334  /System/Library/Fonts/GeezaPro.ttc

[ebraminio]

Still arguable maybe, I don't know

[behdad]

For a font that has no mark positioning data, this is really good already.

[ebraminio]

Replaced with high quality images. Maybe kasrah can be put a little higher?

[jfkthame]

From a brief look at the Geeza Pro on my 10.13 system, it appears to include a number of cross-stream kerning subtables (in a 'kern' table, not 'kerx'). I'm guessing these are intended to adjust the height of diacritics; but are they supported by HB yet?

[ebraminio]

Oh thanks @jfkthame, pretty sure that is the issue and Kern table doesn't show any indication of handling cross-stream AFAICS.

diff --git a/src/hb-ot-kern-table.hh b/src/hb-ot-kern-table.hh
index e361330b..9ab64af7 100644
--- a/src/hb-ot-kern-table.hh
+++ b/src/hb-ot-kern-table.hh
@@ -209,9 +209,10 @@ struct KernSubTableFormat1
     };
 
     inline driver_context_t (const KernSubTableFormat1 *table_,
-                            AAT::hb_aat_apply_context_t *c_) :
+                            AAT::hb_aat_apply_context_t *c_, bool is_cross_stream_) :
        c (c_),
        table (table_),
+       is_cross_stream (is_cross_stream_),
        /* Apparently the offset kernAction is from the beginning of the state-machine,
         * similar to offsets in morx table, NOT from beginning of this table, like
         * other subtables in kerx.  Discovered via testing. */
@@ -258,7 +259,9 @@ struct KernSubTableFormat1
          int v = *actions++;
          if (idx < buffer->len && buffer->info[idx].mask & kern_mask)
          {
-           if (HB_DIRECTION_IS_HORIZONTAL (buffer->props.direction))
+           bool is_horizontal = HB_DIRECTION_IS_HORIZONTAL (buffer->props.direction);
+           if (is_cross_stream) is_horizontal = !is_horizontal;
+           if (is_horizontal)
            {
              buffer->pos[idx].x_advance += c->font->em_scale_x (v);
              if (HB_DIRECTION_IS_BACKWARD (buffer->props.direction))
@@ -284,16 +287,16 @@ struct KernSubTableFormat1
     const UnsizedArrayOf<FWORD> &kernAction;
     unsigned int stack[8];
     unsigned int depth;
+    bool is_cross_stream;
   };
 
-  inline bool apply (AAT::hb_aat_apply_context_t *c) const
+  inline bool apply (AAT::hb_aat_apply_context_t *c, bool is_cross_stream) const
   {
     TRACE_APPLY (this);
-
     if (!c->plan->requested_kerning)
       return false;
 
-    driver_context_t dc (this, c);
+    driver_context_t dc (this, c, is_cross_stream);
 
     AAT::StateTableDriver<AAT::MortTypes, EntryData> driver (machine, c->buffer, c->font->face);
     driver.drive (&dc);
@@ -483,12 +486,12 @@ struct KernSubTable
     }
   }
 
-  inline void apply (AAT::hb_aat_apply_context_t *c, unsigned int format) const
+  inline void apply (AAT::hb_aat_apply_context_t *c, unsigned int format, bool is_cross_stream) const
   {
     /* TODO Switch to dispatch(). */
     switch (format) {
     case 0: u.format0.apply (c); return;
-    case 1: u.format1.apply (c); return;
+    case 1: u.format1.apply (c, is_cross_stream); return;
     case 2: u.format2.apply (c); return;
     case 3: u.format3.apply (c); return;
     default:                    return;
@@ -528,6 +531,9 @@ struct KernSubTableWrapper
   inline bool is_supported (void) const
   { return !(thiz()->coverage & T::CheckFlags); }
 
+  inline bool is_cross_stream (void) const
+  { return !(thiz()->coverage & T::CrossStream); }
+
   inline bool is_horizontal (void) const
   { return (thiz()->coverage & T::Direction) == T::CheckHorizontal; }
 
@@ -541,7 +547,7 @@ struct KernSubTableWrapper
   { return is_supported () && is_horizontal () ? get_kerning (left, right) : 0; }
 
   inline void apply (AAT::hb_aat_apply_context_t *c) const
-  { thiz()->subtable.apply (c, thiz()->format); }
+  { thiz()->subtable.apply (c, thiz()->format, is_cross_stream ()); }
 
   inline unsigned int get_size (void) const { return thiz()->length; }

And that made result a little different or better,

Before:

After:

But the way HB's buffer is ordered, I guess Behdad is better to have a look at this.

[behdad]

From a brief look at the Geeza Pro on my 10.13 system, it appears to include a number of cross-stream kerning subtables (in a 'kern' table, not 'kerx'). I'm guessing these are intended to adjust the height of diacritics; but are they supported by HB yet?

Interesting. Never saw a font with those. I'll implement that.

[behdad]

But the way HB's buffer is ordered, I guess Behdad is better to have a look at this.

That wouldn't be enough. Also, have to disable fallback mark positioning. I'll take a look.

[behdad]

I hope we don't have to look into kern table to decide if fallback mark positioning should be applied. Currently, looks like we might need to.

[behdad]

Also, we apply kern table if GPOS kern is missing. That can interact badly with mark positioning. Oh well.

[behdad]

Maybe kasrah can be put a little higher?

We decided to not move bottom marks up. That helped with Hebrew.