From f6a242b6050e8342c34eb458a696fcedb40de2e5 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 18:58:41 +0000 Subject: [PATCH 01/24] [repacker] begin adding PairPos splitting support. --- src/Makefile.am | 2 +- src/Makefile.sources | 4 +- src/OT/Layout/GPOS/MarkArray.hh | 9 +- src/OT/Layout/GPOS/PairValueRecord.hh | 2 + ...{gsubgpos-graph.cc => gsubgpos-context.cc} | 4 +- src/graph/gsubgpos-context.hh | 60 ++++++++++ src/graph/gsubgpos-graph.hh | 76 ++++++++---- src/graph/pairpos-graph.hh | 110 ++++++++++++++++++ src/harfbuzz-subset.cc | 2 +- src/hb-repacker.hh | 31 ++++- src/meson.build | 4 +- 11 files changed, 268 insertions(+), 36 deletions(-) rename src/graph/{gsubgpos-graph.cc => gsubgpos-context.cc} (93%) create mode 100644 src/graph/gsubgpos-context.hh create mode 100644 src/graph/pairpos-graph.hh diff --git a/src/Makefile.am b/src/Makefile.am index 7e1fb3b12..9940bb652 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -411,7 +411,7 @@ test_priority_queue_SOURCES = test-priority-queue.cc hb-static.cc test_priority_queue_CPPFLAGS = $(HBCFLAGS) test_priority_queue_LDADD = libharfbuzz.la $(HBLIBS) -test_repacker_SOURCES = test-repacker.cc hb-static.cc graph/gsubgpos-graph.cc +test_repacker_SOURCES = test-repacker.cc hb-static.cc graph/gsubgpos-context.cc test_repacker_CPPFLAGS = $(HBCFLAGS) test_repacker_LDADD = libharfbuzz.la libharfbuzz-subset.la $(HBLIBS) diff --git a/src/Makefile.sources b/src/Makefile.sources index 9f4e17dbe..16473634e 100644 --- a/src/Makefile.sources +++ b/src/Makefile.sources @@ -349,7 +349,9 @@ HB_SUBSET_sources = \ hb-repacker.hh \ graph/graph.hh \ graph/gsubgpos-graph.hh \ - graph/gsubgpos-graph.cc \ + graph/gsubgpos-context.hh \ + graph/gsubgpos-context.cc \ + graph/pairpos-graph.hh \ graph/serialize.hh \ $(NULL) diff --git a/src/OT/Layout/GPOS/MarkArray.hh b/src/OT/Layout/GPOS/MarkArray.hh index be5073864..cb5e8b268 100644 --- a/src/OT/Layout/GPOS/MarkArray.hh +++ b/src/OT/Layout/GPOS/MarkArray.hh @@ -97,10 +97,11 @@ struct MarkArray : Array16Of /* Array of MarkRecords--in Cove } }; -static void Markclass_closure_and_remap_indexes (const Coverage &mark_coverage, - const MarkArray &mark_array, - const hb_set_t &glyphset, - hb_map_t* klass_mapping /* INOUT */) +HB_INTERNAL inline +void Markclass_closure_and_remap_indexes (const Coverage &mark_coverage, + const MarkArray &mark_array, + const hb_set_t &glyphset, + hb_map_t* klass_mapping /* INOUT */) { hb_set_t orig_classes; diff --git a/src/OT/Layout/GPOS/PairValueRecord.hh b/src/OT/Layout/GPOS/PairValueRecord.hh index 55de5abeb..bd95abde1 100644 --- a/src/OT/Layout/GPOS/PairValueRecord.hh +++ b/src/OT/Layout/GPOS/PairValueRecord.hh @@ -1,6 +1,8 @@ #ifndef OT_LAYOUT_GPOS_PAIRVALUERECORD_HH #define OT_LAYOUT_GPOS_PAIRVALUERECORD_HH +#include "ValueFormat.hh" + namespace OT { namespace Layout { namespace GPOS_impl { diff --git a/src/graph/gsubgpos-graph.cc b/src/graph/gsubgpos-context.cc similarity index 93% rename from src/graph/gsubgpos-graph.cc rename to src/graph/gsubgpos-context.cc index d12e52fee..f71c73aa1 100644 --- a/src/graph/gsubgpos-graph.cc +++ b/src/graph/gsubgpos-context.cc @@ -28,7 +28,7 @@ namespace graph { -make_extension_context_t::make_extension_context_t (hb_tag_t table_tag_, +gsubgpos_graph_context_t::gsubgpos_graph_context_t (hb_tag_t table_tag_, graph_t& graph_, hb_vector_t& buffer_) : table_tag (table_tag_), @@ -47,7 +47,7 @@ make_extension_context_t::make_extension_context_t (hb_tag_t table_tag_, buffer.alloc (num_non_ext_subtables () * extension_size); } -unsigned make_extension_context_t::num_non_ext_subtables () { +unsigned gsubgpos_graph_context_t::num_non_ext_subtables () { unsigned count = 0; for (auto l : lookups.values ()) { diff --git a/src/graph/gsubgpos-context.hh b/src/graph/gsubgpos-context.hh new file mode 100644 index 000000000..ff8f29e0d --- /dev/null +++ b/src/graph/gsubgpos-context.hh @@ -0,0 +1,60 @@ +/* + * Copyright © 2022 Google, Inc. + * + * This is part of HarfBuzz, a text shaping library. + * + * Permission is hereby granted, without written agreement and without + * license or royalty fees, to use, copy, modify, and distribute this + * software and its documentation for any purpose, provided that the + * above copyright notice and the following two paragraphs appear in + * all copies of this software. + * + * IN NO EVENT SHALL THE COPYRIGHT HOLDER BE LIABLE TO ANY PARTY FOR + * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES + * ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS DOCUMENTATION, EVEN + * IF THE COPYRIGHT HOLDER HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH + * DAMAGE. + * + * THE COPYRIGHT HOLDER SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING, + * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS + * ON AN "AS IS" BASIS, AND THE COPYRIGHT HOLDER HAS NO OBLIGATION TO + * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. + * + * Google Author(s): Garret Rieger + */ + +#include "graph.hh" +#include "../hb-ot-layout-gsubgpos.hh" + +#ifndef GRAPH_GSUBGPOS_CONTEXT_HH +#define GRAPH_GSUBGPOS_CONTEXT_HH + +namespace graph { + +struct Lookup; + +struct gsubgpos_graph_context_t +{ + hb_tag_t table_tag; + graph_t& graph; + hb_vector_t& buffer; + unsigned lookup_list_index; + hb_hashmap_t lookups; + + HB_INTERNAL gsubgpos_graph_context_t (hb_tag_t table_tag_, + graph_t& graph_, + hb_vector_t& buffer_); + + bool in_error () const + { + return buffer.in_error (); + } + + private: + HB_INTERNAL unsigned num_non_ext_subtables (); +}; + +} + +#endif // GRAPH_GSUBGPOS_CONTEXT diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index e3c003cae..8d6e3a343 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -27,6 +27,8 @@ #include "graph.hh" #include "../hb-ot-layout-gsubgpos.hh" #include "../OT/Layout/GSUB/ExtensionSubst.hh" +#include "gsubgpos-context.hh" +#include "pairpos-graph.hh" #ifndef GRAPH_GSUBGPOS_GRAPH_HH #define GRAPH_GSUBGPOS_GRAPH_HH @@ -35,27 +37,6 @@ namespace graph { struct Lookup; -struct make_extension_context_t -{ - hb_tag_t table_tag; - graph_t& graph; - hb_vector_t& buffer; - unsigned lookup_list_index; - hb_hashmap_t lookups; - - HB_INTERNAL make_extension_context_t (hb_tag_t table_tag_, - graph_t& graph_, - hb_vector_t& buffer_); - - bool in_error () const - { - return buffer.in_error (); - } - - private: - HB_INTERNAL unsigned num_non_ext_subtables (); -}; - template struct ExtensionFormat1 : public OT::ExtensionFormat1 { @@ -65,6 +46,16 @@ struct ExtensionFormat1 : public OT::ExtensionFormat1 this->extensionLookupType = type; this->extensionOffset = 0; } + + unsigned get_lookup_type () const + { + return this->extensionLookupType; + } + + unsigned get_subtable_index (graph_t& graph, unsigned this_index) const + { + return graph.index_for_offset (this_index, &this->extensionOffset); + } }; struct Lookup : public OT::Lookup @@ -86,7 +77,7 @@ struct Lookup : public OT::Lookup return lookupType == extension_type (table_tag); } - bool make_extension (make_extension_context_t& c, + bool make_extension (gsubgpos_graph_context_t& c, unsigned this_index) { unsigned type = lookupType; @@ -115,7 +106,43 @@ struct Lookup : public OT::Lookup return true; } - bool make_subtable_extension (make_extension_context_t& c, + bool split_subtables_if_needed (gsubgpos_graph_context_t& c, + unsigned this_index) + { + unsigned type = lookupType; + bool is_ext = is_extension (c.table_tag); + + if (c.table_tag != HB_OT_TAG_GPOS) + return true; + + if (!is_ext && type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair) + return true; + + // TODO check subtable type. + for (unsigned i = 0; i < subTable.len; i++) + { + unsigned subtable_index = c.graph.index_for_offset (this_index, &subTable[i]); + if (is_ext) { + unsigned ext_subtable_index = subtable_index; + ExtensionFormat1* extension = + (ExtensionFormat1*) + c.graph.object (ext_subtable_index).head; + + subtable_index = extension->get_subtable_index (c.graph, ext_subtable_index); + type = extension->get_lookup_type (); + if (type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair) + continue; + } + + PairPos* pairPos = (PairPos*) c.graph.object (subtable_index).head; + // TODO sanitize + pairPos->split_subtables (c, subtable_index); + } + + return true; + } + + bool make_subtable_extension (gsubgpos_graph_context_t& c, unsigned lookup_index, unsigned subtable_index) { @@ -214,6 +241,9 @@ struct GSTAR : public OT::GSUBGPOS return len >= get_size (); } + // TODO(garretrieger): add find subtable's method, could be templated locate a specific + // subtable type, maybe take the lookup map as a starting point? + void find_lookups (graph_t& graph, hb_hashmap_t& lookups /* OUT */) { diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh new file mode 100644 index 000000000..09bb8cbf5 --- /dev/null +++ b/src/graph/pairpos-graph.hh @@ -0,0 +1,110 @@ +/* + * Copyright © 2022 Google, Inc. + * + * This is part of HarfBuzz, a text shaping library. + * + * Permission is hereby granted, without written agreement and without + * license or royalty fees, to use, copy, modify, and distribute this + * software and its documentation for any purpose, provided that the + * above copyright notice and the following two paragraphs appear in + * all copies of this software. + * + * IN NO EVENT SHALL THE COPYRIGHT HOLDER BE LIABLE TO ANY PARTY FOR + * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES + * ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS DOCUMENTATION, EVEN + * IF THE COPYRIGHT HOLDER HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH + * DAMAGE. + * + * THE COPYRIGHT HOLDER SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING, + * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS + * ON AN "AS IS" BASIS, AND THE COPYRIGHT HOLDER HAS NO OBLIGATION TO + * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. + * + * Google Author(s): Garret Rieger + */ + +#ifndef GRAPH_PAIRPOS_GRAPH_HH +#define GRAPH_PAIRPOS_GRAPH_HH + +#include "../OT/Layout/GPOS/PairPos.hh" +#include "../OT/Layout/GPOS/PosLookupSubTable.hh" + +namespace graph { + +struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 +{ + unsigned get_size () const + { + return OT::Layout::GPOS_impl::PairPosFormat1_3::min_size + + pairSet.get_size () - SmallTypes::size; + } + + bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + { + printf("Checking if pair pos %u needs splits...\n", this_index); + hb_set_t visited; + const unsigned base_size = get_size (); + printf(" base_size = %u\n", base_size); + unsigned accumulated = base_size; + // TODO: include coverage size + unsigned num_pair_sets = pairSet.len; + printf(" num_pair_sets = %u\n", num_pair_sets); + for (unsigned i = 0; i < pairSet.len; i++) + { + unsigned pair_set_index = pair_set_graph_index (c, this_index, i); + accumulated += c.graph.find_subgraph_size (pair_set_index, visited); + + if (accumulated > (1 << 16)) + { + // TODO: do the split + + printf(" PairPos split needed %u/%u\n", i, num_pair_sets); + accumulated = base_size; + } + } + + return true; + } + + private: + unsigned pair_set_graph_index (gsubgpos_graph_context_t& c, unsigned this_index, unsigned i) const + { + return c.graph.index_for_offset (this_index, &pairSet[i]); + } +}; + +struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4 +{ + bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + { + // TODO + return true; + } +}; + +struct PairPos : public OT::Layout::GPOS_impl::PairPos +{ + bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + { + unsigned format = u.format; + printf("PairPos::format = %u\n", format); + switch (u.format) { + case 1: + return ((PairPosFormat1*)(&u.format1))->split_subtables (c, this_index); + case 2: + return ((PairPosFormat2*)(&u.format2))->split_subtables (c, this_index); +#ifndef HB_NO_BORING_EXPANSION + case 3: + case 4: + // Don't split 24bit PairPos's. +#endif + default: + return true; + } + } +}; + +} + +#endif // GRAPH_PAIRPOS_GRAPH_HH diff --git a/src/harfbuzz-subset.cc b/src/harfbuzz-subset.cc index 1a3b458a9..b6d722515 100644 --- a/src/harfbuzz-subset.cc +++ b/src/harfbuzz-subset.cc @@ -1,4 +1,4 @@ -#include "graph/gsubgpos-graph.cc" +#include "gsubgpos-context.cc" #include "hb-aat-layout.cc" #include "hb-aat-map.cc" #include "hb-blob.cc" diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index ca629893b..31dfa0862 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -66,12 +66,34 @@ inline int compare_sizes (const void* a, const void* b) return 0; } +static inline +bool _presplit_subtables_if_needed (graph::gsubgpos_graph_context_t& ext_context) +{ + // Algorithm: + // 1. Scan all sub-tables and compute their sizes. + // 2. For any PairPos subtables that are >64kb: + // a. split into two more pieces that are <64kb + // b. De-dup subgraphs (optional) (can this be done during serialization?) + + // TODO: Add a split subtables method at the lookup level, it could scan it's subtables and split as + // needed. + // TODO: rename gsubgpos_graph_context_t to be more general. + + for (unsigned lookup_index : ext_context.lookups.keys ()) + { + graph::Lookup* lookup = ext_context.lookups.get(lookup_index); + lookup->split_subtables_if_needed (ext_context, lookup_index); + } + + return true; +} + /* * Analyze the lookups in a GSUB/GPOS table and decide if any should be promoted * to extension lookups. */ static inline -bool _promote_extensions_if_needed (graph::make_extension_context_t& ext_context) +bool _promote_extensions_if_needed (graph::gsubgpos_graph_context_t& ext_context) { // Simple Algorithm (v1, current): // 1. Calculate how many bytes each non-extension lookup consumes. @@ -293,10 +315,15 @@ hb_resolve_overflows (const T& packed, { if (recalculate_extensions) { - graph::make_extension_context_t ext_context (table_tag, sorted_graph, extension_buffer); + graph::gsubgpos_graph_context_t ext_context (table_tag, sorted_graph, extension_buffer); if (ext_context.in_error ()) return nullptr; + if (!_presplit_subtables_if_needed (ext_context)) { + DEBUG_MSG (SUBSET_REPACK, nullptr, "Subtable splitting failed."); + return nullptr; + } + if (!_promote_extensions_if_needed (ext_context)) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Extensions promotion failed."); return nullptr; diff --git a/src/meson.build b/src/meson.build index 27c75e688..631b124b4 100644 --- a/src/meson.build +++ b/src/meson.build @@ -345,7 +345,7 @@ hb_subset_sources = files( 'hb-subset-plan.cc', 'hb-subset-plan.hh', 'hb-subset-repacker.cc', - 'graph/gsubgpos-graph.cc', + 'graph/gsubgpos-context.cc', 'hb-subset.cc', 'hb-subset.hh', ) @@ -574,7 +574,7 @@ if get_option('tests').enabled() 'test-number': ['test-number.cc', 'hb-number.cc'], 'test-ot-tag': ['hb-ot-tag.cc'], 'test-priority-queue': ['test-priority-queue.cc', 'hb-static.cc'], - 'test-repacker': ['test-repacker.cc', 'hb-static.cc', 'graph/gsubgpos-graph.cc'], + 'test-repacker': ['test-repacker.cc', 'hb-static.cc', 'graph/gsubgpos-context.cc'], 'test-set': ['test-set.cc', 'hb-static.cc'], 'test-serialize': ['test-serialize.cc', 'hb-static.cc'], 'test-unicode-ranges': ['test-unicode-ranges.cc'], From bf0986c7d1263f28ac0aceb74de8e586a2add362 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 19:33:46 +0000 Subject: [PATCH 02/24] [repack] sketch splitting mechanism for PairPosFormat1. --- src/graph/pairpos-graph.hh | 60 +++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 09bb8cbf5..380453ade 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -34,40 +34,80 @@ namespace graph { struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 { - unsigned get_size () const - { - return OT::Layout::GPOS_impl::PairPosFormat1_3::min_size - + pairSet.get_size () - SmallTypes::size; - } - bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { printf("Checking if pair pos %u needs splits...\n", this_index); hb_set_t visited; - const unsigned base_size = get_size (); + const unsigned base_size = OT::Layout::GPOS_impl::PairPosFormat1_3::min_size; printf(" base_size = %u\n", base_size); unsigned accumulated = base_size; // TODO: include coverage size unsigned num_pair_sets = pairSet.len; printf(" num_pair_sets = %u\n", num_pair_sets); + hb_vector_t split_points; for (unsigned i = 0; i < pairSet.len; i++) { unsigned pair_set_index = pair_set_graph_index (c, this_index, i); accumulated += c.graph.find_subgraph_size (pair_set_index, visited); + accumulated += SmallTypes::size; // for PairSet offset. - if (accumulated > (1 << 16)) + if (accumulated > (1 << 15)) // TODO (1 << 16) { - // TODO: do the split - printf(" PairPos split needed %u/%u\n", i, num_pair_sets); + split_points.push (i); accumulated = base_size; } } + // TODO: do the split + do_split (c, this_index, split_points); + return true; } private: + + // Split this PairPos into two or more PairPos's. split_points defines + // the indices (first index to include in the new table) to split at. + // Returns the object id's of the newly created PairPos subtables. + hb_vector_t do_split (gsubgpos_graph_context_t& c, + unsigned this_index, + const hb_vector_t split_points) + { + hb_vector_t new_objects; + if (!split_points) + return new_objects; + + for (unsigned i = 0; i < split_points.length; i++) + { + unsigned start = split_points[i]; + unsigned end = (i < split_points.length - 1) ? split_points[i + 1] : pairSet.len; + new_objects.push (clone_range (c, this_index, start, end)); + // TODO error checking. + } + + shrink (split_points[0]); + + return new_objects; + } + + void shrink (unsigned size) + { + printf(" shrink to [0, %u).\n", size); + // TODO + } + + // Create a new PairPos including PairSet's from start (inclusive) to end (exclusive). + // Returns object id of the new object. + unsigned clone_range (gsubgpos_graph_context_t& c, + unsigned this_index, + unsigned start, unsigned end) const + { + printf(" cloning range [%u, %u).\n", start, end); + // TODO + return -1; + } + unsigned pair_set_graph_index (gsubgpos_graph_context_t& c, unsigned this_index, unsigned i) const { return c.graph.index_for_offset (this_index, &pairSet[i]); From 8e5fffc44a9606b0aea459d3668762c127430101 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 20:00:00 +0000 Subject: [PATCH 03/24] [repack] add helper to create new nodes. Switch to malloc'ing each node individually rather than trying to guess up front the total buffer space needed. --- src/graph/gsubgpos-context.cc | 23 +++++++++++++++++------ src/graph/gsubgpos-context.hh | 12 +++++++----- src/graph/gsubgpos-graph.hh | 11 +++++------ src/hb-repacker.hh | 7 +------ 4 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/graph/gsubgpos-context.cc b/src/graph/gsubgpos-context.cc index f71c73aa1..e0ff6ff85 100644 --- a/src/graph/gsubgpos-context.cc +++ b/src/graph/gsubgpos-context.cc @@ -29,22 +29,33 @@ namespace graph { gsubgpos_graph_context_t::gsubgpos_graph_context_t (hb_tag_t table_tag_, - graph_t& graph_, - hb_vector_t& buffer_) + graph_t& graph_) : table_tag (table_tag_), graph (graph_), - buffer (buffer_), lookup_list_index (0), - lookups () + lookups (), + buffers () { + if (table_tag_ != HB_OT_TAG_GPOS + && table_tag_ != HB_OT_TAG_GSUB) + return; + GSTAR* gstar = graph::GSTAR::graph_to_gstar (graph_); if (gstar) { gstar->find_lookups (graph, lookups); lookup_list_index = gstar->get_lookup_list_index (graph_); } +} + +unsigned gsubgpos_graph_context_t::create_node (unsigned size) +{ + char* buffer = (char*) hb_calloc (1, size); + if (!buffer) + return -1; + + buffers.push (buffer); - unsigned extension_size = OT::ExtensionFormat1::static_size; - buffer.alloc (num_non_ext_subtables () * extension_size); + return graph.new_node (buffer, buffer + size); } unsigned gsubgpos_graph_context_t::num_non_ext_subtables () { diff --git a/src/graph/gsubgpos-context.hh b/src/graph/gsubgpos-context.hh index ff8f29e0d..89649f197 100644 --- a/src/graph/gsubgpos-context.hh +++ b/src/graph/gsubgpos-context.hh @@ -38,19 +38,21 @@ struct gsubgpos_graph_context_t { hb_tag_t table_tag; graph_t& graph; - hb_vector_t& buffer; unsigned lookup_list_index; hb_hashmap_t lookups; + hb_vector_t buffers; HB_INTERNAL gsubgpos_graph_context_t (hb_tag_t table_tag_, - graph_t& graph_, - hb_vector_t& buffer_); + graph_t& graph_); - bool in_error () const + ~gsubgpos_graph_context_t () { - return buffer.in_error (); + for (char* b : buffers) + hb_free (b); } + HB_INTERNAL unsigned create_node (unsigned size); + private: HB_INTERNAL unsigned num_non_ext_subtables (); }; diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index 8d6e3a343..d35ea8df3 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -148,17 +148,16 @@ struct Lookup : public OT::Lookup { unsigned type = lookupType; unsigned extension_size = OT::ExtensionFormat1::static_size; - unsigned start = c.buffer.length; - unsigned end = start + extension_size; - if (!c.buffer.resize (c.buffer.length + extension_size)) + + + unsigned ext_index = c.create_node (extension_size); + if (ext_index == (unsigned) -1) return false; ExtensionFormat1* extension = - (ExtensionFormat1*) &c.buffer[start]; + (ExtensionFormat1*) c.graph.object (ext_index).head; extension->reset (type); - unsigned ext_index = c.graph.new_node (&c.buffer.arrayZ[start], - &c.buffer.arrayZ[end]); if (ext_index == (unsigned) -1) return false; auto& lookup_vertex = c.graph.vertices_[lookup_index]; diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index 31dfa0862..1ee5985be 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -307,18 +307,13 @@ hb_resolve_overflows (const T& packed, return graph::serialize (sorted_graph); } - hb_vector_t extension_buffer; // Needs to live until serialization is done. - + graph::gsubgpos_graph_context_t ext_context (table_tag, sorted_graph); if ((table_tag == HB_OT_TAG_GPOS || table_tag == HB_OT_TAG_GSUB) && will_overflow) { if (recalculate_extensions) { - graph::gsubgpos_graph_context_t ext_context (table_tag, sorted_graph, extension_buffer); - if (ext_context.in_error ()) - return nullptr; - if (!_presplit_subtables_if_needed (ext_context)) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Subtable splitting failed."); return nullptr; From 8d63f60e5b07985f16bdf619f954665e5eee77f4 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 20:36:20 +0000 Subject: [PATCH 04/24] [repack] add graph_t::move_child helper function. --- src/graph/graph.hh | 41 ++++++++++++++++++++++++++++++++++++++ src/graph/pairpos-graph.hh | 27 +++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 9ebb57d15..435e51a9d 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -80,6 +80,16 @@ struct graph_t } } + void remove_real_link (unsigned child_index) + { + for (unsigned i = 0; i < obj.real_links.length; i++) + { + if (obj.real_links[i].objidx != child_index) continue; + obj.real_links.remove (i); + break; + } + } + void remap_parents (const hb_vector_t& id_map) { for (unsigned i = 0; i < parents.length; i++) @@ -515,6 +525,37 @@ struct graph_t } } + /* + * Moves the child of old_parent_idx pointed to by old_offset to a new + * vertex at the new_offset. + */ + template + void move_child (unsigned old_parent_idx, + const O* old_offset, + unsigned new_parent_idx, + const O* new_offset) + { + auto& old_v = vertices_[old_parent_idx]; + auto& new_v = vertices_[new_parent_idx]; + + unsigned child_id = index_for_offset (old_parent_idx, + old_offset); + + auto* new_link = new_v.obj.real_links.push (); + new_link->width = O::static_size; + new_link->objidx = child_id; + new_link->is_signed = 0; + new_link->whence = 0; + new_link->position = (const char*) old_offset - (const char*) new_v.obj.head; + new_link->bias = 0; + + auto& child = vertices_[child_id]; + child.parents.push (new_parent_idx); + + old_v.remove_real_link (child_id); + child.remove_parent (old_parent_idx); + } + /* * duplicates all nodes in the subgraph reachable from node_idx. Does not re-assign * links. index_map is updated with mappings from old id to new id. If a duplication has already diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 380453ade..4576330e4 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -104,8 +104,31 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3::min_size + + num_pair_sets * SmallTypes::size; + + unsigned pair_pos_prime_id = c.create_node (prime_size); + if (pair_pos_prime_id == (unsigned) -1) return -1; + + PairPosFormat1* pair_pos_prime = (PairPosFormat1*) c.graph.object (pair_pos_prime_id).head; + pair_pos_prime->format = this->format; + pair_pos_prime->valueFormat[0] = this->valueFormat[0]; + pair_pos_prime->valueFormat[1] = this->valueFormat[1]; + pair_pos_prime->pairSet.len = num_pair_sets; + + for (unsigned i = start; i < end; i++) + { + c.graph.move_child<> (this_index, + &pairSet[i], + pair_pos_prime_id, + &pair_pos_prime->pairSet[i - start]); + } + + + // TODO: serialize a new coverage table. + return pair_pos_prime_id; } unsigned pair_set_graph_index (gsubgpos_graph_context_t& c, unsigned this_index, unsigned i) const From 29cb8818cde8fa928e5a3d0270a7160a4d7f9c2d Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 21:02:48 +0000 Subject: [PATCH 05/24] [repacker] new coverage serialization in PairPosFormat1. --- src/graph/graph.hh | 2 +- src/graph/pairpos-graph.hh | 30 +++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 435e51a9d..234a26159 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -552,7 +552,7 @@ struct graph_t auto& child = vertices_[child_id]; child.parents.push (new_parent_idx); - old_v.remove_real_link (child_id); + old_v.remove_real_link (child_id); // TODO: needs to include offset to ensure correct one removed child.remove_parent (old_parent_idx); } diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 4576330e4..ed1ba0d55 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -127,7 +127,35 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3iter (), hb_range ()) + | hb_filter ([&] (hb_pair_t p) { + return p.second >= start && p.second < end; + }) + | hb_map_retains_sorting (hb_first) + ; + + unsigned coverage_prime_id = c.create_node (coverage_size); + auto& coverage_prime_obj = c.graph.vertices_[coverage_prime_id].obj; + hb_serialize_context_t serializer = hb_serialize_context_t (coverage_prime_obj.head, + coverage_size); + Coverage_serialize (&serializer, new_coverage); + serializer.end_serialize (); + if (serializer.in_error ()) + return -1; + + hb_blob_ptr_t coverage_copy = serializer.copy_blob (); + memcpy (coverage_prime_obj.head, + coverage_copy.get (), + coverage_copy.get_length ()); + coverage_prime_obj.tail = coverage_prime_obj.head + coverage_copy.get_length (); + // TODO: add coverage as a child + return pair_pos_prime_id; } From 510b8ab1012d7aafd77ce99e6679d968a6a56c60 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 27 Jul 2022 23:30:20 +0000 Subject: [PATCH 06/24] [repack] link new coverage in PairPosFormat1::clone_range. --- src/graph/pairpos-graph.hh | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index ed1ba0d55..c6446cf33 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -86,14 +86,24 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3= old_count) + return; + + pairSet.len = count; + c.graph.vertices_[this_index].obj.tail -= (count - old_count) * SmallTypes::size; + + // TODO } @@ -141,8 +151,8 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 coverage_copy = serializer.copy_blob (); - memcpy (coverage_prime_obj.head, + memcpy (coverage_prime_vertex.obj.head, coverage_copy.get (), coverage_copy.get_length ()); - coverage_prime_obj.tail = coverage_prime_obj.head + coverage_copy.get_length (); - // TODO: add coverage as a child + coverage_prime_vertex.obj.tail = coverage_prime_vertex.obj.head + coverage_copy.get_length (); + + auto* coverage_link = c.graph.vertices_[pair_pos_prime_id].obj.real_links.push (); + coverage_link->width = SmallTypes::size; + coverage_link->objidx = coverage_prime_id; + coverage_link->position = 2; + coverage_prime_vertex.parents.push (pair_pos_prime_id); return pair_pos_prime_id; } From 5024d4de679d5ae0e4ec842c70e8e7a4ad7603f5 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 00:55:36 +0000 Subject: [PATCH 07/24] [repack] more PairPos split implementation. --- src/graph/gsubgpos-context.hh | 5 ++ src/graph/gsubgpos-graph.hh | 82 +++++++++++++++++++++++------- src/graph/pairpos-graph.hh | 94 ++++++++++++++++++++++++----------- 3 files changed, 135 insertions(+), 46 deletions(-) diff --git a/src/graph/gsubgpos-context.hh b/src/graph/gsubgpos-context.hh index 89649f197..49b24198f 100644 --- a/src/graph/gsubgpos-context.hh +++ b/src/graph/gsubgpos-context.hh @@ -53,6 +53,11 @@ struct gsubgpos_graph_context_t HB_INTERNAL unsigned create_node (unsigned size); + void add_buffer (char* buffer) + { + buffers.push (buffer); + } + private: HB_INTERNAL unsigned num_non_ext_subtables (); }; diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index d35ea8df3..88fb66d1b 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -119,6 +119,7 @@ struct Lookup : public OT::Lookup return true; // TODO check subtable type. + hb_vector_t all_new_subtables; for (unsigned i = 0; i < subTable.len; i++) { unsigned subtable_index = c.graph.index_for_offset (this_index, &subTable[i]); @@ -136,29 +137,83 @@ struct Lookup : public OT::Lookup PairPos* pairPos = (PairPos*) c.graph.object (subtable_index).head; // TODO sanitize - pairPos->split_subtables (c, subtable_index); + hb_vector_t new_sub_tables = pairPos->split_subtables (c, subtable_index); + if (new_sub_tables.in_error ()) return false; + + new_sub_tables.iter() | hb_sink (all_new_subtables); } + add_sub_tables (c, this_index, all_new_subtables); + return true; } - bool make_subtable_extension (gsubgpos_graph_context_t& c, - unsigned lookup_index, - unsigned subtable_index) + void add_sub_tables (gsubgpos_graph_context_t& c, + unsigned this_index, + hb_vector_t subtable_indices) { - unsigned type = lookupType; - unsigned extension_size = OT::ExtensionFormat1::static_size; + // bool is_ext = is_extension (c.table_tag); + auto& v = c.graph.vertices_[this_index]; + + size_t new_size = v.table_size () + + subtable_indices.length * OT::Offset16::static_size; + char* buffer = (char*) hb_calloc (1, new_size); + c.add_buffer (buffer); + v.obj.head = buffer; + v.obj.tail = buffer + new_size; + + memcpy (buffer, v.obj.head, v.table_size()); + + Lookup* new_lookup = (Lookup*) buffer; + new_lookup->subTable.len = subTable.len + subtable_indices.length; + + unsigned offset_index = subTable.len; + for (unsigned subtable_id : subtable_indices) + { + // TODO: add extension subtable if needed + auto* link = v.obj.real_links.push (); + link->width = 2; + link->objidx = subtable_id; + link->position = (char*) &new_lookup->subTable[offset_index++] - + (char*) &new_lookup->subTable[0]; + c.graph.vertices_[subtable_id].parents.push (this_index); + } + } + unsigned create_extension_subtable (gsubgpos_graph_context_t& c, + unsigned lookup_index, + unsigned subtable_index, + unsigned type) + { + unsigned extension_size = OT::ExtensionFormat1::static_size; unsigned ext_index = c.create_node (extension_size); if (ext_index == (unsigned) -1) - return false; + return -1; + auto& ext_vertex = c.graph.vertices_[ext_index]; ExtensionFormat1* extension = - (ExtensionFormat1*) c.graph.object (ext_index).head; + (ExtensionFormat1*) ext_vertex.obj.head; extension->reset (type); - if (ext_index == (unsigned) -1) return false; + // Make extension point at the subtable. + auto* l = ext_vertex.obj.real_links.push (); + + l->width = 4; + l->objidx = subtable_index; + l->position = 4; + + return ext_index; + } + + bool make_subtable_extension (gsubgpos_graph_context_t& c, + unsigned lookup_index, + unsigned subtable_index) + { + unsigned type = lookupType; + + unsigned ext_index = create_extension_subtable(c, lookup_index, subtable_index, type); + if (ext_index == (unsigned) -1) + return false; auto& lookup_vertex = c.graph.vertices_[lookup_index]; for (auto& l : lookup_vertex.obj.real_links.writer ()) @@ -171,15 +226,6 @@ struct Lookup : public OT::Lookup // Make extension point at the subtable. auto& ext_vertex = c.graph.vertices_[ext_index]; auto& subtable_vertex = c.graph.vertices_[subtable_index]; - auto* l = ext_vertex.obj.real_links.push (); - - l->width = 4; - l->objidx = subtable_index; - l->is_signed = 0; - l->whence = 0; - l->position = 4; - l->bias = 0; - ext_vertex.parents.push (lookup_index); subtable_vertex.remap_parent (lookup_index, ext_index); diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index c6446cf33..5a595e940 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -34,7 +34,7 @@ namespace graph { struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 { - bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { printf("Checking if pair pos %u needs splits...\n", this_index); hb_set_t visited; @@ -59,10 +59,7 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3= old_count) - return; + return true; pairSet.len = count; c.graph.vertices_[this_index].obj.tail -= (count - old_count) * SmallTypes::size; - // TODO + unsigned coverage_id = c.graph.index_for_offset (this_index, &coverage); + unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); + OT::Layout::Common::Coverage* coverage_table = + (OT::Layout::Common::Coverage*) c.graph.object (coverage_id).head; + + auto new_coverage = + + hb_zip (coverage_table->iter (), hb_range ()) + | hb_filter ([&] (hb_pair_t p) { + return p.second < count; + }) + | hb_map_retains_sorting (hb_first) + ; + + return make_coverage (c, new_coverage, coverage_id, coverage_size); } // Create a new PairPos including PairSet's from start (inclusive) to end (exclusive). @@ -136,7 +157,6 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3pairSet[i - start]); } - unsigned coverage_id = c.graph.index_for_offset (this_index, &coverage); unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); OT::Layout::Common::Coverage* coverage_table = @@ -150,21 +170,11 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 coverage_copy = serializer.copy_blob (); - memcpy (coverage_prime_vertex.obj.head, - coverage_copy.get (), - coverage_copy.get_length ()); - coverage_prime_vertex.obj.tail = coverage_prime_vertex.obj.head + coverage_copy.get_length (); - auto* coverage_link = c.graph.vertices_[pair_pos_prime_id].obj.real_links.push (); coverage_link->width = SmallTypes::size; coverage_link->objidx = coverage_prime_id; @@ -174,6 +184,34 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 + bool make_coverage (gsubgpos_graph_context_t& c, + It glyphs, + unsigned dest_obj, + unsigned max_size) const + { + char* buffer = (char*) hb_calloc (1, max_size); + hb_serialize_context_t serializer = hb_serialize_context_t (buffer, + max_size); + Coverage_serialize (&serializer, glyphs); + serializer.end_serialize (); + if (serializer.in_error ()) + { + hb_free (buffer); + return false; + } + + hb_bytes_t coverage_copy = serializer.copy_bytes (); + c.add_buffer ((char *) coverage_copy.arrayZ); // Give ownership to the context, it will cleanup the buffer. + + auto& obj = c.graph.vertices_[dest_obj].obj; + obj.head = (char *) coverage_copy.arrayZ; + obj.tail = obj.head + coverage_copy.length; + + hb_free (buffer); + return true; + } + unsigned pair_set_graph_index (gsubgpos_graph_context_t& c, unsigned this_index, unsigned i) const { return c.graph.index_for_offset (this_index, &pairSet[i]); @@ -182,16 +220,16 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 { - bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { // TODO - return true; + return hb_vector_t (); } }; struct PairPos : public OT::Layout::GPOS_impl::PairPos { - bool split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { unsigned format = u.format; printf("PairPos::format = %u\n", format); @@ -206,7 +244,7 @@ struct PairPos : public OT::Layout::GPOS_impl::PairPos // Don't split 24bit PairPos's. #endif default: - return true; + return hb_vector_t (); } } }; From d589ce68ea80b22eebdf29efca438b97f97c64a3 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 01:04:37 +0000 Subject: [PATCH 08/24] [repacker] add extension subtable when needed while adding new PairPos table's. --- src/graph/gsubgpos-graph.hh | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index 88fb66d1b..1fa4247b6 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -142,16 +142,17 @@ struct Lookup : public OT::Lookup + new_sub_tables.iter() | hb_sink (all_new_subtables); } - add_sub_tables (c, this_index, all_new_subtables); + add_sub_tables (c, this_index, type, all_new_subtables); return true; } void add_sub_tables (gsubgpos_graph_context_t& c, unsigned this_index, + unsigned type, hb_vector_t subtable_indices) { - // bool is_ext = is_extension (c.table_tag); + bool is_ext = is_extension (c.table_tag); auto& v = c.graph.vertices_[this_index]; size_t new_size = v.table_size () @@ -169,7 +170,13 @@ struct Lookup : public OT::Lookup unsigned offset_index = subTable.len; for (unsigned subtable_id : subtable_indices) { - // TODO: add extension subtable if needed + if (is_ext) + { + unsigned ext_id = create_extension_subtable (c, subtable_id, type); + c.graph.vertices_[subtable_id].parents.push (ext_id); + subtable_id = ext_id; + } + auto* link = v.obj.real_links.push (); link->width = 2; link->objidx = subtable_id; @@ -180,7 +187,6 @@ struct Lookup : public OT::Lookup } unsigned create_extension_subtable (gsubgpos_graph_context_t& c, - unsigned lookup_index, unsigned subtable_index, unsigned type) { @@ -211,7 +217,7 @@ struct Lookup : public OT::Lookup { unsigned type = lookupType; - unsigned ext_index = create_extension_subtable(c, lookup_index, subtable_index, type); + unsigned ext_index = create_extension_subtable(c, subtable_index, type); if (ext_index == (unsigned) -1) return false; From a5c2c8c1319fcc4d776496ebed05ed6c69060a96 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 01:27:55 +0000 Subject: [PATCH 09/24] [repack] fix incorrect shrink. --- src/graph/pairpos-graph.hh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 5a595e940..8d95a06ff 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -51,7 +51,7 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 (1 << 15)) // TODO (1 << 16) + if (accumulated > (1 << 13)) // TODO (1 << 16) { printf(" PairPos split needed %u/%u\n", i, num_pair_sets); split_points.push (i); @@ -109,8 +109,7 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 Date: Thu, 28 Jul 2022 20:17:36 +0000 Subject: [PATCH 10/24] [repacker] bug fixes. --- src/graph/graph.hh | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 234a26159..7dc5413f8 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -80,13 +80,19 @@ struct graph_t } } - void remove_real_link (unsigned child_index) + void remove_real_link (unsigned child_index, const void* offset) { for (unsigned i = 0; i < obj.real_links.length; i++) { - if (obj.real_links[i].objidx != child_index) continue; + auto& link = obj.real_links[i]; + if (link.objidx != child_index) + continue; + + if ((obj.head + link.position) != offset) + continue; + obj.real_links.remove (i); - break; + return; } } @@ -544,15 +550,12 @@ struct graph_t auto* new_link = new_v.obj.real_links.push (); new_link->width = O::static_size; new_link->objidx = child_id; - new_link->is_signed = 0; - new_link->whence = 0; - new_link->position = (const char*) old_offset - (const char*) new_v.obj.head; - new_link->bias = 0; + new_link->position = (const char*) new_offset - (const char*) new_v.obj.head; auto& child = vertices_[child_id]; child.parents.push (new_parent_idx); - old_v.remove_real_link (child_id); // TODO: needs to include offset to ensure correct one removed + old_v.remove_real_link (child_id, old_offset); // TODO: needs to include offset to ensure correct one removed child.remove_parent (old_parent_idx); } From 65afed047db086bf28aef47ba43e6983ba088ba1 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 20:54:28 +0000 Subject: [PATCH 11/24] [repacker] more bug fixes. --- src/graph/graph.hh | 14 ++++++++++++++ src/graph/gsubgpos-graph.hh | 8 ++++---- src/hb-repacker.hh | 4 ++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 7dc5413f8..b0b6df71a 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -228,6 +228,17 @@ struct graph_t return vertices_[i].obj; } + /* + * Generates a new topological sorting of graph ordered by the shortest + * distance to each node if positions are marked as invalid. + */ + void sort_shortest_distance_if_needed () + { + if (!positions_invalid) return; + sort_shortest_distance (); + } + + /* * Generates a new topological sorting of graph ordered by the shortest * distance to each node. @@ -541,6 +552,9 @@ struct graph_t unsigned new_parent_idx, const O* new_offset) { + distance_invalid = true; + positions_invalid = true; + auto& old_v = vertices_[old_parent_idx]; auto& new_v = vertices_[new_parent_idx]; diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index 1fa4247b6..de7cc4992 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -159,14 +159,14 @@ struct Lookup : public OT::Lookup + subtable_indices.length * OT::Offset16::static_size; char* buffer = (char*) hb_calloc (1, new_size); c.add_buffer (buffer); + memcpy (buffer, v.obj.head, v.table_size()); + v.obj.head = buffer; v.obj.tail = buffer + new_size; - memcpy (buffer, v.obj.head, v.table_size()); - Lookup* new_lookup = (Lookup*) buffer; - new_lookup->subTable.len = subTable.len + subtable_indices.length; + new_lookup->subTable.len = subTable.len + subtable_indices.length; unsigned offset_index = subTable.len; for (unsigned subtable_id : subtable_indices) { @@ -181,7 +181,7 @@ struct Lookup : public OT::Lookup link->width = 2; link->objidx = subtable_id; link->position = (char*) &new_lookup->subTable[offset_index++] - - (char*) &new_lookup->subTable[0]; + (char*) new_lookup; c.graph.vertices_[subtable_id].parents.push (this_index); } } diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index 1ee5985be..e80360266 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -314,11 +314,13 @@ hb_resolve_overflows (const T& packed, { if (recalculate_extensions) { + DEBUG_MSG (SUBSET_REPACK, nullptr, "Splitting subtables if needed."); if (!_presplit_subtables_if_needed (ext_context)) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Subtable splitting failed."); return nullptr; } + DEBUG_MSG (SUBSET_REPACK, nullptr, "Promoting lookups to extensions if needed."); if (!_promote_extensions_if_needed (ext_context)) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Extensions promotion failed."); return nullptr; @@ -328,6 +330,8 @@ hb_resolve_overflows (const T& packed, DEBUG_MSG (SUBSET_REPACK, nullptr, "Assigning spaces to 32 bit subgraphs."); if (sorted_graph.assign_spaces ()) sorted_graph.sort_shortest_distance (); + else + sorted_graph.sort_shortest_distance_if_needed (); } unsigned round = 0; From f1bfb6585f96839f45633f856c290ebb3a0da1ea Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 21:01:41 +0000 Subject: [PATCH 12/24] [repacker] cleanup debug prints. --- src/graph/pairpos-graph.hh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 8d95a06ff..777867c0a 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -36,14 +36,10 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { - printf("Checking if pair pos %u needs splits...\n", this_index); hb_set_t visited; const unsigned base_size = OT::Layout::GPOS_impl::PairPosFormat1_3::min_size; - printf(" base_size = %u\n", base_size); unsigned accumulated = base_size; // TODO: include coverage size - unsigned num_pair_sets = pairSet.len; - printf(" num_pair_sets = %u\n", num_pair_sets); hb_vector_t split_points; for (unsigned i = 0; i < pairSet.len; i++) { @@ -51,9 +47,8 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 (1 << 13)) // TODO (1 << 16) + if (accumulated > (1 << 16)) { - printf(" PairPos split needed %u/%u\n", i, num_pair_sets); split_points.push (i); accumulated = base_size; } @@ -103,7 +98,10 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3= old_count) return true; @@ -133,7 +131,8 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3::min_size @@ -230,8 +229,6 @@ struct PairPos : public OT::Layout::GPOS_impl::PairPos { hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { - unsigned format = u.format; - printf("PairPos::format = %u\n", format); switch (u.format) { case 1: return ((PairPosFormat1*)(&u.format1))->split_subtables (c, this_index); From 4e7360f78dbf70b95a98713e15ebb1c1a40687cc Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 28 Jul 2022 22:56:47 +0000 Subject: [PATCH 13/24] [repacker] begin adding tests for PairPosFormat1 splitting. --- src/test-repacker.cc | 214 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 175 insertions(+), 39 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index 1acd75ac0..8d7496072 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -79,11 +79,115 @@ static void add_wide_offset (unsigned id, c->add_link (*offset, id); } +static void add_gsubgpos_header (unsigned lookup_list, + hb_serialize_context_t* c) +{ + char header[] = { + 0, 1, // major + 0, 0, // minor + 0, 0, // script list + 0, 0, // feature list + }; + + start_object (header, 8, c); + add_offset (lookup_list, c); + c->pop_pack (false); +} + +static unsigned add_lookup_list (const unsigned* lookups, + char count, + hb_serialize_context_t* c) +{ + char lookup_count[] = {0, count}; + start_object ((char *) &lookup_count, 2, c); + + for (int i = 0; i < count; i++) + add_offset (lookups[i], c); + + return c->pop_pack (false); +} + +static void start_lookup (int8_t type, + int8_t num_subtables, + hb_serialize_context_t* c) +{ + char lookup[] = { + 0, type, // type + 0, 0, // flag + 0, num_subtables, // num subtables + }; + + start_object (lookup, 6, c); +} + +static unsigned finish_lookup (hb_serialize_context_t* c) +{ + char filter[] = {0, 0}; + extend (filter, 2, c); + return c->pop_pack (false); +} + +static unsigned add_extension (unsigned child, + uint8_t type, + hb_serialize_context_t* c) +{ + char ext[] = { + 0, 1, + 0, (char) type, + }; + + start_object (ext, 4, c); + add_wide_offset (child, c); + + return c->pop_pack (false); + +} + +static unsigned add_coverage (char start, char end, + hb_serialize_context_t* c) +{ + char header[] = { + 0, 2, // format + 0, 1, // range count + 0, start, // start + 0, end, // end + 0, 0, + }; + + return add_object (header, 10, c); +} + +static unsigned add_pair_pos_1 (unsigned* pair_sets, + char count, + unsigned coverage, + hb_serialize_context_t* c) +{ + char format[] = { + 0, 1 + }; + + start_object (format, 2, c); + add_offset (coverage, c); + + char value_format[] = { + 0, 0, + 0, 0, + 0, count, + }; + extend (value_format, 6, c); + + for (char i = 0; i < count; i++) + add_offset (pair_sets[(unsigned) i], c); + + return c->pop_pack (false); +} + static void run_resolve_overflow_test (const char* name, hb_serialize_context_t& overflowing, hb_serialize_context_t& expected, unsigned num_iterations = 0, - bool recalculate_extensions = false) + bool recalculate_extensions = false, + hb_tag_t tag = HB_TAG ('G', 'S', 'U', 'B')) { printf (">>> Testing overflowing resolution for %s\n", name); @@ -93,7 +197,7 @@ static void run_resolve_overflow_test (const char* name, assert (overflowing.offset_overflow ()); hb_blob_t* out = hb_resolve_overflows (overflowing.object_graph (), - HB_TAG ('G', 'S', 'U', 'B'), + tag, num_iterations, recalculate_extensions); assert (out); @@ -884,7 +988,6 @@ populate_serializer_with_24_and_32_bit_offsets (hb_serialize_context_t* c) c->end_serialize(); } - static void populate_serializer_with_extension_promotion (hb_serialize_context_t* c, int num_extensions = 0) @@ -906,31 +1009,19 @@ populate_serializer_with_extension_promotion (hb_serialize_context_t* c, i >= (num_lookups - num_extensions) * 2; i--) { - char ext[] = { - 0, 1, - 0, 5 - }; - - unsigned ext_index = i - (num_lookups - num_extensions) * 2; // 5 - unsigned subtable_index = num_subtables - ext_index - 1; // 10 - 5 - 1 = 4 - - start_object (ext, 4, c); - add_wide_offset (subtables[subtable_index], c); - - extensions[i] = c->pop_pack (false); + unsigned ext_index = i - (num_lookups - num_extensions) * 2; + unsigned subtable_index = num_subtables - ext_index - 1; + extensions[i] = add_extension (subtables[subtable_index], 5, c); } for (int i = num_lookups - 1; i >= 0; i--) { bool is_ext = (i >= (num_lookups - num_extensions)); - char lookup[] = { - 0, is_ext ? (char) 7 : (char) 5, // type - 0, 0, // flag - 0, 2, // num subtables - }; + start_lookup (is_ext ? (char) 7 : (char) 5, + 2, + c); - start_object (lookup, 6, c); if (is_ext) { add_offset (extensions[i * 2], c); add_offset (extensions[i * 2 + 1], c); @@ -939,30 +1030,42 @@ populate_serializer_with_extension_promotion (hb_serialize_context_t* c, add_offset (subtables[i * 2 + 1], c); } - char filter[] = {0, 0}; - extend (filter, 2, c); - - lookups[i] = c->pop_pack (false); + lookups[i] = finish_lookup (c); } - char lookup_count[] = {0, num_lookups}; - start_object ((char *) &lookup_count, 2, c); + unsigned lookup_list = add_lookup_list (lookups, num_lookups, c); - for (int i = 0; i < num_lookups; i++) - add_offset (lookups[i], c); + add_gsubgpos_header (lookup_list, c); - unsigned lookup_list = c->pop_pack (false); + c->end_serialize(); +} - char gsub_header[] = { - 0, 1, // major - 0, 0, // minor - 0, 0, // script list - 0, 0, // feature list - }; +static void +populate_serializer_with_large_pair_pos_1 (hb_serialize_context_t* c) +{ + std::string large_string(60000, 'a'); + c->start_serialize (); - start_object (gsub_header, 8, c); - add_offset (lookup_list, c); - c->pop_pack (false); + unsigned pair_set[4]; + for (int i = 3; i >= 0; i--) + pair_set[i] = add_object (large_string.c_str (), 30000, c); + + unsigned coverage = add_coverage (0, 3, c); + + unsigned pair_pos_2 = add_object (large_string.c_str(), 200, c); + + unsigned pair_pos_1 = add_pair_pos_1 (pair_set, 4, coverage, c); + + start_lookup (2, 2, c); + + add_offset (pair_pos_1, c); + add_offset (pair_pos_2, c); + + unsigned lookup = finish_lookup (c); + + unsigned lookup_list = add_lookup_list (&lookup, 1, c); + + add_gsubgpos_header (lookup_list, c); c->end_serialize(); } @@ -1336,6 +1439,30 @@ static void test_resolve_with_extension_promotion () free (expected_buffer); } +static void test_resolve_with_basic_pair_pos_1_split () +{ + size_t buffer_size = 200000; + void* buffer = malloc (buffer_size); + assert (buffer); + hb_serialize_context_t c (buffer, buffer_size); + populate_serializer_with_large_pair_pos_1 (&c); + + void* expected_buffer = malloc (buffer_size); + assert (expected_buffer); + hb_serialize_context_t e (expected_buffer, buffer_size); + populate_serializer_with_large_pair_pos_1 (&e); + + run_resolve_overflow_test ("test_resolve_with_basic_pair_pos_1_split", + c, + e, + 20, + true, + HB_TAG('G', 'P', 'O', 'S')); + free (buffer); + free (expected_buffer); +} + + static void test_resolve_overflows_via_splitting_spaces () { size_t buffer_size = 160000; @@ -1461,6 +1588,7 @@ test_shared_node_with_virtual_links () int main (int argc, char **argv) { + if (1) { test_serialize (); test_sort_shortest (); test_will_overflow_1 (); @@ -1483,6 +1611,14 @@ main (int argc, char **argv) test_virtual_link (); test_shared_node_with_virtual_links (); test_resolve_with_extension_promotion (); + } + + test_resolve_with_basic_pair_pos_1_split (); + + // TODO: + // - basic splitting case. + // - splitting with extensions. + // TODO(grieger): test with extensions already mixed in as well. // TODO(grieger): test two layer ext promotion setup. // TODO(grieger): test sorting by subtables per byte in ext. promotion. From fb3f6ad7c020fcf9553d21916b04089a243a0bcd Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 00:25:19 +0000 Subject: [PATCH 14/24] [repacker] ensure lookup map is updated when lookup memory location changes. --- src/graph/graph.hh | 6 +++--- src/graph/gsubgpos-graph.hh | 9 +++++++-- src/hb-repacker.hh | 3 ++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index b0b6df71a..a73d86f15 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -291,12 +291,12 @@ struct graph_t check_success (!queue.in_error ()); check_success (!sorted_graph.in_error ()); - if (!check_success (new_id == -1)) - print_orphaned_nodes (); remap_all_obj_indices (id_map, &sorted_graph); - hb_swap (vertices_, sorted_graph); + + if (!check_success (new_id == -1)) + print_orphaned_nodes (); } /* diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index de7cc4992..937e05876 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -142,7 +142,8 @@ struct Lookup : public OT::Lookup + new_sub_tables.iter() | hb_sink (all_new_subtables); } - add_sub_tables (c, this_index, type, all_new_subtables); + if (all_new_subtables) + add_sub_tables (c, this_index, type, all_new_subtables); return true; } @@ -150,7 +151,7 @@ struct Lookup : public OT::Lookup void add_sub_tables (gsubgpos_graph_context_t& c, unsigned this_index, unsigned type, - hb_vector_t subtable_indices) + hb_vector_t& subtable_indices) { bool is_ext = is_extension (c.table_tag); auto& v = c.graph.vertices_[this_index]; @@ -184,6 +185,10 @@ struct Lookup : public OT::Lookup (char*) new_lookup; c.graph.vertices_[subtable_id].parents.push (this_index); } + + // The head location of the lookup has changed, invalidating the lookups map entry + // in the context. Update the map. + c.lookups.set (this_index, new_lookup); } unsigned create_extension_subtable (gsubgpos_graph_context_t& c, diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index e80360266..36bdf24f0 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -82,7 +82,8 @@ bool _presplit_subtables_if_needed (graph::gsubgpos_graph_context_t& ext_context for (unsigned lookup_index : ext_context.lookups.keys ()) { graph::Lookup* lookup = ext_context.lookups.get(lookup_index); - lookup->split_subtables_if_needed (ext_context, lookup_index); + if (!lookup->split_subtables_if_needed (ext_context, lookup_index)) + return false; } return true; From 1d2516f03706d00b362edbb45604c6bc74d4c60a Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 17:57:18 +0000 Subject: [PATCH 15/24] [repack] get basic pairpos split test working. --- src/test-repacker.cc | 68 +++++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 17 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index 8d7496072..affc416ee 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -146,15 +146,25 @@ static unsigned add_extension (unsigned child, static unsigned add_coverage (char start, char end, hb_serialize_context_t* c) { - char header[] = { + if (end - start == 1) + { + char coverage[] = { + 0, 1, // format + 0, 2, // count + 0, start, // glyph[0] + 0, end, // glyph[1] + }; + return add_object (coverage, 8, c); + } + + char coverage[] = { 0, 2, // format 0, 1, // range count 0, start, // start 0, end, // end 0, 0, }; - - return add_object (header, 10, c); + return add_object (coverage, 10, c); } static unsigned add_pair_pos_1 (unsigned* pair_sets, @@ -207,6 +217,12 @@ static void run_resolve_overflow_test (const char* name, assert (!expected.offset_overflow ()); hb_bytes_t expected_result = expected.copy_bytes (); + if (result.length != expected_result.length) + { + printf("result.length (%u) != expected.length (%u).\n", + result.length, + expected_result.length); + } assert (result.length == expected_result.length); bool equal = true; @@ -1040,26 +1056,48 @@ populate_serializer_with_extension_promotion (hb_serialize_context_t* c, c->end_serialize(); } +template static void -populate_serializer_with_large_pair_pos_1 (hb_serialize_context_t* c) +populate_serializer_with_large_pair_pos_1 (hb_serialize_context_t* c, + bool as_extension = false) { std::string large_string(60000, 'a'); c->start_serialize (); - unsigned pair_set[4]; - for (int i = 3; i >= 0; i--) - pair_set[i] = add_object (large_string.c_str (), 30000, c); + constexpr int total_pair_set = num_pair_pos_1 * num_pair_set; + unsigned pair_set[total_pair_set]; + unsigned coverage[num_pair_pos_1]; + unsigned pair_pos_1[num_pair_pos_1]; + + for (int i = num_pair_pos_1 - 1; i >= 0; i--) + { + for (int j = (i + 1) * num_pair_set - 1; j >= i * num_pair_set; j--) + pair_set[j] = add_object (large_string.c_str (), 30000 + j, c); + + coverage[i] = add_coverage (i * num_pair_set, + (i + 1) * num_pair_set - 1, c); - unsigned coverage = add_coverage (0, 3, c); + pair_pos_1[i] = add_pair_pos_1 (&pair_set[i * num_pair_set], + num_pair_set, + coverage[i], + c); + } unsigned pair_pos_2 = add_object (large_string.c_str(), 200, c); - unsigned pair_pos_1 = add_pair_pos_1 (pair_set, 4, coverage, c); + if (as_extension) { + + for (int i = num_pair_pos_1 - 1; i >= 0; i--) + pair_pos_1[i] = add_extension (pair_pos_1[i], 2, c); + pair_pos_2 = add_extension (pair_pos_2, 2, c); + } - start_lookup (2, 2, c); + start_lookup (as_extension ? 9 : 2, 1 + num_pair_pos_1, c); - add_offset (pair_pos_1, c); add_offset (pair_pos_2, c); + for (int i = 0; i < num_pair_pos_1; i++) + add_offset (pair_pos_1[i], c); + unsigned lookup = finish_lookup (c); @@ -1445,12 +1483,12 @@ static void test_resolve_with_basic_pair_pos_1_split () void* buffer = malloc (buffer_size); assert (buffer); hb_serialize_context_t c (buffer, buffer_size); - populate_serializer_with_large_pair_pos_1 (&c); + populate_serializer_with_large_pair_pos_1 <1, 4>(&c); void* expected_buffer = malloc (buffer_size); assert (expected_buffer); hb_serialize_context_t e (expected_buffer, buffer_size); - populate_serializer_with_large_pair_pos_1 (&e); + populate_serializer_with_large_pair_pos_1 <2, 2>(&e, true); run_resolve_overflow_test ("test_resolve_with_basic_pair_pos_1_split", c, @@ -1588,7 +1626,6 @@ test_shared_node_with_virtual_links () int main (int argc, char **argv) { - if (1) { test_serialize (); test_sort_shortest (); test_will_overflow_1 (); @@ -1611,12 +1648,9 @@ main (int argc, char **argv) test_virtual_link (); test_shared_node_with_virtual_links (); test_resolve_with_extension_promotion (); - } - test_resolve_with_basic_pair_pos_1_split (); // TODO: - // - basic splitting case. // - splitting with extensions. // TODO(grieger): test with extensions already mixed in as well. From 674f0194a3d33ba192bef875cbb91bec6d008fe3 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 17:59:50 +0000 Subject: [PATCH 16/24] [repacker] add extension pairpos split test. --- src/test-repacker.cc | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index affc416ee..053c0c603 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -1500,6 +1500,29 @@ static void test_resolve_with_basic_pair_pos_1_split () free (expected_buffer); } +static void test_resolve_with_extension_pair_pos_1_split () +{ + size_t buffer_size = 200000; + void* buffer = malloc (buffer_size); + assert (buffer); + hb_serialize_context_t c (buffer, buffer_size); + populate_serializer_with_large_pair_pos_1 <1, 4>(&c, true); + + void* expected_buffer = malloc (buffer_size); + assert (expected_buffer); + hb_serialize_context_t e (expected_buffer, buffer_size); + populate_serializer_with_large_pair_pos_1 <2, 2>(&e, true); + + run_resolve_overflow_test ("test_resolve_with_extension_pair_pos_1_split", + c, + e, + 20, + true, + HB_TAG('G', 'P', 'O', 'S')); + free (buffer); + free (expected_buffer); +} + static void test_resolve_overflows_via_splitting_spaces () { @@ -1649,9 +1672,7 @@ main (int argc, char **argv) test_shared_node_with_virtual_links (); test_resolve_with_extension_promotion (); test_resolve_with_basic_pair_pos_1_split (); - - // TODO: - // - splitting with extensions. + test_resolve_with_extension_pair_pos_1_split (); // TODO(grieger): test with extensions already mixed in as well. // TODO(grieger): test two layer ext promotion setup. From 2a5902ee50e81ce00d48a9a686ff7fc39690b475 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 18:12:49 +0000 Subject: [PATCH 17/24] [repacker] cleanup. --- src/hb-repacker.hh | 60 ++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index 36bdf24f0..61b142238 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -46,39 +46,39 @@ struct lookup_size_t unsigned lookup_index; size_t size; unsigned num_subtables; -}; - -inline int compare_sizes (const void* a, const void* b) -{ - lookup_size_t* size_a = (lookup_size_t*) a; - lookup_size_t* size_b = (lookup_size_t*) b; - double subtables_per_byte_a = (double) size_a->num_subtables / (double) size_a->size; - double subtables_per_byte_b = (double) size_b->num_subtables / (double) size_b->size; + static int cmp (const void* a, const void* b) + { + return cmp ((const lookup_size_t*) a, + (const lookup_size_t*) b); + } - if (subtables_per_byte_a == subtables_per_byte_b) { - return size_b->lookup_index - size_a->lookup_index; + static int cmp (const lookup_size_t* a, const lookup_size_t* b) + { + double subtables_per_byte_a = (double) a->num_subtables / (double) a->size; + double subtables_per_byte_b = (double) b->num_subtables / (double) b->size; + if (subtables_per_byte_a == subtables_per_byte_b) { + return b->lookup_index - a->lookup_index; + } + double cmp = subtables_per_byte_b - subtables_per_byte_a; + if (cmp < 0) return -1; + if (cmp > 0) return 1; + return 0; } - double cmp = subtables_per_byte_b - subtables_per_byte_a; - if (cmp < 0) return -1; - if (cmp > 0) return 1; - return 0; -} +}; static inline bool _presplit_subtables_if_needed (graph::gsubgpos_graph_context_t& ext_context) { - // Algorithm: - // 1. Scan all sub-tables and compute their sizes. - // 2. For any PairPos subtables that are >64kb: - // a. split into two more pieces that are <64kb - // b. De-dup subgraphs (optional) (can this be done during serialization?) - - // TODO: Add a split subtables method at the lookup level, it could scan it's subtables and split as - // needed. - // TODO: rename gsubgpos_graph_context_t to be more general. - + // For each lookup this will check the size of subtables and split them as needed + // so that no subtable is at risk of overflowing. (where we support splitting for + // that subtable type). + // + // TODO(grieger): de-dup newly added nodes as necessary. Probably just want a full de-dup + // pass after this processing is done. Not super necessary as splits are + // only done where overflow is likely, so de-dup probably will get undone + // later anyways. for (unsigned lookup_index : ext_context.lookups.keys ()) { graph::Lookup* lookup = ext_context.lookups.get(lookup_index); @@ -98,24 +98,20 @@ bool _promote_extensions_if_needed (graph::gsubgpos_graph_context_t& ext_context { // Simple Algorithm (v1, current): // 1. Calculate how many bytes each non-extension lookup consumes. - // 2. Select up to 64k of those to remain as non-extension (greedy, smallest first). + // 2. Select up to 64k of those to remain as non-extension (greedy, highest subtables per byte first) // 3. Promote the rest. // // Advanced Algorithm (v2, not implemented): // 1. Perform connected component analysis using lookups as roots. // 2. Compute size of each connected component. // 3. Select up to 64k worth of connected components to remain as non-extensions. - // (greedy, smallest first) + // (greedy, highest subtables per byte first) // 4. Promote the rest. // TODO(garretrieger): support extension demotion, then consider all lookups. Requires advanced algo. // TODO(garretrieger): also support extension promotion during iterative resolution phase, then // we can use a less conservative threshold here. // TODO(grieger): skip this for the 24 bit case. - // TODO(grieger): sort by # subtables / size instead (high to low). Goal is to get as many subtables - // as possible into space 0 to minimize the number of extension subtables added. - // A fully optimal solution will require a backpack problem dynamic programming type - // solution. if (!ext_context.lookups) return true; hb_vector_t lookup_sizes; @@ -132,7 +128,7 @@ bool _promote_extensions_if_needed (graph::gsubgpos_graph_context_t& ext_context }); } - lookup_sizes.qsort (compare_sizes); + lookup_sizes.qsort (); size_t lookup_list_size = ext_context.graph.vertices_[ext_context.lookup_list_index].table_size (); size_t l2_l3_size = lookup_list_size; // Lookup List + Lookups From 46c1fa7d1b0a58cce20dc5d51d314b4d8148c679 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 18:29:12 +0000 Subject: [PATCH 18/24] [repacker] sanitize PairPos during subtable extension. --- src/graph/gsubgpos-graph.hh | 12 ++++++++++-- src/graph/pairpos-graph.hh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index 937e05876..f7dcc4bfd 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -47,6 +47,12 @@ struct ExtensionFormat1 : public OT::ExtensionFormat1 this->extensionOffset = 0; } + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + return vertex_len >= OT::ExtensionFormat1::static_size; + } + unsigned get_lookup_type () const { return this->extensionLookupType; @@ -118,7 +124,6 @@ struct Lookup : public OT::Lookup if (!is_ext && type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair) return true; - // TODO check subtable type. hb_vector_t all_new_subtables; for (unsigned i = 0; i < subTable.len; i++) { @@ -128,6 +133,8 @@ struct Lookup : public OT::Lookup ExtensionFormat1* extension = (ExtensionFormat1*) c.graph.object (ext_subtable_index).head; + if (!extension->sanitize (c.graph.vertices_[ext_subtable_index])) + continue; subtable_index = extension->get_subtable_index (c.graph, ext_subtable_index); type = extension->get_lookup_type (); @@ -136,7 +143,8 @@ struct Lookup : public OT::Lookup } PairPos* pairPos = (PairPos*) c.graph.object (subtable_index).head; - // TODO sanitize + if (!pairPos->sanitize (c.graph.vertices_[subtable_index])) continue; + hb_vector_t new_sub_tables = pairPos->split_subtables (c, subtable_index); if (new_sub_tables.in_error ()) return false; + new_sub_tables.iter() | hb_sink (all_new_subtables); diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 777867c0a..2b0183d64 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -34,6 +34,16 @@ namespace graph { struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 { + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + unsigned min_size = OT::Layout::GPOS_impl::PairPosFormat1_3::min_size; + if (vertex_len < min_size) return false; + + return vertex_len >= + min_size + pairSet.get_size () - pairSet.len.get_size(); + } + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { hb_set_t visited; @@ -218,6 +228,12 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 { + bool sanitize (graph_t::vertex_t& vertex) const + { + // TODO + return true; + } + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { // TODO @@ -243,6 +259,24 @@ struct PairPos : public OT::Layout::GPOS_impl::PairPos return hb_vector_t (); } } + + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + if (vertex_len < u.format.get_size ()) return false; + + switch (u.format) { + case 1: + return ((PairPosFormat1*)(&u.format1))->sanitize (vertex); + case 2: + return ((PairPosFormat2*)(&u.format2))->sanitize (vertex); + case 3: + case 4: + default: + // We don't handle format 3 and 4 here. + return false; + } + } }; } From 38846f41d3760f9f70a7728b07c9829b776468a5 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 18:30:24 +0000 Subject: [PATCH 19/24] [repacker] more TODO cleanup. --- src/graph/gsubgpos-graph.hh | 3 --- src/graph/pairpos-graph.hh | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index f7dcc4bfd..afa1152c4 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -305,9 +305,6 @@ struct GSTAR : public OT::GSUBGPOS return len >= get_size (); } - // TODO(garretrieger): add find subtable's method, could be templated locate a specific - // subtable type, maybe take the lookup map as a starting point? - void find_lookups (graph_t& graph, hb_hashmap_t& lookups /* OUT */) { diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 2b0183d64..7c905899f 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -230,13 +230,13 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4 split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { - // TODO + // TODO(garretrieger): implement me! return hb_vector_t (); } }; From a0b8893e467d39aabac96836c0b6afbccb14c68d Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 19:58:51 +0000 Subject: [PATCH 20/24] [repacker] add Coverage sanitize. --- src/Makefile.sources | 2 + src/graph/coverage-graph.hh | 80 +++++++++++++++++++++++++++++++++++++ src/graph/pairpos-graph.hh | 21 +++++++--- src/harfbuzz-subset.cc | 14 +++---- src/meson.build | 4 ++ 5 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 src/graph/coverage-graph.hh diff --git a/src/Makefile.sources b/src/Makefile.sources index 16473634e..a3fb92bac 100644 --- a/src/Makefile.sources +++ b/src/Makefile.sources @@ -352,6 +352,8 @@ HB_SUBSET_sources = \ graph/gsubgpos-context.hh \ graph/gsubgpos-context.cc \ graph/pairpos-graph.hh \ + graph/coverage-graph.hh \ + graph/pairpos-graph.hh \ graph/serialize.hh \ $(NULL) diff --git a/src/graph/coverage-graph.hh b/src/graph/coverage-graph.hh new file mode 100644 index 000000000..1d9fd0eb5 --- /dev/null +++ b/src/graph/coverage-graph.hh @@ -0,0 +1,80 @@ +/* + * Copyright © 2022 Google, Inc. + * + * This is part of HarfBuzz, a text shaping library. + * + * Permission is hereby granted, without written agreement and without + * license or royalty fees, to use, copy, modify, and distribute this + * software and its documentation for any purpose, provided that the + * above copyright notice and the following two paragraphs appear in + * all copies of this software. + * + * IN NO EVENT SHALL THE COPYRIGHT HOLDER BE LIABLE TO ANY PARTY FOR + * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES + * ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS DOCUMENTATION, EVEN + * IF THE COPYRIGHT HOLDER HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH + * DAMAGE. + * + * THE COPYRIGHT HOLDER SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING, + * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS + * ON AN "AS IS" BASIS, AND THE COPYRIGHT HOLDER HAS NO OBLIGATION TO + * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS. + * + * Google Author(s): Garret Rieger + */ + +#include "graph.hh" +#include "../OT/Layout/Common/Coverage.hh" + +#ifndef GRAPH_COVERAGE_GRAPH_HH +#define GRAPH_COVERAGE_GRAPH_HH + +namespace graph { + +struct CoverageFormat1 : public OT::Layout::Common::CoverageFormat1_3 +{ + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + constexpr unsigned min_size = OT::Layout::Common::CoverageFormat1_3::min_size; + if (vertex_len < min_size) return false; + return vertex_len >= min_size + glyphArray.get_size () - glyphArray.len.get_size (); + } +}; + +struct CoverageFormat2 : public OT::Layout::Common::CoverageFormat2_4 +{ + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + constexpr unsigned min_size = OT::Layout::Common::CoverageFormat2_4::min_size; + if (vertex_len < min_size) return false; + return vertex_len >= min_size + rangeRecord.get_size () - rangeRecord.len.get_size (); + } +}; + +struct Coverage : public OT::Layout::Common::Coverage +{ + bool sanitize (graph_t::vertex_t& vertex) const + { + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + if (vertex_len < OT::Layout::Common::Coverage::min_size) return false; + switch (u.format) + { + case 1: return ((CoverageFormat1*)this)->sanitize (vertex); + case 2: return ((CoverageFormat2*)this)->sanitize (vertex); +#ifndef HB_NO_BORING_EXPANSION + // Not currently supported + case 3: + case 4: +#endif + default: return false; + } + } +}; + + +} + +#endif // GRAPH_COVERAGE_GRAPH_HH diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 7c905899f..429d84323 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -27,6 +27,7 @@ #ifndef GRAPH_PAIRPOS_GRAPH_HH #define GRAPH_PAIRPOS_GRAPH_HH +#include "coverage-graph.hh" #include "../OT/Layout/GPOS/PairPos.hh" #include "../OT/Layout/GPOS/PosLookupSubTable.hh" @@ -47,9 +48,13 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) { hb_set_t visited; - const unsigned base_size = OT::Layout::GPOS_impl::PairPosFormat1_3::min_size; + + const unsigned coverage_id = c.graph.index_for_offset (this_index, &coverage); + const unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); + const unsigned base_size = OT::Layout::GPOS_impl::PairPosFormat1_3::min_size + + coverage_size; + unsigned accumulated = base_size; - // TODO: include coverage size hb_vector_t split_points; for (unsigned i = 0; i < pairSet.len; i++) { @@ -121,8 +126,10 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3sanitize (coverage_v)) + return false; auto new_coverage = + hb_zip (coverage_table->iter (), hb_range ()) @@ -167,8 +174,10 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3sanitize (coverage_v)) + return false; auto new_coverage = + hb_zip (coverage_table->iter (), hb_range ()) diff --git a/src/harfbuzz-subset.cc b/src/harfbuzz-subset.cc index b6d722515..ae4907eb4 100644 --- a/src/harfbuzz-subset.cc +++ b/src/harfbuzz-subset.cc @@ -1,10 +1,10 @@ -#include "gsubgpos-context.cc" +#include "graph/gsubgpos-context.cc" #include "hb-aat-layout.cc" #include "hb-aat-map.cc" #include "hb-blob.cc" +#include "hb-buffer.cc" #include "hb-buffer-serialize.cc" #include "hb-buffer-verify.cc" -#include "hb-buffer.cc" #include "hb-common.cc" #include "hb-draw.cc" #include "hb-face.cc" @@ -23,15 +23,15 @@ #include "hb-ot-meta.cc" #include "hb-ot-metrics.cc" #include "hb-ot-name.cc" +#include "hb-ot-shape.cc" #include "hb-ot-shape-fallback.cc" #include "hb-ot-shape-normalize.cc" -#include "hb-ot-shape.cc" #include "hb-ot-shaper-arabic.cc" #include "hb-ot-shaper-default.cc" #include "hb-ot-shaper-hangul.cc" #include "hb-ot-shaper-hebrew.cc" -#include "hb-ot-shaper-indic-table.cc" #include "hb-ot-shaper-indic.cc" +#include "hb-ot-shaper-indic-table.cc" #include "hb-ot-shaper-khmer.cc" #include "hb-ot-shaper-myanmar.cc" #include "hb-ot-shaper-syllabic.cc" @@ -41,17 +41,17 @@ #include "hb-ot-tag.cc" #include "hb-ot-var.cc" #include "hb-set.cc" -#include "hb-shape-plan.cc" #include "hb-shape.cc" +#include "hb-shape-plan.cc" #include "hb-shaper.cc" #include "hb-static.cc" #include "hb-style.cc" -#include "hb-subset-cff-common.cc" +#include "hb-subset.cc" #include "hb-subset-cff1.cc" #include "hb-subset-cff2.cc" +#include "hb-subset-cff-common.cc" #include "hb-subset-input.cc" #include "hb-subset-plan.cc" #include "hb-subset-repacker.cc" -#include "hb-subset.cc" #include "hb-ucd.cc" #include "hb-unicode.cc" diff --git a/src/meson.build b/src/meson.build index 631b124b4..1f1105cae 100644 --- a/src/meson.build +++ b/src/meson.build @@ -346,6 +346,10 @@ hb_subset_sources = files( 'hb-subset-plan.hh', 'hb-subset-repacker.cc', 'graph/gsubgpos-context.cc', + 'graph/gsubgpos-context.hh', + 'graph/gsubgpos-graph.hh', + 'graph/pairpos-graph.hh', + 'graph/coverage-graph.hh', 'hb-subset.cc', 'hb-subset.hh', ) From 3b91fb2a9fa71da0000246bbd124b019c350ee43 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 20:04:42 +0000 Subject: [PATCH 21/24] [repacker] cleanup todo. --- src/graph/graph.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index a73d86f15..b3aef558a 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -569,7 +569,7 @@ struct graph_t auto& child = vertices_[child_id]; child.parents.push (new_parent_idx); - old_v.remove_real_link (child_id, old_offset); // TODO: needs to include offset to ensure correct one removed + old_v.remove_real_link (child_id, old_offset); child.remove_parent (old_parent_idx); } From 14f95ee0cf0289fe1daffa9e10a9d0dfc8e0e6be Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 20:09:52 +0000 Subject: [PATCH 22/24] [repacker] re-count shared node sizes in split PairPos segments. --- src/graph/pairpos-graph.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 429d84323..b5c8c41ea 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -66,6 +66,7 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 Date: Fri, 29 Jul 2022 20:38:53 +0000 Subject: [PATCH 23/24] [repacker] add todo. --- src/graph/pairpos-graph.hh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index b5c8c41ea..678c4ba1f 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -62,6 +62,10 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3 (1 << 16)) { split_points.push (i); From 9578c44ea226bfd0e230bc60de16f328c40ba557 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Fri, 29 Jul 2022 21:58:24 +0000 Subject: [PATCH 24/24] [repacker] add HB_FALLTRHOUGH. --- src/graph/pairpos-graph.hh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 678c4ba1f..9b146fcc1 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -265,8 +265,8 @@ struct PairPos : public OT::Layout::GPOS_impl::PairPos case 2: return ((PairPosFormat2*)(&u.format2))->split_subtables (c, this_index); #ifndef HB_NO_BORING_EXPANSION - case 3: - case 4: + case 3: HB_FALLTHROUGH; + case 4: HB_FALLTHROUGH; // Don't split 24bit PairPos's. #endif default: @@ -284,8 +284,10 @@ struct PairPos : public OT::Layout::GPOS_impl::PairPos return ((PairPosFormat1*)(&u.format1))->sanitize (vertex); case 2: return ((PairPosFormat2*)(&u.format2))->sanitize (vertex); - case 3: - case 4: +#ifndef HB_NO_BORING_EXPANSION + case 3: HB_FALLTHROUGH; + case 4: HB_FALLTHROUGH; +#endif default: // We don't handle format 3 and 4 here. return false;