Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[subset] Add more variable font subsetting tests #2912

Merged
merged 4 commits into from
Apr 1, 2021
Merged

[subset] Add more variable font subsetting tests #2912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b57dd1a
Add full variable font subsetting test.
garretrieger Mar 25, 2021
e1effb2
[subset] don't keep FeatureVariationRecord's with no subsittutions.
garretrieger Mar 25, 2021
f2a6214
[subset] don't drop features referenced in a feature variation substi…
garretrieger Mar 25, 2021
cfc5337
[subset] Match FeatureVariationRecord dropping from fontTools.
garretrieger Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions src/hb-ot-layout-common.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,7 @@ struct Feature
auto *out = c->serializer->start_embed (*this);
if (unlikely (!out || !c->serializer->extend_min (out))) return_trace (false);

bool subset_featureParams = out->featureParams.serialize_subset (c, featureParams, this, tag);
out->featureParams.serialize_subset (c, featureParams, this, tag);

auto it =
+ hb_iter (lookupIndex)
Expand All @@ -1117,8 +1117,9 @@ struct Feature
;

out->lookupIndex.serialize (c->serializer, l, it);
return_trace (bool (it) || subset_featureParams
|| (tag && *tag == HB_TAG ('p', 'r', 'e', 'f')));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to this pref case? There was a reason we added that. Breaks shaping with certain scripts (Khmer?) I think. Probably fonttools doesn't have it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into it. If we want this then we'll need to check for it during feature collection since if we drop a feature here that the feature collection was planning to keep the remapped feature indices get out of sync with the actual list of subsetted features.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want this then we'll need to check for it during feature collection

Right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh misread that, this always keeps "pref" which fontTools does too: https://github.com/fonttools/fonttools/blob/main/Lib/fontTools/subset/__init__.py#L1280. I'll check the feature collection code to make sure it's doing the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in a separate PR, this change won't make the current behaviour any worse since this method now always returns true.

// The decision to keep or drop this feature is already made before we get here
// so always retain it.
return_trace (true);
}

bool sanitize (hb_sanitize_context_t *c,
Expand Down Expand Up @@ -2965,7 +2966,8 @@ struct ConditionSet
+ conditions.iter ()
| hb_apply (subset_offset_array (c, out->conditions, this))
;
return_trace (true);

return_trace (bool (out->conditions));
}

bool sanitize (hb_sanitize_context_t *c) const
Expand Down Expand Up @@ -3000,6 +3002,12 @@ struct FeatureTableSubstitutionRecord
bool subset (hb_subset_layout_context_t *c, const void *base) const
{
TRACE_SUBSET (this);
if (!c->feature_index_map->has (featureIndex)) {
// Feature that is being substituted is not being retained, so we don't
// need this.
return_trace (false);
}

auto *out = c->subset_context->serializer->embed (this);
if (unlikely (!out)) return_trace (false);

Expand Down Expand Up @@ -3052,6 +3060,15 @@ struct FeatureTableSubstitution
record.closure_features (this, lookup_indexes, feature_indexes);
}

bool intersects_features (const hb_map_t *feature_index_map) const
{
for (const FeatureTableSubstitutionRecord& record : substitutions)
{
if (feature_index_map->has (record.featureIndex)) return true;
}
return false;
}

bool subset (hb_subset_context_t *c,
hb_subset_layout_context_t *l) const
{
Expand All @@ -3065,7 +3082,8 @@ struct FeatureTableSubstitution
+ substitutions.iter ()
| hb_apply (subset_record_array (l, &(out->substitutions), this))
;
return_trace (true);

return_trace (bool (out->substitutions));
}

bool sanitize (hb_sanitize_context_t *c) const
Expand Down Expand Up @@ -3102,6 +3120,11 @@ struct FeatureVariationRecord
(base+substitutions).closure_features (lookup_indexes, feature_indexes);
}

bool intersects_features (const void *base, const hb_map_t *feature_index_map) const
{
return (base+substitutions).intersects_features (feature_index_map);
}

bool subset (hb_subset_layout_context_t *c, const void *base) const
{
TRACE_SUBSET (this);
Expand Down Expand Up @@ -3188,9 +3211,18 @@ struct FeatureVariations
out->version.major = version.major;
out->version.minor = version.minor;

+ varRecords.iter ()
| hb_apply (subset_record_array (l, &(out->varRecords), this))
;
int keep_up_to = -1;
for (int i = varRecords.len - 1; i >= 0; i--) {
if (varRecords[i].intersects_features (this, l->feature_index_map)) {
keep_up_to = i;
break;
}
}

unsigned count = (unsigned) (keep_up_to + 1);
for (unsigned i = 0; i < count; i++) {
subset_record_array (l, &(out->varRecords), this) (varRecords[i]);
}
garretrieger marked this conversation as resolved.
Show resolved Hide resolved
return_trace (bool (out->varRecords));
}

Expand Down
1 change: 1 addition & 0 deletions test/subset/data/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ EXTRA_DIST += \
expected/sbix \
expected/colr \
expected/cbdt \
expected/variable \
fonts \
profiles \
$(NULL)
Expand Down
1 change: 1 addition & 0 deletions test/subset/data/Makefile.sources
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ TESTS = \
tests/layout.notonastaliqurdu.tests \
tests/layout.tests \
tests/sbix.tests \
tests/variable.tests \
$(NULL)

XFAIL_TESTS = \
Expand Down
Binary file added test/subset/data/expected/variable/Fraunces.keep-layout.26,66,69,124,125.ttf
View file
Open in desktop
Binary file not shown.
Binary file added test/subset/data/expected/variable/Fraunces.keep-layout.61.ttf
View file
Open in desktop
Binary file not shown.
Binary file added test/subset/data/fonts/Fraunces.ttf
View file
Open in desktop
Binary file not shown.
9 changes: 9 additions & 0 deletions test/subset/data/tests/variable.tests
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FONTS:
Fraunces.ttf

PROFILES:
keep-layout.txt

SUBSETS:
a
&fiĤĥ
1 change: 1 addition & 0 deletions test/subset/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ tests = [
'sbix',
'colr',
'cbdt',
'variable',
]

repack_tests = [
Expand Down