Merge pull request #4474 from googlefonts/fix_value_format

[instancer] fix value format for SinglePos/PairPos when strip_hints is on

src/OT/Layout/GDEF/GDEF.hh

@@ -705,6 +705,7 @@ struct GDEFVersion1_2
     if (subset_varstore)
     {
       out->version.minor = 3;
+      c->plan->has_gdef_varstore = true;
     } else if (subset_markglyphsetsdef) {
       out->version.minor = 2;
       c->serializer->revert (snapshot_version2);

src/OT/Layout/GPOS/Common.hh

@@ -23,7 +23,7 @@ static void SinglePos_serialize (hb_serialize_context_t *c,
                                  const SrcLookup *src,
                                  Iterator it,
                                  const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *layout_variation_idx_delta_map,
-                                 bool update_var_device_flags);
+                                 unsigned new_format);
 
 
 }

src/OT/Layout/GPOS/PairPosFormat1.hh

@@ -135,10 +135,26 @@ struct PairPosFormat1_3
     hb_pair_t<unsigned, unsigned> newFormats = hb_pair (valueFormat[0], valueFormat[1]);
 
     if (c->plan->normalized_coords)
-      newFormats = compute_effective_value_formats (glyphset, &c->plan->layout_variation_idx_delta_map);
-
-    if (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING)
-      newFormats = compute_effective_value_formats (glyphset);
+    {
+      /* all device flags will be dropped when full instancing, no need to strip
+       * hints, also do not strip emtpy cause we don't compute the new default
+       * value during stripping */
+      newFormats = compute_effective_value_formats (glyphset, false, false, &c->plan->layout_variation_idx_delta_map);
+    }
+    /* do not strip hints for VF */
+    else if (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING)
+    {
+      hb_blob_t* blob = hb_face_reference_table (c->plan->source, HB_TAG ('f','v','a','r'));
+      bool has_fvar = (blob != hb_blob_get_empty ());
+      hb_blob_destroy (blob);
+
+      bool strip = !has_fvar;
+      /* special case: strip hints when a VF has no GDEF varstore after
+       * subsetting*/
+      if (has_fvar && !c->plan->has_gdef_varstore)
+        strip = true;
+      newFormats = compute_effective_value_formats (glyphset, strip, true);
+    }
     
     out->valueFormat[0] = newFormats.first;
     out->valueFormat[1] = newFormats.second;
@@ -173,6 +189,7 @@ struct PairPosFormat1_3
 
 
   hb_pair_t<unsigned, unsigned> compute_effective_value_formats (const hb_set_t& glyphset,
+                                                                 bool strip_hints, bool strip_empty,
                                                                  const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map = nullptr) const
   {
     unsigned record_size = PairSet::get_size (valueFormat);
@@ -193,16 +210,8 @@ struct PairPosFormat1_3
       {
         if (record->intersects (glyphset))
         {
-          if (!varidx_delta_map)
-          {
-            format1 = format1 | valueFormat[0].get_effective_format (record->get_values_1 ());
-            format2 = format2 | valueFormat[1].get_effective_format (record->get_values_2 (valueFormat[0]));
-          }
-          else
-          {
-            format1 = format1 | valueFormat[0].update_var_device_table_flags (record->get_values_1 (), &set, varidx_delta_map);
-            format2 = format2 | valueFormat[1].update_var_device_table_flags (record->get_values_2 (valueFormat[0]), &set, varidx_delta_map);
-          }
+          format1 = format1 | valueFormat[0].get_effective_format (record->get_values_1 (), strip_hints, strip_empty, &set, varidx_delta_map);
+          format2 = format2 | valueFormat[1].get_effective_format (record->get_values_2 (valueFormat[0]), strip_hints, strip_empty, &set, varidx_delta_map);
         }
         record = &StructAtOffset<const PairValueRecord> (record, record_size);
       }

src/OT/Layout/GPOS/PairPosFormat2.hh

@@ -287,11 +287,27 @@ struct PairPosFormat2_4 : ValueBase
     unsigned len2 = valueFormat2.get_len ();
 
     hb_pair_t<unsigned, unsigned> newFormats = hb_pair (valueFormat1, valueFormat2);
-    if (c->plan->normalized_coords)
-      newFormats = compute_effective_value_formats (klass1_map, klass2_map, &c->plan->layout_variation_idx_delta_map);
 
-    if (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING)
-      newFormats = compute_effective_value_formats (klass1_map, klass2_map);
+    if (c->plan->normalized_coords)
+    {
+      /* in case of full instancing, all var device flags will be dropped so no
+       * need to strip hints here */
+      newFormats = compute_effective_value_formats (klass1_map, klass2_map, false, false, &c->plan->layout_variation_idx_delta_map);
+    }
+    /* do not strip hints for VF */
+    else if (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING)
+    {
+      hb_blob_t* blob = hb_face_reference_table (c->plan->source, HB_TAG ('f','v','a','r'));
+      bool has_fvar = (blob != hb_blob_get_empty ());
+      hb_blob_destroy (blob);
+
+      bool strip = !has_fvar;
+      /* special case: strip hints when a VF has no GDEF varstore after
+       * subsetting*/
+      if (has_fvar && !c->plan->has_gdef_varstore)
+        strip = true;
+      newFormats = compute_effective_value_formats (klass1_map, klass2_map, strip, true);
+    }
 
     out->valueFormat1 = newFormats.first;
     out->valueFormat2 = newFormats.second;
@@ -324,6 +340,7 @@ struct PairPosFormat2_4 : ValueBase
 
   hb_pair_t<unsigned, unsigned> compute_effective_value_formats (const hb_map_t& klass1_map,
                                                                  const hb_map_t& klass2_map,
+                                                                 bool strip_hints, bool strip_empty,
                                                                  const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map = nullptr) const
   {
     unsigned len1 = valueFormat1.get_len ();
@@ -338,16 +355,8 @@ struct PairPosFormat2_4 : ValueBase
       for (unsigned class2_idx : + hb_range ((unsigned) class2Count) | hb_filter (klass2_map))
       {
         unsigned idx = (class1_idx * (unsigned) class2Count + class2_idx) * record_size;
-        if (!varidx_delta_map)
-        {
-          format1 = format1 | valueFormat1.get_effective_format (&values[idx]);
-          format2 = format2 | valueFormat2.get_effective_format (&values[idx + len1]);
-        }
-        else
-        {
-          format1 = format1 | valueFormat1.update_var_device_table_flags (&values[idx], this, varidx_delta_map);
-          format2 = format2 | valueFormat2.update_var_device_table_flags (&values[idx + len1], this, varidx_delta_map);
-        }
+        format1 = format1 | valueFormat1.get_effective_format (&values[idx], strip_hints, strip_empty, this, varidx_delta_map);
+        format2 = format2 | valueFormat2.get_effective_format (&values[idx + len1], strip_hints, strip_empty, this, varidx_delta_map);
       }
 
       if (format1 == valueFormat1 && format2 == valueFormat2)

src/OT/Layout/GPOS/SinglePos.hh

@@ -39,15 +39,12 @@ struct SinglePos
                   const SrcLookup* src,
                   Iterator glyph_val_iter_pairs,
                   const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *layout_variation_idx_delta_map,
-                  bool update_var_device_flags)
+                  unsigned newFormat)
   {
     if (unlikely (!c->extend_min (u.format))) return;
     unsigned format = 2;
-    ValueFormat new_format = src->get_value_format ();
-
-    if (update_var_device_flags)
-      new_format = new_format.update_var_device_table_flags (+ glyph_val_iter_pairs | hb_map (hb_second),
-                                                             src, layout_variation_idx_delta_map);
+    ValueFormat new_format;
+    new_format = newFormat;
 
     if (glyph_val_iter_pairs)
       format = get_format (glyph_val_iter_pairs);
@@ -90,8 +87,8 @@ SinglePos_serialize (hb_serialize_context_t *c,
                      const SrcLookup *src,
                      Iterator it,
                      const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *layout_variation_idx_delta_map,
-                     bool update_var_device_flags)
-{ c->start_embed<SinglePos> ()->serialize (c, src, it, layout_variation_idx_delta_map, update_var_device_flags); }
+                     unsigned new_format)
+{ c->start_embed<SinglePos> ()->serialize (c, src, it, layout_variation_idx_delta_map, new_format); }
 
 
 }

src/OT/Layout/GPOS/SinglePosFormat1.hh

@@ -146,6 +146,30 @@ struct SinglePosFormat1 : ValueBase
     hb_set_t intersection;
     (this+coverage).intersect_set (glyphset, intersection);
 
+    unsigned new_format = valueFormat;
+
+    if (c->plan->normalized_coords)
+    {
+      new_format = valueFormat.get_effective_format (values.arrayZ, false, false, this, &c->plan->layout_variation_idx_delta_map);
+    }
+    /* do not strip hints for VF */
+    else if (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING)
+    {
+      hb_blob_t* blob = hb_face_reference_table (c->plan->source, HB_TAG ('f','v','a','r'));
+      bool has_fvar = (blob != hb_blob_get_empty ());
+      hb_blob_destroy (blob);
+
+      bool strip = !has_fvar;
+      /* special case: strip hints when a VF has no GDEF varstore after
+       * subsetting*/
+      if (has_fvar && !c->plan->has_gdef_varstore)
+        strip = true;
+      new_format = valueFormat.get_effective_format (values.arrayZ,
+                                                     strip, /* strip hints */
+                                                     true, /* strip empty */
+                                                     this, nullptr);
+    }
+
     auto it =
     + hb_iter (intersection)
     | hb_map_retains_sorting (glyph_map)
@@ -153,7 +177,7 @@ struct SinglePosFormat1 : ValueBase
     ;
 
     bool ret = bool (it);
-    SinglePos_serialize (c->serializer, this, it, &c->plan->layout_variation_idx_delta_map, bool (c->plan->normalized_coords));
+    SinglePos_serialize (c->serializer, this, it, &c->plan->layout_variation_idx_delta_map, new_format);
     return_trace (ret);
   }
 };

src/OT/Layout/GPOS/SinglePosFormat2.hh

@@ -143,6 +143,37 @@ struct SinglePosFormat2 : ValueBase
     coverage.serialize_serialize (c, glyphs);
   }
 
+  template<typename Iterator,
+      hb_requires (hb_is_iterator (Iterator))>
+  unsigned compute_effective_format (const hb_face_t *face,
+                                     Iterator it,
+                                     bool is_instancing, bool strip_hints,
+                                     bool has_gdef_varstore,
+                                     const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map) const
+  {
+    hb_blob_t* blob = hb_face_reference_table (face, HB_TAG ('f','v','a','r'));
+    bool has_fvar = (blob != hb_blob_get_empty ());
+    hb_blob_destroy (blob);
+
+    unsigned new_format = 0;
+    if (is_instancing)
+    {
+      new_format = new_format | valueFormat.get_effective_format (+ it | hb_map (hb_second), false, false, this, varidx_delta_map);
+    }
+    /* do not strip hints for VF */
+    else if (strip_hints)
+    {
+      bool strip = !has_fvar;
+      if (has_fvar && !has_gdef_varstore)
+        strip = true;
+      new_format = new_format | valueFormat.get_effective_format (+ it | hb_map (hb_second), strip, true, this, nullptr);
+    }
+    else
+      new_format = valueFormat;
+
+    return new_format;
+  }
+
   bool subset (hb_subset_context_t *c) const
   {
     TRACE_SUBSET (this);
@@ -163,8 +194,13 @@ struct SinglePosFormat2 : ValueBase
                               })
     ;
 
+    unsigned new_format = compute_effective_format (c->plan->source, it,
+                                                    bool (c->plan->normalized_coords),
+                                                    bool (c->plan->flags & HB_SUBSET_FLAGS_NO_HINTING),
+                                                    c->plan->has_gdef_varstore,
+                                                    &c->plan->layout_variation_idx_delta_map);
     bool ret = bool (it);
-    SinglePos_serialize (c->serializer, this, it, &c->plan->layout_variation_idx_delta_map, bool (c->plan->normalized_coords));
+    SinglePos_serialize (c->serializer, this, it, &c->plan->layout_variation_idx_delta_map, new_format);
     return_trace (ret);
   }
 };

src/OT/Layout/GPOS/ValueFormat.hh

@@ -144,11 +144,29 @@ struct ValueFormat : HBUINT16
     return ret;
   }
 
-  unsigned int get_effective_format (const Value *values) const
+  unsigned int get_effective_format (const Value *values, bool strip_hints, bool strip_empty, const ValueBase *base,
+                                     const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map) const
   {
     unsigned int format = *this;
     for (unsigned flag = xPlacement; flag <= yAdvDevice; flag = flag << 1) {
-      if (format & flag) should_drop (*values++, (Flags) flag, &format);
+      if (format & flag)
+      {
+        if (strip_hints && flag >= xPlaDevice)
+        {
+          format = format & ~flag;
+          values++;
+          continue;
+        }
+        if (varidx_delta_map && flag >= xPlaDevice)
+        {
+          update_var_flag (values++, (Flags) flag, &format, base, varidx_delta_map);
+          continue;
+        }
+        /* do not strip empty when instancing, cause we don't know whether the new
+         * default value is 0 or not */
+        if (strip_empty) should_drop (*values, (Flags) flag, &format);
+        values++;
+      }
     }
 
     return format;
@@ -156,11 +174,12 @@ struct ValueFormat : HBUINT16
 
   template<typename Iterator,
       hb_requires (hb_is_iterator (Iterator))>
-  unsigned int get_effective_format (Iterator it) const {
+  unsigned int get_effective_format (Iterator it, bool strip_hints, bool strip_empty, const ValueBase *base,
+                                     const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map) const {
     unsigned int new_format = 0;
 
     for (const hb_array_t<const Value>& values : it)
-      new_format = new_format | get_effective_format (&values);
+      new_format = new_format | get_effective_format (&values, strip_hints, strip_empty, base, varidx_delta_map);
 
     return new_format;
   }
@@ -253,36 +272,6 @@ struct ValueFormat : HBUINT16
     }
   }
 
-  unsigned update_var_device_table_flags (const Value *values,
-                                          const ValueBase *base,
-                                          const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map) const
-  {
-    unsigned format = *this;
-    for (unsigned flag = xPlacement; flag <= yAdvDevice; flag = flag << 1)
-    {
-      if (format & flag)
-      {
-        if (flag >= xPlaDevice) update_var_flag (values, (Flags) flag, &format, base, varidx_delta_map);
-        values++;
-      }
-    }
-
-    return format;
-  }
-
-  template<typename Iterator,
-      hb_requires (hb_is_iterator (Iterator))>
-  unsigned update_var_device_table_flags (Iterator it,
-                                          const ValueBase* base,
-                                          const hb_hashmap_t<unsigned, hb_pair_t<unsigned, int>> *varidx_delta_map) const
-  {
-    unsigned new_format = 0;
-    for (const hb_array_t<const Value>& values : it)
-      new_format = new_format | update_var_device_table_flags (&values, base, varidx_delta_map);
-
-    return new_format;
-  }
-
   private:
   bool sanitize_value_devices (hb_sanitize_context_t *c, const ValueBase *base, const Value *values) const
   {

src/hb-subset-plan.cc

@@ -1124,6 +1124,7 @@ hb_subset_plan_t::hb_subset_plan_t (hb_face_t *face,
   user_axes_location = input->axes_location;
   all_axes_pinned = false;
   pinned_at_default = true;
+  has_gdef_varstore = false;
 
 #ifdef HB_EXPERIMENTAL_API
   for (auto _ : input->name_table_overrides)

src/hb-subset-plan.hh

@@ -147,6 +147,9 @@ struct hb_subset_plan_t
   bool gsub_insert_catch_all_feature_variation_rec;
   bool gpos_insert_catch_all_feature_variation_rec;
 
+  // whether GDEF VarStore is retained
+  mutable bool has_gdef_varstore;
+
 #define HB_SUBSET_PLAN_MEMBER(Type, Name) Type Name;
 #include "hb-subset-plan-member-list.hh"
 #undef HB_SUBSET_PLAN_MEMBER

src/hb-subset.cc

@@ -462,7 +462,7 @@ _dependencies_satisfied (hb_subset_plan_t *plan, hb_tag_t tag,
   case HB_OT_TAG_maxp:
     return !plan->normalized_coords || !pending_subset_tags.has (HB_OT_TAG_glyf);
   case HB_OT_TAG_GPOS:
-    return !plan->normalized_coords || plan->all_axes_pinned || !pending_subset_tags.has (HB_OT_TAG_GDEF);
+    return plan->all_axes_pinned || !pending_subset_tags.has (HB_OT_TAG_GDEF);
   default:
     return true;
   }

test/subset/data/tests/value_format_partial_instance.tests

@@ -0,0 +1,12 @@
+FONTS:
+NotoSansOriya-valueformat-subset.ttf
+
+PROFILES:
+default.txt
+drop-hints.txt
+
+SUBSETS:
+*
+
+INSTANCES:
+wght=400

test/subset/meson.build

@@ -76,6 +76,7 @@ if get_option('experimental_api')
     'empty_region_vardata',
     'feature_variations_partial_instance',
     'gdef_partial_instance',
+    'value_format_partial_instance',
     ]
 endif