Compress type 1 GPOS tables better #1539
Conversation
|
Thanks. I see how this is an improvement. One day I'll implement an optimal packer. This is quite similar to the VarStore optimizer. |
|
@mhosken please let me know if you intend to address the review comments above, so I can go on merging this, thank you. |
|
Sorry. Yes I do plan to tidy this up, although I have a conference next
week so it may be easy or hard ;) But I do intend to get this merged.
…On Thu, 4 Apr 2019, 17:10 Cosimo Lupo, ***@***.***> wrote:
@mhosken <https://github.com/mhosken> please let me know if you intend to
address the review comments above, so I can go on merging this, thank you.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1539 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFbHbTo3OSvmRA7jhx5_R-UNvZghDIvUks5vdc-VgaJpZM4b1raM>
.
|
|
Thanks |
| count = 0 | ||
| for k in valueKey[1:]: | ||
| if hasattr(k[1], '__len__') and len(k[1]): | ||
| count += len(k[1][3]) + 3 |
There was a problem hiding this comment.
The calculation here does not take delta-format into account BTW.
There was a problem hiding this comment.
oh.. when the device table has DeltaFormat == 0x8000 (aka VariationIndex table), it is always 3-ushort long, right?
There was a problem hiding this comment.
btw, this otlLib.buildSinglePos function is currently only used within feaLib and mtiLib, which only ever output DeltaFormat 1, 2 or 3.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok now i'm proper confused, so i'll leave that to you
There was a problem hiding this comment.
I don't feel touching it either. Martin can fix at some point.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Device tables are indeed very rare, but now with variable fonts, we have a lot of VariationIndex tables. I am having to touch these lines in order to support variable fonts with a GPSO vpal feature, so I can fix it. I see the spec is confusing: in the text, it implies that there is a uint16 in the delta array for each integer size in the inclusive size range. Fortunately, the example makes it clear that there is a delta value for each ppem size in the size range, the bit size of a delta value is specified by the delta flag, and these are packed into the uint16 array sequentially as if the array as a continuous byte string. |
The current code has a tendency to create lots of little subtables for a big set of key values. This is not only wasteful (it takes 5 uint16 overhead for a new type 1 subtable, and if your value record is a single value, that means you can get 5 keys in the main coverage table for the same space), it is slower since an engine can only search subtables linearly.
This PR does a more careful calculation of value record size and then decides whether it really needs a subtable or not based on the number of keys and the size of the value.
This PR fails the same tests that the master it is based on fails.