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

Compress type 1 GPOS tables better #1539

Merged
merged 3 commits into from Apr 15, 2019
Merged

Compress type 1 GPOS tables better #1539

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06ed699
Compress type 1 GPOS tables better
mhosken Mar 15, 2019
27d8d1b
Update tests
mhosken Mar 15, 2019
1a92b3c
Comment changes to help clarify
mhosken Apr 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions Lib/fontTools/otlLib/builder.py
View file
Open in desktop
Expand Up @@ -408,7 +408,7 @@ def buildSinglePos(mapping, glyphMap):
# If a ValueRecord is shared between multiple glyphs, we generate
# a SinglePos format 1 subtable; that is the most compact form.
for key, glyphs in coverages.items():
if len(glyphs) > 1:
if len(glyphs) * _getSinglePosValueSize(key) > 5:
anthrotype marked this conversation as resolved.
Show resolved Hide resolved
format1Mapping = {g: values[key] for g in glyphs}
result.append(buildSinglePosSubtable(format1Mapping, glyphMap))
handled.add(key)
Expand All @@ -419,7 +419,9 @@ def buildSinglePos(mapping, glyphMap):
for valueFormat, keys in masks.items():
f2 = [k for k in keys if k not in handled]
if len(f2) > 1:
format2Mapping = {coverages[k][0]: values[k] for k in f2}
format2Mapping = {}
for k in f2:
format2Mapping.update((g, values[k]) for g in coverages[k])
result.append(buildSinglePosSubtable(format2Mapping, glyphMap))
handled.update(f2)

Expand All @@ -428,8 +430,8 @@ def buildSinglePos(mapping, glyphMap):
# is unique as well. We encode these in format 1 again.
for key, glyphs in coverages.items():
if key not in handled:
assert len(glyphs) == 1, glyphs
st = buildSinglePosSubtable({glyphs[0]: values[key]}, glyphMap)
for g in glyphs:
st = buildSinglePosSubtable({g: values[key]}, glyphMap)
anthrotype marked this conversation as resolved.
Show resolved Hide resolved
result.append(st)

# When the OpenType layout engine traverses the subtables, it will
Expand Down Expand Up @@ -491,6 +493,14 @@ def _makeDeviceTuple(device):
return (device.DeltaFormat, device.StartSize, device.EndSize,
tuple(device.DeltaValue))

def _getSinglePosValueSize(valueKey):
count = 0
for k in valueKey[1:]:
if hasattr(k[1], '__len__') and len(k[1]):
count += len(k[1][3]) + 3
Copy link
Member

Choose a reason for hiding this comment

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

The calculation here does not take delta-format into account BTW.

Copy link
Member

Choose a reason for hiding this comment

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

oh.. when the device table has DeltaFormat == 0x8000 (aka VariationIndex table), it is always 3-ushort long, right?

Copy link
Member

Choose a reason for hiding this comment

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

btw, this otlLib.buildSinglePos function is currently only used within feaLib and mtiLib, which only ever output DeltaFormat 1, 2 or 3.

Copy link
Member

Choose a reason for hiding this comment

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

The DeltaFormat == 0x8000 is not handled by code in that module that converts Device table to a tuple. But that's not what I'm talking about. The code above assumes that each delta value takes 2 bytes. That's simply not true. Each delta value takes one of 1, 2, or 4 bits depending on DeltaFormat.

Copy link
Member

Choose a reason for hiding this comment

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

ok now i'm proper confused, so i'll leave that to you

Copy link
Member

Choose a reason for hiding this comment

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

cc @mhosken

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel touching it either. Martin can fix at some point.

Copy link
Member

Choose a reason for hiding this comment

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

OK I got it

https://docs.microsoft.com/en-us/typography/opentype/spec/chapter2#device-and-variationindex-tables

Format values 0x0001 to 0x0003 are used for Device tables, and indicate the format of delta adjustment values contained directly within the device table: signed 2-, 4,- or 8-bit values

Martin is just taking the len() of the device.DeltaValue (thus assuming one uint16 or 2 bytes per adjustment delta value) but we should instead count the length of the packed representation.

As far as I understand, this is not a major issue, we simply don't optimize as much as we could, right?

Copy link
Member

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I saw this when I wrote it, but decided not to complexify the code to handle a pretty rare use case. If someone who is actually using device records wants to refine the code, they can.

else:
count += 1
return count
anthrotype marked this conversation as resolved.
Show resolved Hide resolved

def buildValue(value):
self = ValueRecord()
Expand Down