From fdd1952c751960392f29162298e7884d5d5ca6c4 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Thu, 4 Aug 2022 19:21:16 +0000 Subject: [PATCH] [repacker] PairPosFormat2 splitting - fix coverage and classdef splitting. The old code was splitting based on coverage index, but should have been splitting on class value. --- src/graph/classdef-graph.hh | 29 +++------ src/graph/coverage-graph.hh | 16 ++++- src/graph/pairpos-graph.hh | 121 ++++++++++++++++++++---------------- src/test-repacker.cc | 44 ++++++++----- 4 files changed, 119 insertions(+), 91 deletions(-) diff --git a/src/graph/classdef-graph.hh b/src/graph/classdef-graph.hh index c355b9732..fd6472a0d 100644 --- a/src/graph/classdef-graph.hh +++ b/src/graph/classdef-graph.hh @@ -57,35 +57,22 @@ struct ClassDefFormat2 : public OT::ClassDefFormat2_4 struct ClassDef : public OT::ClassDef { template - static bool clone_class_def (gsubgpos_graph_context_t& c, - unsigned class_def_id, - unsigned new_parent_id, - unsigned link_position, - It glyphs) + static bool add_class_def (gsubgpos_graph_context_t& c, + unsigned parent_id, + unsigned link_position, + It glyph_and_class, + unsigned max_size) { - unsigned class_def_size = c.graph.vertices_[class_def_id].table_size (); - auto& class_def_v = c.graph.vertices_[class_def_id]; - ClassDef* class_def_table = (ClassDef*) class_def_v.obj.head; - if (!class_def_table->sanitize (class_def_v)) - return false; - - auto new_class_def = - + glyphs - | hb_map_retains_sorting ([&] (hb_codepoint_t gid) { - return hb_pair (gid, class_def_table->get_class (gid)); - }) - ; - unsigned class_def_prime_id = c.graph.new_node (nullptr, nullptr); auto& class_def_prime_vertex = c.graph.vertices_[class_def_prime_id]; - if (!make_class_def (c, new_class_def, class_def_prime_id, class_def_size)) + if (!make_class_def (c, glyph_and_class, class_def_prime_id, max_size)) return false; - auto* class_def_link = c.graph.vertices_[new_parent_id].obj.real_links.push (); + auto* class_def_link = c.graph.vertices_[parent_id].obj.real_links.push (); class_def_link->width = SmallTypes::size; class_def_link->objidx = class_def_prime_id; class_def_link->position = link_position; - class_def_prime_vertex.parents.push (new_parent_id); + class_def_prime_vertex.parents.push (parent_id); return true; } diff --git a/src/graph/coverage-graph.hh b/src/graph/coverage-graph.hh index a5e5e1491..7309029c2 100644 --- a/src/graph/coverage-graph.hh +++ b/src/graph/coverage-graph.hh @@ -77,16 +77,26 @@ struct Coverage : public OT::Layout::Common::Coverage | hb_map_retains_sorting (hb_first) ; + return add_coverage (c, new_parent_id, link_position, new_coverage, coverage_size); + } + + template + static Coverage* add_coverage (gsubgpos_graph_context_t& c, + unsigned parent_id, + unsigned link_position, + It glyphs, + unsigned max_size) + { unsigned coverage_prime_id = c.graph.new_node (nullptr, nullptr); auto& coverage_prime_vertex = c.graph.vertices_[coverage_prime_id]; - if (!make_coverage (c, new_coverage, coverage_prime_id, coverage_size)) + if (!make_coverage (c, glyphs, coverage_prime_id, max_size)) return nullptr; - auto* coverage_link = c.graph.vertices_[new_parent_id].obj.real_links.push (); + auto* coverage_link = c.graph.vertices_[parent_id].obj.real_links.push (); coverage_link->width = SmallTypes::size; coverage_link->objidx = coverage_prime_id; coverage_link->position = link_position; - coverage_prime_vertex.parents.push (new_parent_id); + coverage_prime_vertex.parents.push (parent_id); return (Coverage*) coverage_prime_vertex.obj.head; } diff --git a/src/graph/pairpos-graph.hh b/src/graph/pairpos-graph.hh index 6b0e4e075..00cae4fad 100644 --- a/src/graph/pairpos-graph.hh +++ b/src/graph/pairpos-graph.hh @@ -320,6 +320,8 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4::min_size + num_records * split_context.class1_record_size; @@ -328,7 +330,7 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4format = this->format; pair_pos_prime->valueFormat1 = this->valueFormat1; pair_pos_prime->valueFormat2 = this->valueFormat2; @@ -340,35 +342,55 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4sanitize (coverage_v) + || !class_def_1_table->sanitize (class_def_1_v)) + return false; + + auto klass_map = + + coverage_table->iter () + | hb_map_retains_sorting ([&] (hb_codepoint_t gid) { + return hb_pair (gid, class_def_1_table->get_class (gid)); + }) + | hb_filter ([&] (hb_codepoint_t klass) { + return klass >= start && klass < end; + }, hb_second) + | hb_map_retains_sorting ([&] (hb_pair_t gid_and_class) { + // Classes must be from 0...N so subtract start + return hb_pair (gid_and_class.first, gid_and_class.second - start); + }) + ; + + if (!Coverage::add_coverage (split_context.c, + pair_pos_prime_id, + 2, + + klass_map | hb_map_retains_sorting (hb_first), + coverage_v.table_size ())) return -1; // classDef1 - unsigned class_def_1_id = - split_context.c.graph.index_for_offset (split_context.this_index, &classDef1); - if (!ClassDef::clone_class_def (split_context.c, - class_def_1_id, - pair_pos_prime_id, - 8, - new_coverage->iter ())) + if (!ClassDef::add_class_def (split_context.c, + pair_pos_prime_id, + 8, + + klass_map, + class_def_1_v.table_size ())) return -1; // classDef2 unsigned class_def_2_id = - split_context.c.graph.index_for_offset (split_context.this_index, &classDef2); - auto* class_def_link = split_context.c.graph.vertices_[pair_pos_prime_id].obj.real_links.push (); + graph.index_for_offset (split_context.this_index, &classDef2); + auto* class_def_link = graph.vertices_[pair_pos_prime_id].obj.real_links.push (); class_def_link->width = SmallTypes::size; class_def_link->objidx = class_def_2_id; class_def_link->position = 10; - split_context.c.graph.vertices_[class_def_2_id].parents.push (pair_pos_prime_id); - split_context.c.graph.duplicate (pair_pos_prime_id, class_def_2_id); + graph.vertices_[class_def_2_id].parents.push (pair_pos_prime_id); + graph.duplicate (pair_pos_prime_id, class_def_2_id); return pair_pos_prime_id; } @@ -450,48 +472,43 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4= old_count) return true; + graph_t& graph = split_context.c.graph; class1Count = count; - split_context.c.graph.vertices_[split_context.this_index].obj.tail -= + graph.vertices_[split_context.this_index].obj.tail -= (old_count - count) * split_context.class1_record_size; unsigned coverage_id = - split_context.c.graph.index_for_offset (split_context.this_index, &coverage); - auto& coverage_v = split_context.c.graph.vertices_[coverage_id]; + graph.index_for_offset (split_context.this_index, &coverage); + unsigned class_def_1_id = + graph.index_for_offset (split_context.this_index, &classDef1); + auto& coverage_v = graph.vertices_[coverage_id]; + auto& class_def_1_v = graph.vertices_[class_def_1_id]; Coverage* coverage_table = (Coverage*) coverage_v.obj.head; - if (!coverage_table->sanitize (coverage_v)) + ClassDef* class_def_1_table = (ClassDef*) class_def_1_v.obj.head; + if (!coverage_table->sanitize (coverage_v) + || !class_def_1_table->sanitize (class_def_1_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) - ; - - if (!Coverage::make_coverage (split_context.c, new_coverage, coverage_id, coverage_v.table_size ())) + auto klass_map = + + coverage_table->iter () + | hb_map_retains_sorting ([&] (hb_codepoint_t gid) { + return hb_pair (gid, class_def_1_table->get_class (gid)); + }) + | hb_filter ([&] (hb_codepoint_t klass) { + return klass < count; + }, hb_second) + ; + + if (!Coverage::make_coverage (split_context.c, + + klass_map | hb_map_retains_sorting (hb_first), + coverage_id, + coverage_v.table_size ())) return false; - // classDef1 - coverage_table = (Coverage*) coverage_v.obj.head; // get the new table - unsigned class_def_id = split_context.c.graph.index_for_offset (split_context.this_index, - &classDef1); - auto& class_def_v = split_context.c.graph.vertices_[class_def_id]; - ClassDef* class_def_table = (ClassDef*) class_def_v.obj.head; - if (!class_def_table->sanitize (class_def_v)) - return false; - - auto new_class_def = - + coverage_table->iter () - | hb_map_retains_sorting ([&] (hb_codepoint_t gid) { - return hb_pair (gid, class_def_table->get_class (gid)); - }) - ; - return ClassDef::make_class_def (split_context.c, - new_class_def, - class_def_id, - class_def_v.table_size ()); + + klass_map, + class_def_1_id, + class_def_1_v.table_size ()); } hb_hashmap_t diff --git a/src/test-repacker.cc b/src/test-repacker.cc index e7aee2efb..ae2a05a90 100644 --- a/src/test-repacker.cc +++ b/src/test-repacker.cc @@ -143,6 +143,7 @@ static unsigned add_extension (unsigned child, } +// Adds coverage table fro [start, end] static unsigned add_coverage (char start, char end, hb_serialize_context_t* c) { @@ -167,24 +168,29 @@ static unsigned add_coverage (char start, char end, return add_object (coverage, 10, c); } -static unsigned add_class_def (uint8_t start_glyph, - uint16_t class_count, +// Adds a class that maps glyphs from [start_glyph, end_glyph) +// to classes 1...n +static unsigned add_class_def (uint16_t start_glyph, + uint16_t end_glyph, hb_serialize_context_t* c) { + unsigned count = end_glyph - start_glyph; uint8_t header[] = { 0, 1, // format - 0, start_glyph, // startGlyphID - (uint8_t) ((class_count >> 8) & 0xFF), - (uint8_t) (class_count & 0xFF), // count + + (uint8_t) ((start_glyph >> 8) & 0xFF), + (uint8_t) (start_glyph & 0xFF), // start_glyph + + (uint8_t) ((count >> 8) & 0xFF), + (uint8_t) (count & 0xFF), // count }; start_object ((char*) header, 6, c); - for (uint16_t i = 1; i <= class_count; i++) + for (uint16_t i = 1; i <= count; i++) { - unsigned klass = start_glyph + 10 + i; uint8_t class_value[] = { - (uint8_t) ((klass >> 8) & 0xFF), - (uint8_t) (klass & 0xFF), // count + (uint8_t) ((i >> 8) & 0xFF), + (uint8_t) (i & 0xFF), // count }; extend ((char*) class_value, 2, c); } @@ -1216,19 +1222,27 @@ populate_serializer_with_large_pair_pos_2 (hb_serialize_context_t* c, unsigned* device_tables = (unsigned*) calloc (num_pair_pos_2 * num_class_1 * num_class_2, sizeof(unsigned)); + // Total glyphs = num_class_1 * num_pair_pos_2 for (int i = num_pair_pos_2 - 1; i >= 0; i--) { + unsigned start_glyph = 5 + i * num_class_1; if (num_class_2 >= num_class_1) { - class_def_2[i] = add_class_def (10, num_class_2, c); - class_def_1[i] = add_class_def (5 + i * num_class_1, num_class_1, c); + class_def_2[i] = add_class_def (11, + 10 + num_class_2, c); + class_def_1[i] = add_class_def (start_glyph + 1, + start_glyph + num_class_1, + c); } else { - class_def_1[i] = add_class_def (5 + i * num_class_1, num_class_1, c); - class_def_2[i] = add_class_def (10, num_class_2, c); + class_def_1[i] = add_class_def (start_glyph + 1, + start_glyph + num_class_1, + c); + class_def_2[i] = add_class_def (11, + 10 + num_class_2, c); } - coverage[i] = add_coverage (5 + i * num_class_1, - 5 + (i + 1) * num_class_1 - 1, + coverage[i] = add_coverage (start_glyph, + start_glyph + num_class_1 - 1, c); if (with_device_tables)