Rework handling of requiredFeature to solve problem with rlig in arial.ttf from winxp

https://bugzilla.mozilla.org/show_bug.cgi?id=986802
Fixes https://github.com/behdad/harfbuzz/pull/39

API Change:

-hb_ot_layout_language_get_required_feature_index
+hb_ot_layout_language_get_required_feature

New API takes an extra pointer argument.  Pass NULL in to get
behavior of previous API.

Reworked by behdad

src/hb-ot-layout-gsubgpos-private.hh

@@ -2268,8 +2268,8 @@ struct GSUBGPOS
 
   inline unsigned int get_feature_count (void) const
   { return (this+featureList).len; }
-  inline const Tag& get_feature_tag (unsigned int i) const
+  inline hb_tag_t get_feature_tag (unsigned int i) const
-  { return (this+featureList).get_tag (i); }
+  { return i == Index::NOT_FOUND_INDEX ? HB_TAG_NONE : (this+featureList).get_tag (i); }
   inline unsigned int get_feature_tags (unsigned int start_offset,
 					unsigned int *feature_count /* IN/OUT */,
 					hb_tag_t     *feature_tags /* OUT */) const

src/hb-ot-layout.cc

@@ -321,15 +321,19 @@ hb_ot_layout_script_find_language (hb_face_t    *face,
 }
 
 hb_bool_t
-hb_ot_layout_language_get_required_feature_index (hb_face_t    *face,
+hb_ot_layout_language_get_required_feature (hb_face_t    *face,
-						  hb_tag_t      table_tag,
+					    hb_tag_t      table_tag,
-						  unsigned int  script_index,
+					    unsigned int  script_index,
-						  unsigned int  language_index,
+					    unsigned int  language_index,
-						  unsigned int *feature_index)
+					    unsigned int *feature_index,
+					    hb_tag_t     *feature_tag)
 {
-  const OT::LangSys &l = get_gsubgpos_table (face, table_tag).get_script (script_index).get_lang_sys (language_index);
+  const OT::GSUBGPOS &g = get_gsubgpos_table (face, table_tag);
+  const OT::LangSys &l = g.get_script (script_index).get_lang_sys (language_index);
 
-  if (feature_index) *feature_index = l.get_required_feature_index ();
+  unsigned int index = l.get_required_feature_index ();
+  if (feature_index) *feature_index = index;
+  if (feature_tag) *feature_tag = g.get_feature_tag (index);
 
   return l.has_required_feature ();
 }
@@ -468,11 +472,12 @@ _hb_ot_layout_collect_lookups_features (hb_face_t      *face,
   if (!features)
   {
     unsigned int required_feature_index;
-    if (hb_ot_layout_language_get_required_feature_index (face,
+    if (hb_ot_layout_language_get_required_feature (face,
-							  table_tag,
+						    table_tag,
-							  script_index,
+						    script_index,
-							  language_index,
+						    language_index,
-							  &required_feature_index))
+						    &required_feature_index,
+						    NULL))
       _hb_ot_layout_collect_lookups_lookups (face,
 					     table_tag,
 					     required_feature_index,

src/hb-ot-layout.h

@@ -140,11 +140,12 @@ hb_ot_layout_script_find_language (hb_face_t    *face,
 				   unsigned int *language_index);
 
 hb_bool_t
-hb_ot_layout_language_get_required_feature_index (hb_face_t    *face,
+hb_ot_layout_language_get_required_feature (hb_face_t    *face,
-						  hb_tag_t      table_tag,
+					    hb_tag_t      table_tag,
-						  unsigned int  script_index,
+					    unsigned int  script_index,
-						  unsigned int  language_index,
+					    unsigned int  language_index,
-						  unsigned int *feature_index);
+					    unsigned int *feature_index,
+					    hb_tag_t     *feature_tag);
 
 unsigned int
 hb_ot_layout_language_get_feature_indexes (hb_face_t    *face,

src/hb-ot-map.cc

@@ -99,6 +99,7 @@ void hb_ot_map_builder_t::add_feature (hb_tag_t tag, unsigned int value,
 {
   feature_info_t *info = feature_infos.push();
   if (unlikely (!info)) return;
+  if (unlikely (!tag)) return;
   info->tag = tag;
   info->seq = feature_infos.len;
   info->max_value = value;
@@ -131,9 +132,25 @@ hb_ot_map_builder_t::compile (hb_ot_map_t &m)
 {
   m.global_mask = 1;
 
-  for (unsigned int table_index = 0; table_index < 2; table_index++) {
+  unsigned int required_feature_index[2];
+  hb_tag_t required_feature_tag[2];
+  /* We default to applying required feature in stage 0.  If the required
+   * feature has a tag that is known to the shaper, we apply required feature
+   * in the stage for that tag.
+   */
+  unsigned int required_feature_stage[2] = {0, 0};
+
+  for (unsigned int table_index = 0; table_index < 2; table_index++)
+  {
     m.chosen_script[table_index] = chosen_script[table_index];
     m.found_script[table_index] = found_script[table_index];
+
+    hb_ot_layout_language_get_required_feature (face,
+						table_tags[table_index],
+						script_index[table_index],
+						language_index[table_index],
+						&required_feature_index[table_index],
+						&required_feature_tag[table_index]);
   }
 
   if (!feature_infos.len)
@@ -166,7 +183,8 @@ hb_ot_map_builder_t::compile (hb_ot_map_t &m)
 
   /* Allocate bits now */
   unsigned int next_bit = 1;
-  for (unsigned int i = 0; i < feature_infos.len; i++) {
+  for (unsigned int i = 0; i < feature_infos.len; i++)
+  {
     const feature_info_t *info = &feature_infos[i];
 
     unsigned int bits_needed;
@@ -184,12 +202,20 @@ hb_ot_map_builder_t::compile (hb_ot_map_t &m)
     hb_bool_t found = false;
     unsigned int feature_index[2];
     for (unsigned int table_index = 0; table_index < 2; table_index++)
+    {
+      if (required_feature_tag[table_index] == info->tag)
+      {
+	required_feature_stage[table_index] = info->stage[table_index];
+	found = true;
+	continue;
+      }
       found |= hb_ot_layout_language_find_feature (face,
 						   table_tags[table_index],
 						   script_index[table_index],
 						   language_index[table_index],
 						   info->tag,
 						   &feature_index[table_index]);
+    }
     if (!found && !(info->flags & F_HAS_FALLBACK))
       continue;
 
@@ -224,23 +250,21 @@ hb_ot_map_builder_t::compile (hb_ot_map_t &m)
   add_gsub_pause (NULL);
   add_gpos_pause (NULL);
 
-  for (unsigned int table_index = 0; table_index < 2; table_index++) {
+  for (unsigned int table_index = 0; table_index < 2; table_index++)
-    hb_tag_t table_tag = table_tags[table_index];
+  {
-
     /* Collect lookup indices for features */
 
-    unsigned int required_feature_index;
-    if (hb_ot_layout_language_get_required_feature_index (face,
-							  table_tag,
-							  script_index[table_index],
-							  language_index[table_index],
-							  &required_feature_index))
-      m.add_lookups (face, table_index, required_feature_index, 1, true);
-
     unsigned int stage_index = 0;
     unsigned int last_num_lookups = 0;
     for (unsigned stage = 0; stage < current_stage[table_index]; stage++)
     {
+      if (required_feature_index[table_index] != HB_OT_LAYOUT_NO_FEATURE_INDEX &&
+	  required_feature_stage[table_index] == stage)
+	m.add_lookups (face, table_index,
+		       required_feature_index[table_index],
+		       1 /* mask */,
+		       true /* auto_zwj */);
+
       for (unsigned i = 0; i < m.features.len; i++)
         if (m.features[i].stage[table_index] == stage)
 	  m.add_lookups (face, table_index,

src/main.cc

@@ -151,8 +151,8 @@ main (int argc, char **argv)
 	for (int n_feature = 0; n_feature < num_features; n_feature++) {
 	  const Feature &feature = g.get_feature (n_feature);
 	  int num_lookups = feature.get_lookup_count ();
-	  printf ("    Feature %2d of %2d: %.4s\n", n_feature, num_features,
+	  printf ("    Feature %2d of %2d: %c%c%c%c\n", n_feature, num_features,
-	          (const char *)g.get_feature_tag(n_feature));
+	          HB_UNTAG(g.get_feature_tag(n_feature)));
 
 	  printf ("        %d lookup(s) found in feature\n", num_lookups);
 	  for (int n_lookup = 0; n_lookup < num_lookups; n_lookup++) {