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.

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

Processing section mostly rewritten #66

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Processing section mostly rewritten #66

wants to merge 2 commits into from

Conversation

Lorp
Copy link
Collaborator

@Lorp Lorp commented Aug 18, 2022

Also significant changes to:

  • the axisIndexMap partof the Format section
  • the recommendations on how to build ItemVariationStore efficiently
    Other minor changes.

@Lorp Lorp self-assigned this Aug 18, 2022
@Lorp Lorp requested a review from behdad August 18, 2022 17:37
@behdad
Copy link
Member

behdad commented Aug 18, 2022

I think this is stepping into the internals of ItemVariationStore again. I like separation of concerns.

@@ -30,27 +30,36 @@ Use cases include:

The table format for `avar` version 2 is the same as `avar` version 1, appended with offsets to two extra structures, `axisIndexMap` and `varStore`, and with the data for those structures.

`axisIndexMap` is a *DeltaSetIndexMap* structure that maps the axis indices implied in `fvar` to indices used in `varStore`. The outer index identifies an *ItemVariationData* structure in `varStore`. The inner index identifies a *deltaSet* within an *ItemVariationData*.
`axisIndexMap` is a *DeltaSetIndexMap* structure that maps the axis indices implied in `fvar` to indices used in `varStore`. In the mapping, the index of each mapping entry is the axis index; the outer index identifies an *ItemVariationData* structure in `varStore`; the inner index identifies a *deltaSet* within an *ItemVariationData*. If either outer or inner index is 0xffff, then that axis’s value is ignored by the `avar` version 2 process. The number of mapping entries may be smaller than the number of axes in 'fvar', in which case remaining axes are ignored.
Copy link
Member

Choose a reason for hiding this comment

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

If either outer or inner index is 0xffff, then that axis’s value is ignored by the avar version 2 process.

The OpenType spec only says the pair 0xFFFF,0xFFFF means "no variation".

The number of mapping entries may be smaller than the number of axes in 'fvar', in which case remaining axes are ignored.

OpenType spec says any missing values at the end get the value of the last entry. Again, this is details specified in DeltaSetIndexMap structure already.

Copy link
Member

Choose a reason for hiding this comment

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

And because of how DeltaSetIndexMap compression works, it is more efficient to encode an entry in ItemVariationStore for "no variation" and use that, instead of using 0xFFFF,0xFFFF, which would force DeltaSetIndexMap entries to 4bytes per entry.

Then again, these are compiler-optimization issues, which IMO should NOT be part of the spec. Definitely not here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah indeed it does. (I was looking for such an explanation in the docs for DeltaSetIndexMap when in fact it applies whatever method is used to find outer and inner.)

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 saw in your RobotoFlex out.ttf demo font you had used 6 ItemVariationDatas. The reason to do that is to avoid proliferation of zeroes in the deltaSets, isn’t it?

Copy link
Member

Choose a reason for hiding this comment

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

Not just zeroes. Also to choose the optimal encoding using one-byte vs two-byte columns. We have an optimizer that takes care of 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.

Got it

* The inner value identifies a *deltaSet* within the *ItemVariationData*.
* The *ItemVariationData* contains an array of *variationRegion* indices, implying an array of indices to scalars.
* Multiply each scalar by the delta value at the same index, yielding an array of products. (We can do this since the array of region indices has the same length as the *deltaSet*.)
* Take the sum of those products to obtain the interpolated delta value.
Copy link
Member

Choose a reason for hiding this comment

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

The substeps up to here are internals of ItemVariationStore and should not be explained. Really the algorithm for this table is 3 lines and I prefer to keep it that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My issue is probably with the lack of a good algorithm description in the spec for ItemVariationStore.

In order to reduce proliferation of zero deltas, it is recommended to store in `axisIndexMap` only those axes that are remapped by `avar` version 2. For the same reason, if there are multiple different types of axis mapping affecting different sets of axes, consider using multiple `ItemVariationData` structures.
For axes that do not have `avar` version 2 adjustments, either use the special value 0xffff for both outer and inner indices in `axisIndexMap` or reorder the axes such that the non-participating axes are at the end and `axisIndexMap` can have fewer entries.

To reduce proliferation of zero deltas, consider grouping axes in terms of the axes they depend upon. Each group can be represented by its own *ItemVariationData* structure, thus using smaller *deltaSet*s that do not refer to irrelevant axes.
Copy link
Member

Choose a reason for hiding this comment

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

This is irrelevant. ItemVariationStore can reorder axes. The order of axes has no optimization value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to have these axes excluded on the basis of axisCount > mapCount. There is a tiny benefit of implying multiple 0xffff/0xffff entries this way at the end of the deltaSetIndexMap, but not worth mentioning.

@behdad
Copy link
Member

behdad commented Aug 18, 2022

I suggest you keep optimization ideas out of the spec. They don't belong here.

@Lorp
Copy link
Collaborator Author

Lorp commented Aug 18, 2022

In principle I fully understand your wish for separation of concerns. I am labouring the points because I think the idea of using axis values to influences axis values is potentially confusing to implementers. Despite a spec being logically complete, people still have to read it and write code based on it :)

I think you’re right to imply that internals of the Variation Algo should not be regurgitated here — that can be regarded as a black box that is already understood (or relied upon without understanding). Maybe the clarifying effort should be on how how the Variation Algo is used TWICE for any instance: first to vary axis values, second (in the good old way) to vary outline points and other values.

* The set of axes involved in adjusting the coordinate for a given axis may include the axis itself.

* It is expected that implementations that handle only `avar` version 1 will ignore the entire table by rejecting the `majorVersion` value.
* It is expected that implementations that handle only `avar` version 1 will ignore the entire table by rejecting the *majorVersion* value.
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 need to test this. It doesn’t make sense to recommend behaviour for existing code.

Copy link
Member

Choose a reason for hiding this comment

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

It's what the spec says implementations should do with majorVersion. If they don't do it's their fault.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you prefer to drop the line or to leave it in, to remind implementors what they are supposed to do?

Copy link
Member

Choose a reason for hiding this comment

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

I like keeping it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants