Build CFF2 Variable Font from sparse sources, and with more than one VarData table. #1547

readroberts · 2019-03-18T17:13:14Z

in varLib/cffLib.py, add support for sparse sources, and sources with more than one model, and hence more than one VarData element in the VarStore.

Support blend ops in cffLib/specializer.py generalizeCommands() and specializeCommands().

readroberts · 2019-03-18T18:18:03Z

Note: this is a PR for a revision of an earlier branch. See #1475

…sindex. Fixes endless compile loop in some circumstances. Fixed bug in mutator: need to remove vsindex from snapshotted charstrings, plus formatting clean up

…--retain-gid option is used. Needed to make subset_test.py::test_retain_gids_cff2 tests pass.

… more than one model, and hence more than one VarData element in the VarStore. CFF2 source fonts with multiple FontDicts in the FDArray need some extra work. With sparse fonts, some of the source fonts may have a fewer FontDicts than the default font. The getfd_map function() builds a map from the FontDict indices in the default font to those in each region font. This is needed when building up the blend value lists in the master font FontDict PrivateDicts, in order to fetch PrivateDict values from the correct FontDict in each region font. In specializer.py, add support for CFF2 CharStrings with blend operators. 1) In generalizeCommands, convert a blend op to a list of args that are blend lists for the following regular operator. A blend list as a default font value, followed by the delta tuple. 2) In specializeCommands(), convert these back to blend ops, combining as many successive blend lists as allowed by the stack limit. Add test case for sparse CFF2 sources. The test font has 55 glyphs. 2 glyphs use only 2 sources (weight = 0 and 100). The rest use 4 source fonts: the two end points of the weight axis, and two intermediate masters. The intermediate masters are only 1 design space unit apart, and are used to change glyph design at the point in design space. For the rest, at most 2 glyphs use the same set of source fonts. There are 12 source fonts. Add test case for specializer programToCommands() and commandsToProgram by converting each CharString.program in the font to a command list, and back again, and comparing original and final versions.

anthrotype · 2019-03-19T16:31:50Z

I cherry-picked the HVAR commit into master since I am about about to cut a new release, and don't feel ready to merge this one just yet (would like to take another look and also give @behdad another chance to review).
6355376

readroberts · 2019-03-19T21:49:59Z

Of course, there is certainly enough rework to need a new review. Let's keep the momentum going!

…mmandsToProgram(), mask arg must be appended following the operator.

behdad

Okay. Thanks for this and sorry for delay. Looks a lot better.

Based on the overall changes, I like to propose this new place to apply the changes:

In programToCommands, first thing before any other processing, convert each blend to ONE tuple. This means, each tuple might represent multiple values at this point,
In generalizeCommands, first before anything else, break such tuples into smaller ones, each representing one value. Move the width-extraction code from programToCommands here. Since we processed blends already, that code will work without modification.
Move combining of blend lists into longer blend lists to the end phase of specializeCommands, with stack size tracking and all.
Convert blend lists to operations in commandsToProgram.

Make sure we support recursive blends. The stack-size calculations get a bit complicated but not hugely so. Use recursive functions for many processes (like adding two values).

behdad · 2019-04-09T21:50:13Z

Lib/fontTools/cffLib/specializer.py

@@ -4,6 +4,7 @@

 from __future__ import print_function, division, absolute_import
 from fontTools.misc.py23 import *
+from fontTools.cffLib import maxStackLimit


I still think we should reuse the stack limit passed to the functions. Currently that one defaults to 48. I don't know how to accommodate what you want (default to CFF2's for blends).

My concern here is that the default value for the specializeCommands() maxstack argument is reasonable for CFF fonts, but not for CFF2 fonts with a blend operator. How about a check to set maxstack to cffLib.maxStackLimit if a) it is the default value of 48, and numRegions is not None?

Yes I like that. If numRegions is not None, then assume CFF2.

behdad · 2019-04-09T23:59:03Z

Lib/fontTools/cffLib/specializer.py

+					if isinstance(arg1, list):
+						new_args = [[a1 + a2 for a1, a2 in zip(arg0, arg1)]]
+					else:
+						new_args = [[a1 + arg1 for a1 in arg0]]


This is wrong. You should just add arg1 to the first entry, not all. No?

behdad · 2019-04-09T23:59:28Z

Lib/fontTools/cffLib/specializer.py

+					if isinstance(arg1, list):
+						new_args = [[arg0 + a1 for a1 in arg1]]
+					else:
+						new_args = [arg0 + arg1]


I suggest the above block should be moved to a function (and handle symmetry by calling itself with args reversed.)

behdad · 2019-04-10T00:00:31Z

Lib/fontTools/cffLib/specializer.py

+		else:
+			program.extend(args)
+			if op:
+				program.append(op)


Do you really need this change? The code handles hintmask/cntrmask just fine.

behdad · 2019-04-10T00:00:58Z

Lib/fontTools/cffLib/specializer.py

+			'hmoveto', 'vmoveto', 'rmoveto',
+			'endchar'}:
+			# We skip this when seen_blend == True because a blend operator
+			# can leave an odd number of arguments on the stack.


I know that we only support blend in CFF2, and CFF2 doesn't have width. But I prefer if you implement this to work with both.

I'll sketch in my overall review how that can be done instead.

behdad · 2019-04-10T00:02:31Z

Lib/fontTools/cffLib/specializer.py

+		blend_args.append(blendList)
+		tuplei = next_ti
+		argi += 1
+	return blend_args


This function should handle errors. Ie. if there's insufficient arguments, just encode them to roundtrip/ignore like other code in generalizeCommands does.

behdad · 2019-04-10T00:05:45Z

Lib/fontTools/cffLib/specializer.py

+		blendList = [op_args[argi]] + op_args[tuplei:next_ti]
+		blend_args.append(blendList)
+		tuplei = next_ti
+		argi += 1


Is this block correct? I thought there's all default values first, then all deltas for first region, then all deltas for second region, etc. No?

At any rate, I think this while loop should be converted to something using itertools or otherwise for loops. There's nothing "while" about this.

Is this block correct? I thought there's all default values first, then all deltas for first region, then all deltas for second region, etc. No?

Apparently no.

behdad · 2019-04-10T00:07:22Z

Lib/fontTools/cffLib/specializer.py

+			assert numRegions is not None, (
+				"Cannot process charstring without numRegions argument")
+			blendArgs = _convertBlendOpToArgs(args, numRegions)
+			continue


This block will be unnecessary in the order I like things to be done, as sketched in main comment.

behdad · 2019-04-10T00:07:57Z

Lib/fontTools/cffLib/specializer.py

+	for arg in args:
+		if isinstance(arg, list):
+			return True
+	return False


This function can be simply inlined as any(isinstance(arg, list) for arg in args).

behdad · 2019-04-10T12:35:29Z

Lib/fontTools/cffLib/specializer.py

+			blend_args = []
+			stack_use = prev_stack_use + num_blends
+
+	return blend_cmds, blend_args


I know spec doesn't allow it, but we should support blends where the args are also blended themselves. Or at least be able to pass those through without error.

…'s comments in PR 1547 on April 10, 2019. Fix some bugs in handling hinting.

behdad

Haven't done a full review. But a few points that need fixing already.

Also, it occurred to me: we are passing down numRegions. Whereas the program might have a vsindex operator. Shouldn't we pass a map/function that when passed the vsindex, returns numRegion?

behdad · 2019-04-17T13:46:48Z

Lib/fontTools/cffLib/specializer.py

@@ -26,13 +27,15 @@ def programToString(program):
 	return ' '.join(str(x) for x in program)


-def programToCommands(program):
+def programToCommands(program, numRegions=None, **kwargs):


Why all the kwargs args?

The extra args are there because of specializeProgram(program, **kwargs) and generalizeProgram(program, **kwargs). **kwargs needs to hold both numRegions and generalizeFirst. If these args are both in **kwargs, then programToCommnd and specializeCommand complain about the unused arg unless you provide the additional **kwargs to swallow the unused arg. I don't really like what I did, but it is better than the alternatives I thought of.

Why not just spell out the arguments that are needed?

Because I saw that the prior code used **kwargs rather than the arguments actually needed. I assumed this was a deliberate choice. I do see that it allows for adding future new arguments to the called functions, without having to change specializeProgram() and generalizeProgram(). That said, I am perfectly happy to follow your suggestion. I would then change **kwargs in specializeProgram() and generalizeProgram to the two args, and pass only the needed args to the callees. Just confirm that this is what you would prefer.

I see. That definitely explains your choice. Thank you.

Generally I like using kwargs for methods that just pass along arguments, but not in methods that consume them. What I like to see here, I think, is to keep things as they are, just add numRegion anywhere it's needed. You can add numRegions=None just before **kwargs in the methods that take that. This will separate that one argument from the rest, then you decide which methods you pass the numRegions to and which one the **kwargs. I think that works but up to you whichever works. Thanks.

behdad · 2019-04-18T22:19:22Z

Lib/fontTools/cffLib/specializer.py

+				elif isinstance(arg1, list):
+					new_args = _combineLineArgs(arg1, arg0)
+				else:
+					new_args = [arg0 + arg1]


This block should simply become `new_args = [_addArgs(args[0], other_args[0])]

behdad · 2019-04-18T22:22:58Z

Lib/fontTools/cffLib/specializer.py

+def _combineLineArgs(listArg, valArg):
+	listArg[0] += valArg
+	newArgList = listArg
+	return newArgList


This function seems incomplete. The whole if hierarchy below should come here, and it should recurse somethings. This should become:

def _addArgs(a, b): if isinstance(b, list): if isinstance(a, list): return [_addArgs(va, vb) for va,vb in zip(a, b)] else: a, b = b, a if isinstance(a, list): return [_addArgs(a[0], b)] + a[1:]] return a + b

Yep, that is better.

@behdad Just pushed the changes above.

simplify the logic to combine arguments for successive single-argument hlineto or vlineto operators For generalizeProgram() and specializeProgram(), pass the numRegions arguments explicitly rather than in a **kwargs argument.

readroberts · 2019-04-19T18:40:58Z

@behdad About your question: "Shouldn't we pass a map/function that when passed the vsindex, returns numRegion?". I think this will add complexity to the logic. 'vsindex' may not ( and indeed is usually not) present in the program, so you would have to pass in both the mapping function and the default vsindex, which may be set only in the private dict, or not at all. You can see how numRegions is derived at line 945 in specializer_test.py, and line varLib/cff.py: whether or not there is a 'vsindex' in the charstring is already handled by the T2Charstring.vsindex property.

behdad · 2019-04-19T21:04:58Z

@behdad About your question: "Shouldn't we pass a map/function that when passed the vsindex, returns numRegion?". I think this will add complexity to the logic. 'vsindex' may not ( and indeed is usually not) present in the program, so you would have to pass in both the mapping function and the default vsindex, which may be set only in the private dict, or not at all. You can see how numRegions is derived at line 945 in specializer_test.py, and line varLib/cff.py: whether or not there is a 'vsindex' in the charstring is already handled by the T2Charstring.vsindex property.

I still think the specializer should do the right thing / be generic.

I suggest this: take numRegions as is. When we need to use it:

if it's None, err,
if it's an integer, use it,
otherwise call it with one argument. The argument is the last vsindex seen so far (I know spec says vsindex can only occur once and at the beginning, but there's no reason NOT to write the code more generally here). If we have not seen vsindex so far, pass None as parameter. Use the return value.

I suggest you do this in specializer and document it in the docstring. Whether you use it in varLib/cff is another issue and up to you.

Thanks. Or feel free to ignore, and I do it after you land.

readroberts · 2019-04-22T16:44:01Z

@behdad That creates an extra problem case: what if the numRegions passed in as an integer, and the program contains a vsindex operator? How about a variation on your original suggestion: the numRegions arg is always None or a function. If the function is not passed an arg, it returns the default numRegions for the charstring, else it takes vsindex as an argument, and returns the numRegion implied by the vsindex.

behdad · 2019-04-22T16:46:35Z

@behdad That creates an extra problem case: what if the numRegions passed in as an integer, and the program contains a vsindex operator? How about a variation on your original suggestion: the numRegions arg is always None or a function. If the function is not passed an arg, it returns the default numRegions for the charstring, else it takes vsindex as an argument, and returns the numRegion implied by the vsindex.

Sure, that's what I originally had in mind. I thought about not passing an argument, vs passing None. The latter is easier since you can initialize a variable to None, and update it if you see vsindex op, and pass that variable to function, as opposed to conditionalize the call to the function.

readroberts · 2019-04-22T16:59:50Z

Sounds good. I propose adding a numRegions property in psCharstrings.py::T2CharString. This not only provides the needed function, but can reset the T2CharString current vsindex value whenever T2CharString.numRegions() is called with a not None value. I think I will need to update T2CharString anyway - it now assumes that any vsindex op occurs only at the start of the charstring.

behdad · 2019-04-22T17:05:11Z

Sounds good. I propose adding a numRegions property in psCharstrings.py::T2CharString. This not only provides the needed function, but can reset the T2CharString current vsindex value whenever T2CharString.numRegions() is called with a not None value. I think I will need to update T2CharString anyway - it now assumes that any vsindex op occurs only at the start of the charstring.

I'm not sure how that works, but sure, if you think so.

- programToCommands now takes a function argument, numRegions - 'vsindex' is now allowed to occur more than once in the charstring Since vsindex may now occur more than once in a charstring, changed misc/psCharString.py::T2Charstring accordingly: - removed vsindex property, since this is no longer a static item, and now depends on current location in the charstring - add a numRegions function to get the num regions in use according to the current charstring vsindex. Updated specializer_test.py and varLib/mutator.py to match

readroberts · 2019-04-23T18:06:50Z

@behdad Updated specializer with change in handling of numRegions and vsindex; please review. Other files updated to match -see commit message.

behdad

Thanks Read. Looks great!

behdad · 2019-04-24T02:57:02Z

Lib/fontTools/cffLib/specializer.py

+	program (¯\_(ツ)_/¯).
+	'numRegions' may be None, or a function that returns the number
+	of regions. If the function is not passed a vsindex argument, it returns
+	the default number of regions for the charstring, else it returns the


If the function is not passed a vsindex argument is inaccurate. We pass a None to it in that case.

Also, instead of it returns, use it must return?

behdad · 2019-04-24T02:58:23Z

Lib/fontTools/cffLib/specializer.py

 			# replace the blend op args on the stack with a single list
 			# containing all the blend op args.
-			numBlendOps = stack[-1]*(numRegions+1) + 1
+			numBlendOps = stack[-1] * numSourceFonts + 1
+			# replace first blend op by a list of the blend ops.
 			stack[-numBlendOps] = stack[-numBlendOps:]
 			del stack[-numBlendOps + 1:]


I think the above two lines can be written as:
stack[-numBlendOps:] = [stack[-numBlendOps:]]

behdad · 2019-04-24T02:58:57Z

Lib/fontTools/cffLib/specializer.py

-		elif (not seen_width_op) and token in {'hstem', 'hstemhm', 'vstem', 'vstemhm',
+
+		elif token == 'vsindex':
+			vsIndex = stack[-1]


Should we assert it's integer, or doesn't matter?

behdad · 2019-04-24T03:01:42Z

Lib/fontTools/misc/psCharStrings.py

@@ -944,6 +944,16 @@ def __init__(self, bytecode=None, program=None, private=None, globalSubrs=None):
 		self.program = program
 		self.private = private
 		self.globalSubrs = globalSubrs if globalSubrs is not None else []
+		self._cur_vsindex = None
+
+	def numRegions(self, vsindex=None):


Should this be renamed getNumRegions?

behdad · 2019-04-24T03:02:49Z

Lib/fontTools/cffLib/specializer.py

 	"""Takes a T2CharString program list and returns list of commands.
 	Each command is a two-tuple of commandname,arg-list.  The commandname might
 	be empty string if no commandname shall be emitted (used for glyph width,
 	hintmask/cntrmask argument, as well as stray arguments at the end of the
-	program (¯\_(ツ)_/¯)."""
+	program (¯\_(ツ)_/¯).
+	'numRegions' may be None, or a function that returns the number


Instead of function say callable?

behdad · 2019-04-24T03:03:38Z

Lib/fontTools/cffLib/specializer.py

+def _addArgs(a, b):
+	if isinstance(b, list):
+		if isinstance(a, list):
+			return [_addArgs(va, vb) for va,vb in zip(a, b)]


Should we check that the lengths match?

@behdad Sure. This can't happen if the command list is build by programToCommands, but a developer could build one independently and do it incorrectly. I would raise an error if the lengths don't match, catch it at line 585 and continue without changing the command. Sound OK?

Yes. Thanks.

In specializer.py:programToCommands(): - edit comments about getNumRegions arg - at line 58, use more compact stack array editing syntax - assert that vsindex arg is an int at line 76 In specializer.py:specializeCommands(): - When combining successive [vh]lineto's, assert that when both args are lists, that they are the same length, and continue if not. In fontTools/misc/psCharStrings.py::T2CharString: rename numRegions method to getNumRegions. This is parallel to the same function name in PrivateDict(), and avoids confusion with the self.numRegions field in SimpleT2Decompiler(). Applied same name change to argument for specializer.py:programToCommands().

readroberts · 2019-04-24T18:42:56Z

@behdad Thanks, the changes you suggested yesterday were all useful. Please take a look at the latest commit, which I think implements all of them

behdad

Thanks Read. Looks great.

readroberts · 2019-04-26T16:30:38Z

Change submitted in #1591: built new branch and PR to allow clean commit

readroberts requested review from behdad and anthrotype March 18, 2019 18:45

readroberts added 3 commits March 19, 2019 09:09

Added getter (in the form of a property decorator) for T2Charstring.v…

25c589a

…sindex. Fixes endless compile loop in some circumstances. Fixed bug in mutator: need to remove vsindex from snapshotted charstrings, plus formatting clean up

Fix for subsetting HVAR tables that have an AdvanceWidthMap when the …

6800b3f

…--retain-gid option is used. Needed to make subset_test.py::test_retain_gids_cff2 tests pass.

readroberts force-pushed the cff2vf-sparse-sources branch from a73caf3 to 4ab1c07 Compare March 19, 2019 16:18

anthrotype assigned behdad Mar 25, 2019

readroberts force-pushed the cff2vf-sparse-sources branch from f667b2e to dc506bd Compare April 3, 2019 00:21

Bug fix for supporting hint operators: in cffLib.py specialize.py::co…

ce472a1

…mmandsToProgram(), mask arg must be appended following the operator.

readroberts force-pushed the cff2vf-sparse-sources branch from dc506bd to ce472a1 Compare April 3, 2019 01:00

behdad requested changes Apr 10, 2019

View reviewed changes

Re-organize handling of blend operators in specializer.py, per Behdad…

2d0e29c

…'s comments in PR 1547 on April 10, 2019. Fix some bugs in handling hinting.

behdad reviewed Apr 18, 2019

View reviewed changes

behdad approved these changes Apr 24, 2019

View reviewed changes

behdad reviewed Apr 24, 2019

View reviewed changes

behdad approved these changes Apr 25, 2019

View reviewed changes

readroberts mentioned this pull request Apr 26, 2019

Sparse cff2vf support #1591

Merged

readroberts closed this Apr 26, 2019

readroberts deleted the cff2vf-sparse-sources branch April 26, 2019 16:30

Build CFF2 Variable Font from sparse sources, and with more than one VarData table. #1547

Build CFF2 Variable Font from sparse sources, and with more than one VarData table. #1547

Conversation

readroberts commented Mar 18, 2019

readroberts commented Mar 18, 2019

anthrotype commented Mar 19, 2019 • edited Loading

readroberts commented Mar 19, 2019

behdad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

readroberts commented Apr 19, 2019

behdad commented Apr 19, 2019

readroberts commented Apr 22, 2019

behdad commented Apr 22, 2019

readroberts commented Apr 22, 2019

behdad commented Apr 22, 2019

readroberts commented Apr 23, 2019

behdad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

readroberts commented Apr 24, 2019

behdad left a comment

Choose a reason for hiding this comment

readroberts commented Apr 26, 2019

anthrotype commented Mar 19, 2019 •

edited

Loading

behdad Apr 18, 2019 •

edited

Loading