From 4418beac932f98d77a774e94d51745637b03b513 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 19:08:04 +0000 Subject: [PATCH 01/29] [repacker] start implmenting MarkBasePos splitting. --- src/Makefile.sources | 2 +- src/graph/markbasepos-graph.hh | 310 +++++++++++++++++++++++++++++++++ src/meson.build | 1 + 3 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 src/graph/markbasepos-graph.hh diff --git a/src/Makefile.sources b/src/Makefile.sources index 0a6b845ee..37c83dc9f 100644 --- a/src/Makefile.sources +++ b/src/Makefile.sources @@ -351,10 +351,10 @@ HB_SUBSET_sources = \ graph/gsubgpos-graph.hh \ graph/gsubgpos-context.hh \ graph/gsubgpos-context.cc \ - graph/pairpos-graph.hh \ graph/coverage-graph.hh \ graph/classdef-graph.hh \ graph/pairpos-graph.hh \ + graph/markbasepos-graph.hh \ graph/split-helpers.hh \ graph/serialize.hh \ $(NULL) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh new file mode 100644 index 000000000..9007188b1 --- /dev/null +++ b/src/graph/markbasepos-graph.hh @@ -0,0 +1,310 @@ +/* + * 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_MARKBASEPOS_GRAPH_HH +#define GRAPH_MARKBASEPOS_GRAPH_HH + +#include "split-helpers.hh" +#include "coverage-graph.hh" +#include "../OT/Layout/GPOS/MarkBasePos.hh" +#include "../OT/Layout/GPOS/PosLookupSubTable.hh" + +namespace graph { + +struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix +{ + bool sanitize (graph_t::vertex_t& vertex) const + { + // TODO + return false; + } +}; + +struct MarkArray : public OT::Layout::GPOS_impl::MarkArray +{ + bool sanitize (graph_t::vertex_t& vertex) const + { + // TODO + return false; + } +}; + +struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 +{ + 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::MarkBasePosFormat1_2::min_size; + if (vertex_len < min_size) return false; + + // TODO + return true; + } + + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) + { + hb_set_t visited; + + const unsigned coverage_id = c.graph.index_for_offset (this_index, &markCoverage); + const unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); + + const unsigned base_coverage_id = c.graph.index_for_offset (this_index, &baseCoverage); + const unsigned base_size = + OT::Layout::GPOS_impl::PairPosFormat1_3::min_size + + c.graph.vertices_[base_coverage_id].table_size (); + + hb_vector_t class_to_info = get_class_info (c, this_index); + + unsigned partial_coverage_size = 4; + unsigned accumulated = base_size; + hb_vector_t split_points; + unsigned class_count = classCount; + for (unsigned klass = 0; klass < class_count; klass++) + { + class_info_t& info = class_to_info[klass]; + partial_coverage_size += OT::HBUINT16::static_size * info.num_marks; + unsigned accumulated_delta = OT::Layout::GPOS_impl::MarkRecord::static_size * info.num_marks; + accumulated_delta += OT::Offset16::static_size * info.child_indices.length; + for (unsigned objidx : info.child_indices) + accumulated_delta += c.graph.find_subgraph_size (objidx, visited); + + accumulated += accumulated_delta; + unsigned total = accumulated + partial_coverage_size; + + if (total >= (1 << 16)) + { + split_points.push (klass); + accumulated = base_size + accumulated_delta; + partial_coverage_size = 4 + OT::HBUINT16::static_size * info.num_marks; + visited.clear (); // node sharing isn't allowed between splits. + } + } + + split_context_t split_context { + c, + this, + this_index, + }; + + return actuate_subtable_split (split_context, split_points); + } + + private: + + struct split_context_t { + gsubgpos_graph_context_t& c; + MarkBasePosFormat1* thiz; + unsigned this_index; + + unsigned original_count () + { + return thiz->pairSet.len; + } + + unsigned clone_range (unsigned start, unsigned end) + { + return thiz->clone_range (this->c, this->this_index, start, end); + } + + bool shrink (unsigned count) + { + return thiz->shrink (this->c, this->this_index, count); + } + }; + + struct class_info_t { + unsigned num_marks; + hb_vector_t child_indices; + }; + + hb_vector_t get_class_info (gsubgpos_graph_context_t& c, + unsigned this_index) + { + hb_vector_t class_to_info; + + unsigned class_count= classCount; + class_to_size.resize (class_count); + + unsigned mark_array_id = + c.graph.index_for_offset (this_index, &markArray); + auto& mark_array_v = graph.vertices_[coverage_id]; + MarkArray* mark_array = (MarkArray*) mark_array_v.head; + // TODO sanitize + + unsigned mark_count = mark_array->length; + for (unsigned mark = 0; mark < mark_count; mark++) + { + unsigned klass = (*mark_array)[mark].klass; + class_to_size[klass].num_marks++; + } + + for (const auto* link : mark_array_v.obj.real_links) + { + unsiged mark = (link->position - 2) / + OT::Layout::GPOS_impl::MarkReecord::static_size; + unsigned klass = (*mark_array)[mark].klass; + class_to_info[klass].child_indices.push (link.objidx); + } + + unsigned base_array_id = + c.graph.index_for_offset (this_index, &baseArray); + auto& base_array_v = graph.vertices_[coverage_id]; + AnchorMatrix* base_array = (AnchorMatrix*) base_array_v.head; + // TODO sanitize + + unsigned base_count = base_array->rows; + for (const auto* link : base_array_v.obj.real_links) + { + unsigned index = (link->position - 2) / OT::Offset16::static_size; + unsigned klass = index % class_count; + class_to_info[klass].child_indices.push (link->objidx); + } + + return class_to_info; + } + + bool shrink (gsubgpos_graph_context_t& c, + unsigned this_index, + unsigned count) + { + /* + DEBUG_MSG (SUBSET_REPACK, nullptr, + " Shrinking PairPosFormat1 (%u) to [0, %u).", + this_index, + count); + unsigned old_count = pairSet.len; + if (count >= old_count) + return true; + + pairSet.len = count; + c.graph.vertices_[this_index].obj.tail -= (old_count - count) * SmallTypes::size; + + unsigned coverage_id = c.graph.mutable_index_for_offset (this_index, &coverage); + unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); + auto& coverage_v = c.graph.vertices_[coverage_id]; + + Coverage* coverage_table = (Coverage*) coverage_v.obj.head; + if (!coverage_table || !coverage_table->sanitize (coverage_v)) + return false; + + 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 Coverage::make_coverage (c, new_coverage, coverage_id, coverage_size); + */ + return false; // 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 + { + /* + DEBUG_MSG (SUBSET_REPACK, nullptr, + " Cloning PairPosFormat1 (%u) range [%u, %u).", this_index, start, end); + + unsigned num_pair_sets = end - start; + unsigned prime_size = 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]); + } + + unsigned coverage_id = c.graph.index_for_offset (this_index, &coverage); + if (!Coverage::clone_coverage (c, + coverage_id, + pair_pos_prime_id, + 2, + start, end)) + return -1; + + return pair_pos_prime_id; + */ + return false; // TODO + } +}; + + +struct MarkBasePos : public OT::Layout::GPOS_impl::MarkBasePos +{ + hb_vector_t split_subtables (gsubgpos_graph_context_t& c, + unsigned this_index) + { + switch (u.format) { + case 1: + return ((MarkBasePosFormat1*)(&u.format1))->split_subtables (c, this_index); +#ifndef HB_NO_BORING_EXPANSION + case 2: HB_FALLTHROUGH; + // Don't split 24bit PairPos's. +#endif + default: + 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); +#ifndef HB_NO_BORING_EXPANSION + case 2: HB_FALLTHROUGH; +#endif + default: + // We don't handle format 3 and 4 here. + return false; + } + } +}; + + +} + +#endif // GRAPH_MARKBASEPOS_GRAPH_HH diff --git a/src/meson.build b/src/meson.build index 3cbebd3ea..4cf3451e9 100644 --- a/src/meson.build +++ b/src/meson.build @@ -349,6 +349,7 @@ hb_subset_sources = files( 'graph/gsubgpos-context.hh', 'graph/gsubgpos-graph.hh', 'graph/pairpos-graph.hh', + 'graph/markbasepos-graph.hh', 'graph/coverage-graph.hh', 'graph/classdef-graph.hh', 'graph/split-helpers.hh', From cf817f3d99d46dd39e9004ef94ec71077d8af8d9 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 19:26:59 +0000 Subject: [PATCH 02/29] [repacker] Hook up MarkBasePos splitting. --- src/graph/gsubgpos-graph.hh | 27 +++++++++++++++++++++---- src/graph/markbasepos-graph.hh | 37 +++++++++++++++------------------- 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index f963a49ad..31fc1d8e4 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -29,6 +29,7 @@ #include "../OT/Layout/GSUB/ExtensionSubst.hh" #include "gsubgpos-context.hh" #include "pairpos-graph.hh" +#include "markbasepos-graph.hh" #ifndef GRAPH_GSUBGPOS_GRAPH_HH #define GRAPH_GSUBGPOS_GRAPH_HH @@ -142,11 +143,18 @@ struct Lookup : public OT::Lookup continue; } - PairPos* pairPos = (PairPos*) c.graph.object (subtable_index).head; - if (!pairPos || !pairPos->sanitize (c.graph.vertices_[subtable_index])) continue; - - hb_vector_t new_sub_tables = pairPos->split_subtables (c, subtable_index); + hb_vector_t new_sub_tables; + switch (type) + { + case 2: + new_sub_tables = split_subtable (c, subtable_index); break; + case 4: + new_sub_tables = split_subtable (c, subtable_index); break; + default: + break; + } if (new_sub_tables.in_error ()) return false; + if (!new_sub_tables) continue; hb_pair_t>* entry = all_new_subtables.push (); entry->first = i; entry->second = std::move (new_sub_tables); @@ -159,6 +167,17 @@ struct Lookup : public OT::Lookup return true; } + template + hb_vector_t split_subtable (gsubgpos_graph_context_t& c, + unsigned objidx) + { + T* sub_table = (T*) c.graph.object (objidx).head; + if (!sub_table || !sub_table->sanitize (c.graph.vertices_[objidx])) + return hb_vector_t (); + + return sub_table->split_subtables (c, objidx); + } + void add_sub_tables (gsubgpos_graph_context_t& c, unsigned this_index, unsigned type, diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 9007188b1..d6d3c7aaf 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -68,9 +68,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2::min_size + @@ -87,6 +84,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2pairSet.len; + return thiz->classCount; } unsigned clone_range (unsigned start, unsigned end) @@ -146,41 +144,38 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 class_to_info; unsigned class_count= classCount; - class_to_size.resize (class_count); + class_to_info.resize (class_count); unsigned mark_array_id = c.graph.index_for_offset (this_index, &markArray); - auto& mark_array_v = graph.vertices_[coverage_id]; - MarkArray* mark_array = (MarkArray*) mark_array_v.head; + auto& mark_array_v = c.graph.vertices_[mark_array_id]; + MarkArray* mark_array = (MarkArray*) mark_array_v.obj.head; // TODO sanitize - unsigned mark_count = mark_array->length; + unsigned mark_count = mark_array->len; for (unsigned mark = 0; mark < mark_count; mark++) { - unsigned klass = (*mark_array)[mark].klass; - class_to_size[klass].num_marks++; + unsigned klass = (*mark_array)[mark].get_class (); + class_to_info[klass].num_marks++; } - for (const auto* link : mark_array_v.obj.real_links) + for (const auto& link : mark_array_v.obj.real_links) { - unsiged mark = (link->position - 2) / - OT::Layout::GPOS_impl::MarkReecord::static_size; - unsigned klass = (*mark_array)[mark].klass; + unsigned mark = (link.position - 2) / + OT::Layout::GPOS_impl::MarkRecord::static_size; + unsigned klass = (*mark_array)[mark].get_class (); class_to_info[klass].child_indices.push (link.objidx); } unsigned base_array_id = c.graph.index_for_offset (this_index, &baseArray); - auto& base_array_v = graph.vertices_[coverage_id]; - AnchorMatrix* base_array = (AnchorMatrix*) base_array_v.head; - // TODO sanitize + auto& base_array_v = c.graph.vertices_[base_array_id]; - unsigned base_count = base_array->rows; - for (const auto* link : base_array_v.obj.real_links) + for (const auto& link : base_array_v.obj.real_links) { - unsigned index = (link->position - 2) / OT::Offset16::static_size; + unsigned index = (link.position - 2) / OT::Offset16::static_size; unsigned klass = index % class_count; - class_to_info[klass].child_indices.push (link->objidx); + class_to_info[klass].child_indices.push (link.objidx); } return class_to_info; From 1acd2a8bf901dd9a22b5a23ab7ac7b4021969bd5 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 20:22:31 +0000 Subject: [PATCH 03/29] [repacker] implement MarkBasePosFormat1::clone_range. --- src/graph/coverage-graph.hh | 2 +- src/graph/graph.hh | 12 +++ src/graph/markbasepos-graph.hh | 145 ++++++++++++++++++++++++--------- src/graph/pairpos-graph.hh | 10 +-- 4 files changed, 120 insertions(+), 49 deletions(-) diff --git a/src/graph/coverage-graph.hh b/src/graph/coverage-graph.hh index da71ea6fb..3c1022f09 100644 --- a/src/graph/coverage-graph.hh +++ b/src/graph/coverage-graph.hh @@ -109,7 +109,7 @@ struct Coverage : public OT::Layout::Common::Coverage { char* buffer = (char*) hb_calloc (1, max_size); hb_serialize_context_t serializer (buffer, max_size); - Coverage_serialize (&serializer, glyphs); + OT::Layout::Common::Coverage_serialize (&serializer, glyphs); serializer.end_serialize (); if (serializer.in_error ()) { diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 0d6adcb64..85623c297 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -60,6 +60,18 @@ struct graph_t hb_swap (a.priority, b.priority); } + hb_hashmap_t + position_to_index_map () const + { + hb_hashmap_t result; + + for (const auto& l : obj.real_links) { + result.set (l.position, l.objidx); + } + + return result; + } + bool is_shared () const { return parents.length > 1; diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index d6d3c7aaf..3e74ad184 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -41,6 +41,16 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix // TODO return false; } + + unsigned clone (gsubgpos_graph_context_t& c, + unsigned this_index, + const hb_hashmap_t& pos_to_index, + unsigned start, + unsigned end) + { + // TODO + return -1; + } }; struct MarkArray : public OT::Layout::GPOS_impl::MarkArray @@ -50,6 +60,15 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray // TODO return false; } + + unsigned clone (gsubgpos_graph_context_t& c, + unsigned this_index, + const hb_hashmap_t& pos_to_index, + hb_set_t& marks) + { + // TODO + return -1; + } }; struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 @@ -82,8 +101,8 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 (split_context, split_points); @@ -112,10 +139,18 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 child_indices; + }; + struct split_context_t { gsubgpos_graph_context_t& c; MarkBasePosFormat1* thiz; unsigned this_index; + hb_vector_t class_to_info; + hb_hashmap_t mark_array_links; + hb_hashmap_t base_array_links; unsigned original_count () { @@ -124,20 +159,15 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone_range (this->c, this->this_index, start, end); + return thiz->clone_range (*this, this->this_index, start, end); } bool shrink (unsigned count) { - return thiz->shrink (this->c, this->this_index, count); + return thiz->shrink (*this, this->this_index, count); } }; - struct class_info_t { - unsigned num_marks; - hb_vector_t child_indices; - }; - hb_vector_t get_class_info (gsubgpos_graph_context_t& c, unsigned this_index) { @@ -156,7 +186,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2::min_size - + num_pair_sets * SmallTypes::size; + // TODO - unsigned pair_pos_prime_id = c.create_node (prime_size); - if (pair_pos_prime_id == (unsigned) -1) return -1; + graph_t& graph = sc.c.graph; + unsigned prime_size = OT::Layout::GPOS_impl::MarkBasePosFormat1_2::static_size; - 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; + unsigned prime_id = sc.c.create_node (prime_size); + if (prime_id == (unsigned) -1) return -1; - for (unsigned i = start; i < end; i++) + MarkBasePosFormat1* prime = (MarkBasePosFormat1*) graph.object (prime_id).head; + prime->format = this->format; + prime->classCount = this->classCount; + + unsigned base_coverage_id = + graph.index_for_offset (sc.this_index, &baseCoverage); + auto* base_coverage_link = graph.vertices_[prime_id].obj.real_links.push (); + base_coverage_link->width = SmallTypes::size; + base_coverage_link->objidx = base_coverage_id; + base_coverage_link->position = 4; + graph.vertices_[base_coverage_id].parents.push (prime_id); + graph.duplicate (prime_id, base_coverage_id); + + hb_set_t marks; + for (unsigned klass = start; klass < end; klass++) { - c.graph.move_child<> (this_index, - &pairSet[i], - pair_pos_prime_id, - &pair_pos_prime->pairSet[i - start]); + + sc.class_to_info[klass].marks.iter () + | hb_sink (marks) + ; } - unsigned coverage_id = c.graph.index_for_offset (this_index, &coverage); - if (!Coverage::clone_coverage (c, - coverage_id, - pair_pos_prime_id, - 2, - start, end)) + if (!Coverage::add_coverage (sc.c, + prime_id, + 2, + + marks.iter (), + marks.get_population () * 2 + 4)) return -1; - return pair_pos_prime_id; - */ - return false; // TODO + unsigned mark_array_id = + graph.index_for_offset (sc.this_index, &markArray); + auto& mark_array_v = graph.vertices_[mark_array_id]; + MarkArray* mark_array = (MarkArray*) mark_array_v.obj.head; + // TODO sanitize + unsigned new_mark_array = mark_array->clone (sc.c, + mark_array_id, + sc.mark_array_links, + marks); + + auto* mark_array_link = graph.vertices_[prime_id].obj.real_links.push (); + mark_array_link->width = SmallTypes::size; + mark_array_link->objidx = new_mark_array; + mark_array_link->position = 8; + graph.vertices_[new_mark_array].parents.push (prime_id); + + unsigned base_array_id = + graph.index_for_offset (sc.this_index, &baseArray); + auto& base_array_v = graph.vertices_[base_array_id]; + AnchorMatrix* base_array = (AnchorMatrix*) base_array_v.obj.head; + // TODO sanitize + unsigned new_base_array = base_array->clone (sc.c, + mark_array_id, + sc.base_array_links, + start, end); + auto* base_array_link = graph.vertices_[prime_id].obj.real_links.push (); + base_array_link->width = SmallTypes::size; + base_array_link->objidx = new_base_array; + base_array_link->position = 10; + graph.vertices_[new_base_array].parents.push (prime_id); + + return prime_id; } }; diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 769f29064..976b87232 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -548,14 +548,8 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4 result; - - const auto& o = c.graph.object (this_index); - for (const auto& l : o.real_links) { - result.set (l.position, l.objidx); - } - - return result; + const auto& v = c.graph.vertices_[this_index]; + return v.position_to_index_map (); } const Coverage* get_coverage (gsubgpos_graph_context_t& c, From b00eb77682526b3e8c5f0eba0ddff0c28ff80abd Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 20:33:21 +0000 Subject: [PATCH 04/29] [repack] Add add_link helper to graph. --- src/graph/graph.hh | 16 ++++++++++++++++ src/graph/markbasepos-graph.hh | 19 +++---------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 85623c297..cd95fe03b 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -240,6 +240,22 @@ struct graph_t return vertices_[i].obj; } + /* + * Adds a 16 bit link from parent_id to child_id + */ + template + void add_link (T* offset, + unsigned parent_id, + unsigned child_id) + { + auto& v = vertices_[parent_id]; + auto* link = v.obj.real_links.push (); + link->width = 2; + link->objidx = child_id; + link->position = (char*) offset - (char*) v.obj.head; + vertices_[child_id].parents.push (parent_id); + } + /* * Generates a new topological sorting of graph ordered by the shortest * distance to each node if positions are marked as invalid. diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 3e74ad184..e0223c089 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -271,11 +271,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2width = SmallTypes::size; - base_coverage_link->objidx = base_coverage_id; - base_coverage_link->position = 4; - graph.vertices_[base_coverage_id].parents.push (prime_id); + graph.add_link (&(prime->baseCoverage), prime_id, base_coverage_id); graph.duplicate (prime_id, base_coverage_id); hb_set_t marks; @@ -302,12 +298,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2width = SmallTypes::size; - mark_array_link->objidx = new_mark_array; - mark_array_link->position = 8; - graph.vertices_[new_mark_array].parents.push (prime_id); + graph.add_link (&(prime->markArray), prime_id, new_mark_array); unsigned base_array_id = graph.index_for_offset (sc.this_index, &baseArray); @@ -318,11 +309,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2width = SmallTypes::size; - base_array_link->objidx = new_base_array; - base_array_link->position = 10; - graph.vertices_[new_base_array].parents.push (prime_id); + graph.add_link (&(prime->baseArray), prime_id, new_base_array); return prime_id; } From 0083fd109c9dda14df9d42c9733745e359c62c7f Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 22:09:46 +0000 Subject: [PATCH 05/29] [repacker] add as_table() helper to graph. --- src/graph/graph.hh | 40 +++++++++++++++++++++++++++ src/graph/markbasepos-graph.hh | 50 +++++++++++++++------------------- 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index cd95fe03b..5f2341420 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -169,6 +169,21 @@ struct graph_t } }; + template + struct vertex_and_table_t + { + vertex_and_table_t () : index (0), vertex (nullptr), table (nullptr) + {} + + unsigned index; + vertex_t* vertex; + T* table; + + operator bool () { + return table && vertex; + } + }; + /* * A topological sorting of an object graph. Ordered * in reverse serialization order (first object in the @@ -373,6 +388,31 @@ struct graph_t } } + template + vertex_and_table_t as_table (unsigned parent, const void* offset) + { + return as_table (index_for_offset (parent, offset)); + } + + template + vertex_and_table_t as_table (unsigned index) + { + if (index >= vertices_.length) + return vertex_and_table_t (); + + vertex_and_table_t r; + r.vertex = &vertices_[index]; + r.table = (T*) r.vertex->obj.head; + r.index = index; + if (!r.table) + return vertex_and_table_t (); + + if (!r.table->sanitize (*(r.vertex))) + return vertex_and_table_t (); + + return r; + } + // Finds the object id of the object pointed to by the offset at 'offset' // within object[node_idx]. unsigned index_for_offset (unsigned node_idx, const void* offset) const diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index e0223c089..7442d0490 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -176,24 +176,20 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2len; + auto mark_array = c.graph.as_table (this_index, &markArray); + if (!mark_array) return hb_vector_t (); + unsigned mark_count = mark_array.table->len; for (unsigned mark = 0; mark < mark_count; mark++) { - unsigned klass = (*mark_array)[mark].get_class (); + unsigned klass = (*mark_array.table)[mark].get_class (); class_to_info[klass].marks.add (mark); } - for (const auto& link : mark_array_v.obj.real_links) + for (const auto& link : mark_array.vertex->obj.real_links) { unsigned mark = (link.position - 2) / OT::Layout::GPOS_impl::MarkRecord::static_size; - unsigned klass = (*mark_array)[mark].get_class (); + unsigned klass = (*mark_array.table)[mark].get_class (); class_to_info[klass].child_indices.push (link.objidx); } @@ -289,26 +285,24 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone (sc.c, - mark_array_id, - sc.mark_array_links, - marks); + auto mark_array = + graph.as_table (sc.this_index, &markArray); + if (!mark_array) return -1; + unsigned new_mark_array = + mark_array.table->clone (sc.c, + mark_array.index, + sc.mark_array_links, + marks); graph.add_link (&(prime->markArray), prime_id, new_mark_array); - unsigned base_array_id = - graph.index_for_offset (sc.this_index, &baseArray); - auto& base_array_v = graph.vertices_[base_array_id]; - AnchorMatrix* base_array = (AnchorMatrix*) base_array_v.obj.head; - // TODO sanitize - unsigned new_base_array = base_array->clone (sc.c, - mark_array_id, - sc.base_array_links, - start, end); + auto base_array = + graph.as_table (sc.this_index, &baseArray); + if (!base_array) return -1; + unsigned new_base_array = + base_array.table->clone (sc.c, + mark_array.index, + sc.base_array_links, + start, end); graph.add_link (&(prime->baseArray), prime_id, new_base_array); return prime_id; From 5ea3c0be8f2cdbf998d1d4ec5b879bab5241ef66 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 22:21:28 +0000 Subject: [PATCH 06/29] [repacker] Implement MarkArray::clone. --- src/OT/Layout/GPOS/MarkRecord.hh | 2 +- src/graph/markbasepos-graph.hh | 27 +++++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/OT/Layout/GPOS/MarkRecord.hh b/src/OT/Layout/GPOS/MarkRecord.hh index 7a514453a..a7d489d2a 100644 --- a/src/OT/Layout/GPOS/MarkRecord.hh +++ b/src/OT/Layout/GPOS/MarkRecord.hh @@ -9,7 +9,7 @@ struct MarkRecord { friend struct MarkArray; - protected: + public: HBUINT16 klass; /* Class defined for this mark */ Offset16To markAnchor; /* Offset to Anchor table--from diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 7442d0490..a831ce228 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -66,8 +66,31 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray const hb_hashmap_t& pos_to_index, hb_set_t& marks) { - // TODO - return -1; + unsigned size = MarkArray::min_size + + OT::Layout::GPOS_impl::MarkRecord::static_size * + marks.get_population (); + unsigned prime_id = c.create_node (size); + if (prime_id == (unsigned) -1) return -1; + MarkArray* prime = (MarkArray*) c.graph.object (prime_id).head; + prime->len = marks.get_population (); + + + unsigned i = 0; + for (hb_codepoint_t mark : marks) + { + (*prime)[i].klass = (*this)[mark].klass; + unsigned offset_pos = (char*) &((*this)[mark].markAnchor) - (char*) this; + unsigned* anchor_index; + if (pos_to_index.has (offset_pos, &anchor_index)) + c.graph.move_child (this_index, + &((*this)[mark].markAnchor), + prime_id, + &((*prime)[i].markAnchor)); + + i++; + } + + return prime_id; } }; From c9ddf0815a62c3812ff9386f89d29ac80dfb96ae Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 22:34:59 +0000 Subject: [PATCH 07/29] [repacker] Implement AnchorMatrix::clone. --- src/graph/markbasepos-graph.hh | 37 ++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index a831ce228..652dc2d92 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -46,10 +46,39 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix unsigned this_index, const hb_hashmap_t& pos_to_index, unsigned start, - unsigned end) + unsigned end, + unsigned class_count) { - // TODO - return -1; + unsigned base_count = rows; + unsigned new_class_count = end - start; + unsigned size = AnchorMatrix::min_size + + OT::Offset16::static_size * new_class_count * rows; + unsigned prime_id = c.create_node (size); + if (prime_id == (unsigned) -1) return -1; + AnchorMatrix* prime = (AnchorMatrix*) c.graph.object (prime_id).head; + prime->rows = base_count; + + for (unsigned base = 0; base < base_count; base++) + { + for (unsigned klass = start; klass < end; klass++) + { + unsigned new_klass = klass - start; + + unsigned old_index = base * class_count + klass; + unsigned new_index = base * new_class_count + new_klass; + + unsigned offset_pos = (char*) &(this->matrixZ[old_index]) - + (char*) this; + unsigned* objidx; + if (pos_to_index.has (offset_pos, &objidx)) + c.graph.move_child (this_index, + &(this->matrixZ[old_index]), + prime_id, + &(prime->matrixZ[new_index])); + } + } + + return prime_id; } }; @@ -325,7 +354,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone (sc.c, mark_array.index, sc.base_array_links, - start, end); + start, end, this->classCount); graph.add_link (&(prime->baseArray), prime_id, new_base_array); return prime_id; From bbe14417ad7fcc80b49d0e8426ad757fc7689ccc Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 22:53:30 +0000 Subject: [PATCH 08/29] [repacker] Begin implementing MarkBasePosFormat1::shrink. --- src/graph/markbasepos-graph.hh | 72 ++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 652dc2d92..c269fba97 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -204,6 +204,18 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 mark_array_links; hb_hashmap_t base_array_links; + hb_set_t marks_for (unsigned start, unsigned end) + { + hb_set_t marks; + for (unsigned klass = start; klass < end; klass++) + { + + class_to_info[klass].marks.iter () + | hb_sink (marks) + ; + } + return marks; + } + unsigned original_count () { return thiz->classCount; @@ -259,37 +271,38 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2= old_count) return true; - pairSet.len = count; - c.graph.vertices_[this_index].obj.tail -= (old_count - count) * SmallTypes::size; + classCount = count; - unsigned coverage_id = c.graph.mutable_index_for_offset (this_index, &coverage); - unsigned coverage_size = c.graph.vertices_[coverage_id].table_size (); - auto& coverage_v = c.graph.vertices_[coverage_id]; - - Coverage* coverage_table = (Coverage*) coverage_v.obj.head; - if (!coverage_table || !coverage_table->sanitize (coverage_v)) + auto mark_coverage = sc.c.graph.as_table (this_index, + &markCoverage); + if (!mark_coverage) return false; + hb_set_t marks = sc.marks_for (0, count); + auto new_coverage = + + hb_zip (hb_range (), mark_coverage.table->iter ()) + | hb_filter (marks, hb_first) + | hb_map_retains_sorting (hb_second) + ; + if (!Coverage::make_coverage (sc.c, + new_coverage, + mark_coverage.index, + 4 + 2 * marks.get_population ())) return false; - 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) - ; + // TODO: markArray, baseArray + /* + return Coverage::make_coverage (c, new_coverage, coverage_id, coverage_size); */ @@ -305,8 +318,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2::static_size; @@ -322,18 +333,19 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2baseCoverage), prime_id, base_coverage_id); graph.duplicate (prime_id, base_coverage_id); - hb_set_t marks; - for (unsigned klass = start; klass < end; klass++) - { - + sc.class_to_info[klass].marks.iter () - | hb_sink (marks) - ; - } - + auto mark_coverage = sc.c.graph.as_table (this_index, + &markCoverage); + if (!mark_coverage) return false; + hb_set_t marks = sc.marks_for (start, end); + auto new_coverage = + + hb_zip (hb_range (), mark_coverage.table->iter ()) + | hb_filter (marks, hb_first) + | hb_map_retains_sorting (hb_second) + ; if (!Coverage::add_coverage (sc.c, prime_id, 2, - + marks.iter (), + + new_coverage, marks.get_population () * 2 + 4)) return -1; From f8b55205569aacb81f533179c0c0644d471b2aab Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 11 Aug 2022 23:09:36 +0000 Subject: [PATCH 09/29] [repacker] Add AnchorMatrix::shrink. --- src/graph/markbasepos-graph.hh | 41 +++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index c269fba97..5ff7fbdf4 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -42,6 +42,36 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix return false; } + bool shrink (gsubgpos_graph_context_t& c, + unsigned this_index, + unsigned old_class_count, + unsigned new_class_count) + { + if (new_class_count >= old_class_count) return false; + auto& o = c.graph.vertices_[this_index].obj; + unsigned base_count = rows; + o.tail = o.head + + AnchorMatrix::min_size + + OT::Offset16::static_size * base_count * new_class_count; + + // Reposition links into the new indexing scheme. + for (auto& link : o.real_links.writer ()) + { + unsigned index = (link.position - 2) / 2; + unsigned base = index / old_class_count; + unsigned klass = index % old_class_count; + if (klass >= new_class_count) + // should have already been removed + return false; + + unsigned new_index = base * new_class_count + klass; + + link.position = (char*) &(this->matrixZ[new_index]) - (char*) this; + } + + return true; + } + unsigned clone (gsubgpos_graph_context_t& c, unsigned this_index, const hb_hashmap_t& pos_to_index, @@ -300,7 +330,16 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 (this_index, + &baseArray); + if (!base_array || !base_array.table->shrink (sc.c, + base_array.index, + old_count, + count)) + return false; + + // TODO: markArray /* From c414ef292b7dee52cdf4fb8afaa8f0835b58749b Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 15 Aug 2022 22:10:37 +0000 Subject: [PATCH 10/29] [repacker] Implement MarkArray::shrink. --- src/graph/markbasepos-graph.hh | 46 +++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 5ff7fbdf4..aac19cf94 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -120,6 +120,38 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray return false; } + bool shrink (gsubgpos_graph_context_t& c, + const hb_hashmap_t& mark_array_links, + unsigned this_index, + unsigned new_class_count) + { + auto& o = c.graph.vertices_[this_index].obj; + o.real_links.reset (); + + unsigned new_index = 0; + for (const auto& record : this->iter ()) + { + unsigned klass = record.klass; + if (klass >= new_class_count) continue; + + (*this)[new_index].klass = klass; + unsigned position = (char*) &record.markAnchor - (char*) this; + unsigned* objidx; + if (!mark_array_links.has (position, &objidx)) + { + new_index++; + continue; + } + + c.graph.add_link (&(*this)[new_index].markAnchor, this_index, *objidx); + new_index++; + } + + this->len = new_index; + o.tail = o.head + MarkArray::min_size + OT::Offset16::static_size * new_index; + return true; + } + unsigned clone (gsubgpos_graph_context_t& c, unsigned this_index, const hb_hashmap_t& pos_to_index, @@ -339,13 +371,15 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 (this_index, + &markArray); + if (!mark_array || !mark_array.table->shrink (sc.c, + sc.mark_array_links, + mark_array.index, + count)) + return false; - - return Coverage::make_coverage (c, new_coverage, coverage_id, coverage_size); - */ - return -1; // TODO + return true; } // Create a new PairPos including PairSet's from start (inclusive) to end (exclusive). From 5cf2a25a609a7401b0799b52a972dd10af245aad Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 15 Aug 2022 22:49:24 +0000 Subject: [PATCH 11/29] [repacker] Expose on internal method in the repacker that allows the caller to pass in/out a graph. Will be used in testing so we can compare graphs instead of packed result. --- src/graph/graph.hh | 11 +++++- src/graph/gsubgpos-context.cc | 5 ++- src/graph/gsubgpos-context.hh | 10 ++---- src/hb-repacker.hh | 67 ++++++++++++++++++++--------------- 4 files changed, 53 insertions(+), 40 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 5f2341420..0d3dae2c0 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -196,7 +196,8 @@ struct graph_t : parents_invalid (true), distance_invalid (true), positions_invalid (true), - successful (true) + successful (true), + buffers () { num_roots_for_space_.push (1); bool removed_nil = false; @@ -228,6 +229,8 @@ struct graph_t ~graph_t () { vertices_.fini (); + for (char* b : buffers) + hb_free (b); } bool in_error () const @@ -255,6 +258,11 @@ struct graph_t return vertices_[i].obj; } + void add_buffer (char* buffer) + { + buffers.push (buffer); + } + /* * Adds a 16 bit link from parent_id to child_id */ @@ -1127,6 +1135,7 @@ struct graph_t bool positions_invalid; bool successful; hb_vector_t num_roots_for_space_; + hb_vector_t buffers; }; } diff --git a/src/graph/gsubgpos-context.cc b/src/graph/gsubgpos-context.cc index e0ff6ff85..b2044426d 100644 --- a/src/graph/gsubgpos-context.cc +++ b/src/graph/gsubgpos-context.cc @@ -33,8 +33,7 @@ gsubgpos_graph_context_t::gsubgpos_graph_context_t (hb_tag_t table_tag_, : table_tag (table_tag_), graph (graph_), lookup_list_index (0), - lookups (), - buffers () + lookups () { if (table_tag_ != HB_OT_TAG_GPOS && table_tag_ != HB_OT_TAG_GSUB) @@ -53,7 +52,7 @@ unsigned gsubgpos_graph_context_t::create_node (unsigned size) if (!buffer) return -1; - buffers.push (buffer); + add_buffer (buffer); return graph.new_node (buffer, buffer + size); } diff --git a/src/graph/gsubgpos-context.hh b/src/graph/gsubgpos-context.hh index 49b24198f..9fe9662e6 100644 --- a/src/graph/gsubgpos-context.hh +++ b/src/graph/gsubgpos-context.hh @@ -40,22 +40,16 @@ struct gsubgpos_graph_context_t graph_t& graph; 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_); - ~gsubgpos_graph_context_t () - { - for (char* b : buffers) - hb_free (b); - } - HB_INTERNAL unsigned create_node (unsigned size); void add_buffer (char* buffer) { - buffers.push (buffer); + graph.add_buffer (buffer); } private: diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh index 61b142238..48a3fe456 100644 --- a/src/hb-repacker.hh +++ b/src/hb-repacker.hh @@ -276,35 +276,20 @@ bool _process_overflows (const hb_vector_t& overflows, return resolution_attempted; } -/* - * Attempts to modify the topological sorting of the provided object graph to - * eliminate offset overflows in the links between objects of the graph. If a - * non-overflowing ordering is found the updated graph is serialized it into the - * provided serialization context. - * - * If necessary the structure of the graph may be modified in ways that do not - * affect the functionality of the graph. For example shared objects may be - * duplicated. - * - * For a detailed writeup describing how the algorithm operates see: - * docs/repacker.md - */ -template -inline hb_blob_t* -hb_resolve_overflows (const T& packed, - hb_tag_t table_tag, - unsigned max_rounds = 20, - bool recalculate_extensions = false) { - graph_t sorted_graph (packed); +inline bool +hb_resolve_graph_overflows (hb_tag_t table_tag, + unsigned max_rounds , + bool recalculate_extensions, + graph_t& sorted_graph /* IN/OUT */) +{ sorted_graph.sort_shortest_distance (); bool will_overflow = graph::will_overflow (sorted_graph); if (!will_overflow) - { - return graph::serialize (sorted_graph); - } + return true; - graph::gsubgpos_graph_context_t ext_context (table_tag, sorted_graph); + // TODO attach to graph? or just move buffers to the graph? + graph::gsubgpos_graph_context_t ext_context (table_tag, sorted_graph); // TODO lifetime if ((table_tag == HB_OT_TAG_GPOS || table_tag == HB_OT_TAG_GSUB) && will_overflow) @@ -314,13 +299,13 @@ hb_resolve_overflows (const T& packed, 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; + return false; } 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; + return false; } } @@ -360,15 +345,41 @@ hb_resolve_overflows (const T& packed, if (sorted_graph.in_error ()) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Sorted graph in error state."); - return nullptr; + return false; } if (graph::will_overflow (sorted_graph)) { DEBUG_MSG (SUBSET_REPACK, nullptr, "Offset overflow resolution failed."); - return nullptr; + return false; } + return true; +} + +/* + * Attempts to modify the topological sorting of the provided object graph to + * eliminate offset overflows in the links between objects of the graph. If a + * non-overflowing ordering is found the updated graph is serialized it into the + * provided serialization context. + * + * If necessary the structure of the graph may be modified in ways that do not + * affect the functionality of the graph. For example shared objects may be + * duplicated. + * + * For a detailed writeup describing how the algorithm operates see: + * docs/repacker.md + */ +template +inline hb_blob_t* +hb_resolve_overflows (const T& packed, + hb_tag_t table_tag, + unsigned max_rounds = 20, + bool recalculate_extensions = false) { + graph_t sorted_graph (packed); + if (!hb_resolve_graph_overflows (table_tag, max_rounds, recalculate_extensions, sorted_graph)) + return nullptr; + return graph::serialize (sorted_graph); } From 07fd0528c0eb5d77ac65cf8cef3328df34f24889 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 15 Aug 2022 23:16:51 +0000 Subject: [PATCH 12/29] [repacker] add graph equality check. Does not compare topological sorting, but looks for equivalence of the two graphs. --- src/graph/graph.hh | 53 +++++++++++++++++++++++++++++++++++++++++++++ src/hb-serialize.hh | 5 ++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 0d3dae2c0..3479e002c 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -49,6 +49,21 @@ struct graph_t unsigned end = 0; unsigned priority = 0; + bool equals (vertex_t& other, graph_t& graph) + { + if (as_bytes () != other.as_bytes ()) + return false; + + obj.real_links.qsort (); + other.obj.real_links.qsort (); + return links_equal (graph, obj.real_links, other.obj.real_links); + } + + hb_bytes_t as_bytes () + { + return hb_bytes_t (obj.head, table_size ()); + } + friend void swap (vertex_t& a, vertex_t& b) { hb_swap (a.obj, b.obj); @@ -167,6 +182,39 @@ struct graph_t return -table_size; } + + private: + bool links_equal (graph_t& graph, + const hb_vector_t& this_links, + const hb_vector_t& other_links) const + { + auto a = this_links.iter (); + auto b = other_links.iter (); + + while (a && b) + { + const auto& link_a = *a; + const auto& link_b = *b; + + if (link_a.width != link_b.width || + link_a.is_signed != link_b.is_signed || + link_a.whence != link_b.whence || + link_a.position != link_b.position || + link_a.bias != link_b.bias) + return false; + + if (!graph.vertices_[link_a.objidx].equals (graph.vertices_[link_b.objidx], graph)) + return false; + + a++; + b++; + } + + if (bool (a) != bool (b)) + return false; + + return true; + } }; template @@ -233,6 +281,11 @@ struct graph_t hb_free (b); } + bool operator== (graph_t& other) + { + return vertices_[root_idx ()].equals (other.vertices_[other.root_idx ()], *this); + } + bool in_error () const { return !successful || diff --git a/src/hb-serialize.hh b/src/hb-serialize.hh index 4b22e46a5..f47cde5eb 100644 --- a/src/hb-serialize.hh +++ b/src/hb-serialize.hh @@ -142,7 +142,10 @@ struct hb_serialize_context_t HB_INTERNAL static int cmp (const void* a, const void* b) { - return ((const link_t*)a)->position - ((const link_t*)b)->position; + int cmp = ((const link_t*)a)->position - ((const link_t*)b)->position; + if (cmp) return cmp; + + return ((const link_t*)a)->objidx - ((const link_t*)b)->objidx; } }; From 1405f96b6f211a6353fa8ad431de5486c8dcd309 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 15 Aug 2022 23:48:00 +0000 Subject: [PATCH 13/29] [repacker] change run_resolve_overflow_test to check for graph equivalence. Replaces a check for an exact match on the final serialized bytes. The previous check enforced equivalent topological sorting between result and expected, but we only really care that the graph's are equivalent and don't overflow. --- src/graph/graph.hh | 33 ++++++++++++++++++++++------- src/test-repacker.cc | 50 +++++++++++++++----------------------------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 3479e002c..347a2b012 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -49,17 +49,27 @@ struct graph_t unsigned end = 0; unsigned priority = 0; - bool equals (vertex_t& other, graph_t& graph) + void normalize () { - if (as_bytes () != other.as_bytes ()) + obj.real_links.qsort (); + for (auto& l : obj.real_links) + { + for (unsigned i = 0; i < l.width; i++) + { + obj.head[l.position + i] = 0; + } + } + } + + bool equals (const vertex_t& other, const graph_t& graph) const + { + if (!(as_bytes () == other.as_bytes ())) return false; - obj.real_links.qsort (); - other.obj.real_links.qsort (); return links_equal (graph, obj.real_links, other.obj.real_links); } - hb_bytes_t as_bytes () + hb_bytes_t as_bytes () const { return hb_bytes_t (obj.head, table_size ()); } @@ -184,7 +194,7 @@ struct graph_t } private: - bool links_equal (graph_t& graph, + bool links_equal (const graph_t& graph, const hb_vector_t& this_links, const hb_vector_t& other_links) const { @@ -281,9 +291,16 @@ struct graph_t hb_free (b); } - bool operator== (graph_t& other) + bool operator== (const graph_t& other) const { - return vertices_[root_idx ()].equals (other.vertices_[other.root_idx ()], *this); + return root ().equals (other.root (), *this); + } + + // Sorts links of all objects in a consistent manner and zeroes all offsets. + void normalize () + { + for (auto& v : vertices_.writer ()) + v.normalize (); } bool in_error () const diff --git a/src/test-repacker.cc b/src/test-repacker.cc index e6ac36e7a..0afd6f2b1 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -309,44 +309,28 @@ static void run_resolve_overflow_test (const char* name, name); graph_t graph (overflowing.object_graph ()); + graph_t expected_graph (expected.object_graph ()); + // Check that overflow resolution succeeds assert (overflowing.offset_overflow ()); - hb_blob_t* out = hb_resolve_overflows (overflowing.object_graph (), - tag, - num_iterations, - recalculate_extensions); + assert (hb_resolve_graph_overflows (tag, + num_iterations, + recalculate_extensions, + graph)); + + // Check the graphs can be serialized. + hb_blob_t* out = graph::serialize (graph); assert (out); - - hb_bytes_t result = out->as_bytes (); - - 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; - for (unsigned i = 0; i < expected_result.length; i++) - { - if (result[i] != expected_result[i]) - { - equal = false; - uint8_t a = result[i]; - uint8_t b = expected_result[i]; - printf("%08u: %x != %x\n", i, a, b); - } - } - - assert (equal); - - expected_result.fini (); hb_blob_destroy (out); + out = graph::serialize (expected_graph); + assert (out); + hb_blob_destroy (out); + + // Check the graphs are equivalent + graph.normalize (); + expected_graph.normalize (); + assert (graph == expected_graph); } static void add_virtual_offset (unsigned id, From 8c3db8bdfd675b226a1c04ec09cd085284858211 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 17 Aug 2022 00:36:23 +0000 Subject: [PATCH 14/29] [repacker] more progress on MarkBasePos tests. --- src/graph/gsubgpos-graph.hh | 7 +- src/graph/markbasepos-graph.hh | 11 +- src/test-repacker.cc | 177 +++++++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 7 deletions(-) diff --git a/src/graph/gsubgpos-graph.hh b/src/graph/gsubgpos-graph.hh index 31fc1d8e4..a93e7d1c7 100644 --- a/src/graph/gsubgpos-graph.hh +++ b/src/graph/gsubgpos-graph.hh @@ -122,7 +122,9 @@ struct Lookup : public OT::Lookup if (c.table_tag != HB_OT_TAG_GPOS) return true; - if (!is_ext && type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair) + if (!is_ext && + type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair && + type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::MarkBase) return true; hb_vector_t>> all_new_subtables; @@ -139,7 +141,8 @@ struct Lookup : public OT::Lookup 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) + if (type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair + && type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::MarkBase) continue; } diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index aac19cf94..9326de9cd 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -39,7 +39,7 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix bool sanitize (graph_t::vertex_t& vertex) const { // TODO - return false; + return true; } bool shrink (gsubgpos_graph_context_t& c, @@ -117,7 +117,7 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray bool sanitize (graph_t::vertex_t& vertex) const { // TODO - return false; + return true; } bool shrink (gsubgpos_graph_context_t& c, @@ -399,7 +399,8 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2format = this->format; - prime->classCount = this->classCount; + unsigned new_class_count = end - start; + prime->classCount = new_class_count; unsigned base_coverage_id = graph.index_for_offset (sc.this_index, &baseCoverage); @@ -437,7 +438,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone (sc.c, - mark_array.index, + base_array.index, sc.base_array_links, start, end, this->classCount); graph.add_link (&(prime->baseArray), prime_id, new_base_array); @@ -471,7 +472,7 @@ struct MarkBasePos : public OT::Layout::GPOS_impl::MarkBasePos switch (u.format) { case 1: - return ((PairPosFormat1*)(&u.format1))->sanitize (vertex); + return ((MarkBasePosFormat1*)(&u.format1))->sanitize (vertex); #ifndef HB_NO_BORING_EXPANSION case 2: HB_FALLTHROUGH; #endif diff --git a/src/test-repacker.cc b/src/test-repacker.cc index 0afd6f2b1..53b872ef9 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -297,6 +297,123 @@ static unsigned add_pair_pos_2 (unsigned starting_class, return c->pop_pack (false); } +static unsigned add_mark_base_pos_1 (unsigned mark_coverage, + unsigned base_coverage, + unsigned mark_array, + unsigned base_array, + unsigned class_count, + hb_serialize_context_t* c) +{ + uint8_t format[] = { + 0, 1 + }; + + start_object ((char*) format, 2, c); + add_offset (mark_coverage, c); + add_offset (base_coverage, c); + + uint8_t count[] = { + (uint8_t) ((class_count >> 8) & 0xFF), + (uint8_t) (class_count & 0xFF), + }; + extend ((char*) count, 2, c); + + add_offset (mark_array, c); + add_offset (base_array, c); + + return c->pop_pack (false); +} + +template +struct MarkBasePosBuffers +{ + unsigned base_anchors[class_count * base_count]; + unsigned mark_anchors[mark_count]; + uint32_t anchors_buffer[class_count * base_count]; + uint8_t class_buffer[class_count * 2]; + + MarkBasePosBuffers(hb_serialize_context_t* c) + { + for (unsigned i = 0; i < class_count * base_count; i++) + { + anchors_buffer[i] = i; + base_anchors[i] = add_object ((char*) &anchors_buffer[i], 4, c); + if (i < class_count) { + class_buffer[i*2] = (uint8_t) ((i >> 8) & 0xFF); + class_buffer[i*2 + 1] = (uint8_t) (i & 0xFF); + } + } + + for (unsigned i = 0; i < mark_count; i++) + { + mark_anchors[i] = add_object ((char*) &anchors_buffer[i], 4, c); + } + } + + unsigned create_mark_base_pos_1 (unsigned table_index, hb_serialize_context_t* c) + { + unsigned class_per_table = class_count / table_count; + unsigned mark_per_class = mark_count / class_count; + unsigned start_class = class_per_table * table_index; + unsigned end_class = class_per_table * (table_index + 1) - 1; + + // baseArray + uint8_t base_count_buffer[] = { + (uint8_t) ((base_count >> 8) & 0xFF), + (uint8_t) (base_count & 0xFF), + + }; + start_object ((char*) base_count_buffer, 2, c); + for (unsigned base = 0; base < base_count; base++) + { + for (unsigned klass = start_class; klass <= end_class; klass++) + { + unsigned i = base * class_count + klass; + add_offset (base_anchors[i], c); + } + } + unsigned base_array = c->pop_pack (false); + + // markArray + unsigned num_marks = class_per_table * mark_per_class; + uint8_t mark_count_buffer[] = { + (uint8_t) ((num_marks >> 8) & 0xFF), + (uint8_t) (num_marks & 0xFF), + }; + start_object ((char*) mark_count_buffer, 2, c); + for (unsigned i = 0; i < num_marks; i++) + { + unsigned klass = i % class_per_table; + extend ((char*) &class_buffer[2 * klass], 2, c); + + unsigned mark = table_index * num_marks + i; + add_offset (mark_anchors[mark], c); + } + unsigned mark_array = c->pop_pack (false); + + // markCoverage + unsigned mark_coverage = add_coverage (num_marks * table_index, + num_marks * (table_index + 1) - 1, + c); + + // baseCoverage + unsigned base_coverage = add_coverage (10, 10 + base_count - 1, c); + + return add_mark_base_pos_1 (mark_coverage, + base_coverage, + mark_array, + base_array, + class_count, + c); + } +}; + + + + static void run_resolve_overflow_test (const char* name, hb_serialize_context_t& overflowing, @@ -1292,6 +1409,38 @@ populate_serializer_with_large_pair_pos_2 (hb_serialize_context_t* c, free (device_tables); } +template +static void +populate_serializer_with_large_mark_base_pos_1 (hb_serialize_context_t* c) +{ + c->start_serialize (); + + MarkBasePosBuffers buffers (c); + + unsigned mark_base_pos[table_count]; + for (unsigned i = 0; i < table_count; i++) + mark_base_pos[i] = buffers.create_mark_base_pos_1 (i, c); + + for (int i = 0; i < table_count; i++) + mark_base_pos[i] = add_extension (mark_base_pos[i], 4, c); + + start_lookup (9, table_count, c); + + for (int i = 0; i < table_count; i++) + add_offset (mark_base_pos[i], c); + + unsigned lookup = finish_lookup (c); + + unsigned lookup_list = add_lookup_list (&lookup, 1, c); + + add_gsubgpos_header (lookup_list, c); + + c->end_serialize(); +} + static void test_sort_shortest () { size_t buffer_size = 100; @@ -1776,6 +1925,29 @@ static void test_resolve_with_pair_pos_2_split_with_device_tables () free (expected_buffer); } +static void test_resolve_with_basic_mark_base_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_mark_base_pos_1 <40, 10, 1200, 1>(&c); + + void* expected_buffer = malloc (buffer_size); + assert (expected_buffer); + hb_serialize_context_t e (expected_buffer, buffer_size); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 1050, 2>(&e); + + run_resolve_overflow_test ("test_resolve_with_basic_mark_base_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; @@ -1901,6 +2073,8 @@ test_shared_node_with_virtual_links () int main (int argc, char **argv) { + if (0) + { test_serialize (); test_sort_shortest (); test_will_overflow_1 (); @@ -1928,6 +2102,9 @@ main (int argc, char **argv) test_resolve_with_basic_pair_pos_2_split (); test_resolve_with_pair_pos_2_split_with_device_tables (); test_resolve_with_close_to_limit_pair_pos_2_split (); + } + + test_resolve_with_basic_mark_base_pos_1_split (); // TODO(grieger): have run overflow tests compare graph equality not final packed binary. // TODO(grieger): split test where multiple subtables in one lookup are split to test link ordering. From 36c76c27c6078d9e494f27ce04b0160c906bc444 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 17 Aug 2022 17:30:21 +0000 Subject: [PATCH 15/29] [repacker] when clearing links in MarkArray, also clear parents of the children. --- src/graph/markbasepos-graph.hh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 9326de9cd..86ed3ca6b 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -126,6 +126,8 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray unsigned new_class_count) { auto& o = c.graph.vertices_[this_index].obj; + for (const auto& link : o.real_links) + c.graph.vertices_[link.objidx].remove_parent (this_index); o.real_links.reset (); unsigned new_index = 0; From b46ced956285b1e554dea2086dbe48d78a458692 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 17 Aug 2022 17:51:02 +0000 Subject: [PATCH 16/29] [repacker] correct MarkArray size calculation. --- src/graph/markbasepos-graph.hh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 86ed3ca6b..390bc7249 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -150,7 +150,8 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray } this->len = new_index; - o.tail = o.head + MarkArray::min_size + OT::Offset16::static_size * new_index; + o.tail = o.head + MarkArray::min_size + + OT::Layout::GPOS_impl::MarkRecord::static_size * new_index; return true; } From 19c51ed35c65f1f4758489645e24939f7b92cea6 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 17 Aug 2022 19:15:55 +0000 Subject: [PATCH 17/29] [repacker] Get mark base pos test working. --- src/test-repacker.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index 53b872ef9..bc3cc3f1a 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -427,7 +427,11 @@ static void run_resolve_overflow_test (const char* name, graph_t graph (overflowing.object_graph ()); graph_t expected_graph (expected.object_graph ()); - + if (graph::will_overflow (expected_graph)) + { + expected_graph.assign_spaces (); + expected_graph.sort_shortest_distance (); + } // Check that overflow resolution succeeds assert (overflowing.offset_overflow ()); @@ -1931,12 +1935,12 @@ static void test_resolve_with_basic_mark_base_pos_1_split () void* buffer = malloc (buffer_size); assert (buffer); hb_serialize_context_t c (buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 1200, 1>(&c); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 2000, 1>(&c); void* expected_buffer = malloc (buffer_size); assert (expected_buffer); hb_serialize_context_t e (expected_buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 1050, 2>(&e); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 2000, 2>(&e); run_resolve_overflow_test ("test_resolve_with_basic_mark_base_pos_1_split", c, @@ -2073,8 +2077,6 @@ test_shared_node_with_virtual_links () int main (int argc, char **argv) { - if (0) - { test_serialize (); test_sort_shortest (); test_will_overflow_1 (); @@ -2102,8 +2104,6 @@ main (int argc, char **argv) test_resolve_with_basic_pair_pos_2_split (); test_resolve_with_pair_pos_2_split_with_device_tables (); test_resolve_with_close_to_limit_pair_pos_2_split (); - } - test_resolve_with_basic_mark_base_pos_1_split (); // TODO(grieger): have run overflow tests compare graph equality not final packed binary. From a3ed9f9099c8e6d951eb8d1aeda9bdc5278fa4a0 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Wed, 17 Aug 2022 23:39:11 +0000 Subject: [PATCH 18/29] [repacker] fix graph comparison, and mark base pos generation for the tests. --- src/graph/graph.hh | 39 +++++++++++++++++++++++++++------- src/graph/markbasepos-graph.hh | 8 ++++--- src/test-repacker.cc | 33 +++++++++++++++++++++------- 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 347a2b012..a69deb00a 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -61,12 +61,32 @@ struct graph_t } } - bool equals (const vertex_t& other, const graph_t& graph) const + bool equals (const vertex_t& other, + const graph_t& graph, + const graph_t& other_graph, + unsigned depth) const { if (!(as_bytes () == other.as_bytes ())) - return false; + { + DEBUG_MSG (SUBSET_REPACK, nullptr, + "vertex [%lu] bytes != [%lu] bytes, depth = %u", + table_size (), + other.table_size (), + depth); - return links_equal (graph, obj.real_links, other.obj.real_links); + auto a = as_bytes (); + auto b = other.as_bytes (); + while (a || b) + { + DEBUG_MSG (SUBSET_REPACK, nullptr, + " 0x%x %s 0x%x", *a, (*a == *b) ? "==" : "!=", *b); + a++; + b++; + } + return false; + } + + return links_equal (obj.real_links, other.obj.real_links, graph, other_graph, depth); } hb_bytes_t as_bytes () const @@ -194,9 +214,11 @@ struct graph_t } private: - bool links_equal (const graph_t& graph, - const hb_vector_t& this_links, - const hb_vector_t& other_links) const + bool links_equal (const hb_vector_t& this_links, + const hb_vector_t& other_links, + const graph_t& graph, + const graph_t& other_graph, + unsigned depth) const { auto a = this_links.iter (); auto b = other_links.iter (); @@ -213,7 +235,8 @@ struct graph_t link_a.bias != link_b.bias) return false; - if (!graph.vertices_[link_a.objidx].equals (graph.vertices_[link_b.objidx], graph)) + if (!graph.vertices_[link_a.objidx].equals ( + other_graph.vertices_[link_b.objidx], graph, other_graph, depth + 1)) return false; a++; @@ -293,7 +316,7 @@ struct graph_t bool operator== (const graph_t& other) const { - return root ().equals (other.root (), *this); + return root ().equals (other.root (), *this, other, 0); } // Sorts links of all objects in a consistent manner and zeroes all offsets. diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 390bc7249..69f8fc72b 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -158,7 +158,8 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray unsigned clone (gsubgpos_graph_context_t& c, unsigned this_index, const hb_hashmap_t& pos_to_index, - hb_set_t& marks) + hb_set_t& marks, + unsigned start_class) { unsigned size = MarkArray::min_size + OT::Layout::GPOS_impl::MarkRecord::static_size * @@ -172,7 +173,7 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray unsigned i = 0; for (hb_codepoint_t mark : marks) { - (*prime)[i].klass = (*this)[mark].klass; + (*prime)[i].klass = (*this)[mark].klass - start_class; unsigned offset_pos = (char*) &((*this)[mark].markAnchor) - (char*) this; unsigned* anchor_index; if (pos_to_index.has (offset_pos, &anchor_index)) @@ -433,7 +434,8 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone (sc.c, mark_array.index, sc.mark_array_links, - marks); + marks, + start); graph.add_link (&(prime->markArray), prime_id, new_mark_array); auto base_array = diff --git a/src/test-repacker.cc b/src/test-repacker.cc index bc3cc3f1a..9ab328047 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -177,6 +177,16 @@ static unsigned add_coverage (unsigned start, unsigned end, return add_object ((char*) coverage, 10, c); } + +template +static unsigned add_coverage (It it, + hb_serialize_context_t* c) +{ + c->push (); + OT::Layout::Common::Coverage_serialize (c, it); + return c->pop_pack (false); +} + // Adds a class that maps glyphs from [start_glyph, end_glyph) // to classes 1...n static unsigned add_class_def (uint16_t start_glyph, @@ -384,20 +394,27 @@ struct MarkBasePosBuffers (uint8_t) (num_marks & 0xFF), }; start_object ((char*) mark_count_buffer, 2, c); - for (unsigned i = 0; i < num_marks; i++) + for (unsigned mark = 0; mark < mark_count; mark++) { - unsigned klass = i % class_per_table; - extend ((char*) &class_buffer[2 * klass], 2, c); + unsigned klass = mark % class_count; + if (klass < start_class || klass > end_class) continue; + klass -= start_class; - unsigned mark = table_index * num_marks + i; + extend ((char*) &class_buffer[2 * klass], 2, c); add_offset (mark_anchors[mark], c); } unsigned mark_array = c->pop_pack (false); // markCoverage - unsigned mark_coverage = add_coverage (num_marks * table_index, - num_marks * (table_index + 1) - 1, - c); + auto it = + + hb_range ((hb_codepoint_t) mark_count) + | hb_filter ([&] (hb_codepoint_t mark) { + unsigned klass = mark % class_count; + return klass >= class_per_table * table_index && + klass < class_per_table * (table_index + 1); + }) + ; + unsigned mark_coverage = add_coverage (it, c); // baseCoverage unsigned base_coverage = add_coverage (10, 10 + base_count - 1, c); @@ -406,7 +423,7 @@ struct MarkBasePosBuffers base_coverage, mark_array, base_array, - class_count, + class_per_table, c); } }; From ac1a853abc48da5f89e4cba6c28d2657ca1fb118 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 00:55:47 +0000 Subject: [PATCH 19/29] [repacker] implement sanitize methods for MarkBasePos. --- src/graph/graph.hh | 12 ++++++------ src/graph/markbasepos-graph.hh | 28 ++++++++++++++++------------ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index a69deb00a..4a036af87 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -489,14 +489,14 @@ struct graph_t } } - template - vertex_and_table_t as_table (unsigned parent, const void* offset) + template + vertex_and_table_t as_table (unsigned parent, const void* offset, Ts... ds) { - return as_table (index_for_offset (parent, offset)); + return as_table_from_index (index_for_offset (parent, offset), std::forward(ds)...); } - template - vertex_and_table_t as_table (unsigned index) + template + vertex_and_table_t as_table_from_index (unsigned index, Ts... ds) { if (index >= vertices_.length) return vertex_and_table_t (); @@ -508,7 +508,7 @@ struct graph_t if (!r.table) return vertex_and_table_t (); - if (!r.table->sanitize (*(r.vertex))) + if (!r.table->sanitize (*(r.vertex), std::forward(ds)...)) return vertex_and_table_t (); return r; diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 69f8fc72b..d4dd39a98 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -36,10 +36,13 @@ namespace graph { struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix { - bool sanitize (graph_t::vertex_t& vertex) const + bool sanitize (graph_t::vertex_t& vertex, unsigned class_count) const { - // TODO - return true; + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + if (vertex_len < AnchorMatrix::min_size) return false; + + return vertex_len >= AnchorMatrix::min_size + + OT::Offset16::static_size * class_count * this->rows; } bool shrink (gsubgpos_graph_context_t& c, @@ -116,8 +119,11 @@ struct MarkArray : public OT::Layout::GPOS_impl::MarkArray { bool sanitize (graph_t::vertex_t& vertex) const { - // TODO - return true; + int64_t vertex_len = vertex.obj.tail - vertex.obj.head; + unsigned min_size = MarkArray::min_size; + if (vertex_len < min_size) return false; + + return vertex_len >= get_size (); } bool shrink (gsubgpos_graph_context_t& c, @@ -194,11 +200,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2::min_size; - if (vertex_len < min_size) return false; - - // TODO - return true; + return vertex_len >= MarkBasePosFormat1::static_size; } hb_vector_t split_subtables (gsubgpos_graph_context_t& c, unsigned this_index) @@ -368,7 +370,8 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 (this_index, - &baseArray); + &baseArray, + old_count); if (!base_array || !base_array.table->shrink (sc.c, base_array.index, old_count, @@ -438,8 +441,9 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2markArray), prime_id, new_mark_array); + unsigned class_count = classCount; auto base_array = - graph.as_table (sc.this_index, &baseArray); + graph.as_table (sc.this_index, &baseArray, class_count); if (!base_array) return -1; unsigned new_base_array = base_array.table->clone (sc.c, From 52303638b9446d9840aceecc0890790169a20f0f Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:10:42 +0000 Subject: [PATCH 20/29] [repacker] correct size calculation for MarkBasePosFormat1. --- src/graph/markbasepos-graph.hh | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index d4dd39a98..4ed418997 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -104,6 +104,8 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix (char*) this; unsigned* objidx; if (pos_to_index.has (offset_pos, &objidx)) + // TODO(garretrieger): move_child is O(n) (n is number of children), can use the pos_to_index + // map to speed it up. c.graph.move_child (this_index, &(this->matrixZ[old_index]), prime_id, @@ -210,21 +212,31 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2::min_size + + MarkArray::min_size + + AnchorMatrix::min_size + c.graph.vertices_[base_coverage_id].table_size (); hb_vector_t class_to_info = get_class_info (c, this_index); + unsigned class_count = classCount; + auto base_array = c.graph.as_table (this_index, + &baseArray, + class_count); + if (!base_array) return hb_vector_t (); + unsigned base_count = base_array.table->rows; + unsigned partial_coverage_size = 4; unsigned accumulated = base_size; hb_vector_t split_points; - unsigned class_count = classCount; + for (unsigned klass = 0; klass < class_count; klass++) { class_info_t& info = class_to_info[klass]; partial_coverage_size += OT::HBUINT16::static_size * info.marks.get_population (); - unsigned accumulated_delta = OT::Layout::GPOS_impl::MarkRecord::static_size * info.marks.get_population (); - // TODO this doesn't count up null offsets. - accumulated_delta += OT::Offset16::static_size * info.child_indices.length; + unsigned accumulated_delta = + OT::Layout::GPOS_impl::MarkRecord::static_size * info.marks.get_population () + + OT::Offset16::static_size * base_count; + for (unsigned objidx : info.child_indices) accumulated_delta += c.graph.find_subgraph_size (objidx, visited); From 46b5dbd7ce05841d37b009c2d35004c147f44934 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:18:16 +0000 Subject: [PATCH 21/29] [repacker] optimize index_for_offset. --- src/graph/graph.hh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/graph/graph.hh b/src/graph/graph.hh index 4a036af87..ab95a71c4 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -521,8 +521,11 @@ struct graph_t const auto& node = object (node_idx); if (offset < node.head || offset >= node.tail) return -1; - for (const auto& link : node.real_links) + unsigned length = node.real_links.length; + for (unsigned i = 0; i < length; i++) { + // Use direct access for increased performance, this is a hot method. + const auto& link = node.real_links.arrayZ[i]; if (offset != node.head + link.position) continue; return link.objidx; From 29e3b2467e4b050f0aca8b27a6adb0af2d114323 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:19:54 +0000 Subject: [PATCH 22/29] [repacker] optimzie remove_real_links as it's a hot method. --- 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 ab95a71c4..64878a84a 100644 --- a/src/graph/graph.hh +++ b/src/graph/graph.hh @@ -141,7 +141,7 @@ struct graph_t { for (unsigned i = 0; i < obj.real_links.length; i++) { - auto& link = obj.real_links[i]; + auto& link = obj.real_links.arrayZ[i]; if (link.objidx != child_index) continue; From 6f5c52b604a3bed2a7870be5283994d9a5483fd6 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:48:10 +0000 Subject: [PATCH 23/29] [repacker] optimize AnchorMatrix::clone. Previous runtime is O(n^2) reduced to O(n). --- src/graph/markbasepos-graph.hh | 39 +++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 4ed418997..b40086dd6 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -91,26 +91,31 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix AnchorMatrix* prime = (AnchorMatrix*) c.graph.object (prime_id).head; prime->rows = base_count; - for (unsigned base = 0; base < base_count; base++) + auto& o = c.graph.vertices_[this_index].obj; + int num_links = o.real_links.length; + for (int i = 0; i < num_links; i++) { - for (unsigned klass = start; klass < end; klass++) - { - unsigned new_klass = klass - start; + const auto& link = o.real_links[i]; + unsigned old_index = (link.position - 2) / OT::Offset16::static_size; + unsigned klass = old_index % class_count; + if (klass < start || klass >= end) continue; - unsigned old_index = base * class_count + klass; - unsigned new_index = base * new_class_count + new_klass; + unsigned base = old_index / class_count; + unsigned new_klass = klass - start; + unsigned new_index = base * new_class_count + new_klass; - unsigned offset_pos = (char*) &(this->matrixZ[old_index]) - - (char*) this; - unsigned* objidx; - if (pos_to_index.has (offset_pos, &objidx)) - // TODO(garretrieger): move_child is O(n) (n is number of children), can use the pos_to_index - // map to speed it up. - c.graph.move_child (this_index, - &(this->matrixZ[old_index]), - prime_id, - &(prime->matrixZ[new_index])); - } + + unsigned child_idx = link.objidx; + c.graph.add_link (&(prime->matrixZ[new_index]), + prime_id, + child_idx); + + auto& child = c.graph.vertices_[child_idx]; + child.remove_parent (this_index); + + o.real_links.remove (i); + num_links--; + i--; } return prime_id; From 31976bfb502ae861e083b0933575c654292545f9 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:50:35 +0000 Subject: [PATCH 24/29] [repacker] cleanup unused base_array_links. --- src/graph/markbasepos-graph.hh | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index b40086dd6..326db29ba 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -77,7 +77,6 @@ struct AnchorMatrix : public OT::Layout::GPOS_impl::AnchorMatrix unsigned clone (gsubgpos_graph_context_t& c, unsigned this_index, - const hb_hashmap_t& pos_to_index, unsigned start, unsigned end, unsigned class_count) @@ -268,7 +267,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 (split_context, split_points); @@ -287,7 +285,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 class_to_info; hb_hashmap_t mark_array_links; - hb_hashmap_t base_array_links; hb_set_t marks_for (unsigned start, unsigned end) { @@ -465,7 +462,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2clone (sc.c, base_array.index, - sc.base_array_links, start, end, this->classCount); graph.add_link (&(prime->baseArray), prime_id, new_base_array); From bf28b84ae8cd58c1ca0d137f4a04d98d699ef9d6 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 01:51:37 +0000 Subject: [PATCH 25/29] [repacker] cleanup unused base_array_id. --- src/graph/markbasepos-graph.hh | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 326db29ba..4bbdf3ced 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -258,9 +258,6 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2 Date: Thu, 18 Aug 2022 20:28:05 +0000 Subject: [PATCH 26/29] [repacker] speed up MarkBasePos test case by using a smaller basecount. --- src/test-repacker.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index 9ab328047..f7bb1fdf7 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -342,7 +342,7 @@ struct MarkBasePosBuffers { unsigned base_anchors[class_count * base_count]; unsigned mark_anchors[mark_count]; - uint32_t anchors_buffer[class_count * base_count]; + uint64_t anchors_buffer[class_count * base_count]; uint8_t class_buffer[class_count * 2]; MarkBasePosBuffers(hb_serialize_context_t* c) @@ -350,7 +350,7 @@ struct MarkBasePosBuffers for (unsigned i = 0; i < class_count * base_count; i++) { anchors_buffer[i] = i; - base_anchors[i] = add_object ((char*) &anchors_buffer[i], 4, c); + base_anchors[i] = add_object ((char*) &anchors_buffer[i], 8, c); if (i < class_count) { class_buffer[i*2] = (uint8_t) ((i >> 8) & 0xFF); class_buffer[i*2 + 1] = (uint8_t) (i & 0xFF); @@ -1952,12 +1952,12 @@ static void test_resolve_with_basic_mark_base_pos_1_split () void* buffer = malloc (buffer_size); assert (buffer); hb_serialize_context_t c (buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 2000, 1>(&c); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 1100, 1>(&c); void* expected_buffer = malloc (buffer_size); assert (expected_buffer); hb_serialize_context_t e (expected_buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 2000, 2>(&e); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 1100, 2>(&e); run_resolve_overflow_test ("test_resolve_with_basic_mark_base_pos_1_split", c, From 683c5dd21e63fd187ddc6f9b9ae106fac0078163 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 20:57:04 +0000 Subject: [PATCH 27/29] [repacker] further reduce base count. --- src/test-repacker.cc | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index f7bb1fdf7..de221be5c 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -342,15 +342,20 @@ struct MarkBasePosBuffers { unsigned base_anchors[class_count * base_count]; unsigned mark_anchors[mark_count]; - uint64_t anchors_buffer[class_count * base_count]; + uint8_t anchor_buffers[class_count * base_count + 100]; uint8_t class_buffer[class_count * 2]; MarkBasePosBuffers(hb_serialize_context_t* c) { + for (unsigned i = 0; i < sizeof(anchor_buffers) / 2; i++) + { + uint16_t* value = (uint16_t*) (&anchor_buffers[2*i]); + *value = i; + } + for (unsigned i = 0; i < class_count * base_count; i++) { - anchors_buffer[i] = i; - base_anchors[i] = add_object ((char*) &anchors_buffer[i], 8, c); + base_anchors[i] = add_object ((char*) &anchor_buffers[i], 100, c); if (i < class_count) { class_buffer[i*2] = (uint8_t) ((i >> 8) & 0xFF); class_buffer[i*2 + 1] = (uint8_t) (i & 0xFF); @@ -359,7 +364,7 @@ struct MarkBasePosBuffers for (unsigned i = 0; i < mark_count; i++) { - mark_anchors[i] = add_object ((char*) &anchors_buffer[i], 4, c); + mark_anchors[i] = add_object ((char*) &anchor_buffers[i], 4, c); } } @@ -1952,12 +1957,12 @@ static void test_resolve_with_basic_mark_base_pos_1_split () void* buffer = malloc (buffer_size); assert (buffer); hb_serialize_context_t c (buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 1100, 1>(&c); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 110, 1>(&c); void* expected_buffer = malloc (buffer_size); assert (expected_buffer); hb_serialize_context_t e (expected_buffer, buffer_size); - populate_serializer_with_large_mark_base_pos_1 <40, 10, 1100, 2>(&e); + populate_serializer_with_large_mark_base_pos_1 <40, 10, 110, 2>(&e); run_resolve_overflow_test ("test_resolve_with_basic_mark_base_pos_1_split", c, From 015ca5bc3ce387c46c4e2f0c70e636d8c6ba76ab Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 21:52:55 +0000 Subject: [PATCH 28/29] [repacker] fix compiler alignment warning. --- src/test-repacker.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test-repacker.cc b/src/test-repacker.cc index de221be5c..cd8789f1b 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -349,7 +349,7 @@ struct MarkBasePosBuffers { for (unsigned i = 0; i < sizeof(anchor_buffers) / 2; i++) { - uint16_t* value = (uint16_t*) (&anchor_buffers[2*i]); + OT::HBUINT16* value = (OT::HBUINT16*) (&anchor_buffers[2*i]); *value = i; } From a91bfeeda53fcdf4674ff2069eb5f906f330e2de Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 18 Aug 2022 22:01:48 +0000 Subject: [PATCH 29/29] [repacker] comment cleanup. --- src/graph/markbasepos-graph.hh | 3 +-- src/hb-repacker.hh | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/graph/markbasepos-graph.hh b/src/graph/markbasepos-graph.hh index 4bbdf3ced..56fa81240 100644 --- a/src/graph/markbasepos-graph.hh +++ b/src/graph/markbasepos-graph.hh @@ -400,8 +400,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2