[subset] FeatureVariations subsetting is wrong

[anthrotype]

originally reported by @behdad at harfbuzz/harfbuzz#2334 (comment)

We cannot drop FeatureVariationsRecords that have empty substitutions. That changes the behavior. A FeatureVariationsRecords stops the search when the conditions match; so even if there's no susbstitutions we cannot drop the record as that will keep the search going.
The only optimizations we can do are 1. dropping from the end of the array any records with no susbtitutions, and 2. dropping records earlier with no susbstitution only after analysing the conditions and proving that the dropping does not change functionality.
Both of those are unnecessary right now. We should fix the functionality.
This seems to be wrong in FontTools as well:
https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/subset/__init__.py#L1325

[behdad]

I'll try submitting a minimal fix and a simple pruning but won't be able to contribute tests. I hope there are tests for those code path already.

[anthrotype]

thanks Behdad, i'll fix up the tests as needed

[anthrotype]

To test this dropping-from-end-of-array of FeatureVariationRecords with no substitutions, we need create a VF where FeatureTableSubstitutions contain more than one SubstitutionRecords referencing different FeatureIndex; then we'd drop one feature while keeping another and assert that the thus emptied FeatureVariationRecords at the end of the FeatureVariations are dropped, and that empty records that precede non-empty ones are kept.
The current featureVars API can only create FeatureVariations for a one feature at a time (e.g. rvrn). Making a test font manually is not fun.. I will try again when I get the chance. I'll keep this open in the meantime as reminder.

[anthrotype]

@behdad when the subsetter has dropped all the existing FeatureVariationRecords, should it not also remove the emptied FeatureVariations table and downgrade the GSUB/GPOS table version to 0x00010000 (inside prune_post_subset)?

[behdad]

@behdad when the subsetter has dropped all the existing FeatureVariationRecords, should it not also remove the emptied FeatureVariations table and downgrade the GSUB/GPOS table version to 0x00010000 (inside prune_post_subset)?

Yes. I thought that's implemented.

[behdad]

The current featureVars API can only create FeatureVariations for a one feature at a time (e.g. rvrn). Making a test font manually is not fun.. I will try again when I get the chance. I'll keep this open in the meantime as reminder.

Right. You will hit same situ in partial instancing as well. I don't know how to share such pruning code....

[anthrotype]

I thought that's implemented.

if you mean this:

fonttools/Lib/fontTools/subset/__init__.py

Lines 1637 to 1639 in 9279f93

	if hasattr(table, 'FeatureVariations') and not table.FeatureVariations:
	if table.Version == 0x00010001:
	table.Version = 0x00010000

then nobody seems to be setting table.FeatureVariations = None when FeatureVariationRecords list is empty

[behdad]

then nobody seems to be setting table.FeatureVariations = None when FeatureVariationRecords list is empty

Right... That's normally done when subset() return false. But clearly not in this case..

[anthrotype]

@behdad something like this #1903

[anthrotype]

You will hit same situ in partial instancing as well. I don't know how to share such pruning code....

I didn't quite get what you meant here.

[behdad]

You will hit same situ in partial instancing as well. I don't know how to share such pruning code....

I didn't quite get what you meant here

I mean, I assume you implemented FeatureVariations updating in your partial-instancer code. That code can also do some pruning and possibly removing FeatureVariations if empty.

[anthrotype]

ah yes, we do remove FeatureVariations when left empty in the instancer.

[behdad]

Fixed in #1903

[behdad]

Humm. Not sure.

[behdad]

Was fixed in cab7d13