HMTX incorrect parsing

[sigurdle]

HMTX table seem to be incorrectly parsed

From the spec:
"If numberOfHMetrics is less than the total number of glyphs, then the hMetrics array is followed by an array for the left side bearing values of the remaining glyphs
"
You don't seem to be reading in that array, but instead using the last leftSideBearing value.
It should only be "advanceWidth" that is reused for remaining glyphs

Please convince me that I'm reading the spec wrong before closing the issue and just saying I'm wrong (as it was once before)

Here's the code as it's written now:

function parseHmtxTableAll(data, start, numMetrics, numGlyphs, glyphs) {
    let advanceWidth;
    let leftSideBearing;
    const p = new parse.Parser(data, start);
    for (let i = 0; i < numGlyphs; i += 1) {
        // If the font is monospaced, only one entry is needed. This last entry applies to all subsequent glyphs.
        if (i < numMetrics) {
            advanceWidth = p.parseUShort();
            leftSideBearing = p.parseShort();
        }

        const glyph = glyphs.get(i);
        glyph.advanceWidth = advanceWidth;
        glyph.leftSideBearing = leftSideBearing;
    }
}

[Connum]

Could you provide an example file and what is being parsed vs. how it's supposed to be?

[ILOVEPIE]

Unless there’s an error in the Microsoft documentation, what we have is correct.

[ILOVEPIE]

I looked at the diagram at the bottom of the page you linked in the microsoft documentation. which shows an array of structs containing both the advanceWidth and left side bearing in the array.

https://learn.microsoft.com/en-us/typography/opentype/spec/hmtx

[ILOVEPIE]

see the table "longHorMetric Record"

[sigurdle]

Ok, so this is what it sais:

"The table uses a longHorMetric record to give the advance width and left side bearing of a glyph. Records are indexed by glyph ID. As an optimization, the number of records can be less than the number of glyphs, in which case the advance width value of the last record applies to all remaining glyph IDs
"

So, that only applies to advanceWidth

Then a bit later:

"If numberOfHMetrics is less than the total number of glyphs, then the hMetrics array is followed by an array for the left side bearing values of the remaining glyphs"

But the code in opentype.js treats advanceWidth and "left side bearing" the same

No I don't have any example files, the only effect of this bug, would be that all glyphs get the same leftsideBearing, even though there actually is more data that supplies leftsideBearing for the rest of the glyphs

(This is my understanding anyway, and most fonts probably don't even use this feature, but If you want to be correct...)

[ILOVEPIE]

The issue is that the way the struct is setup, it's not possible to do one element of the struct but not the other, the way C style structs work is that the struct has a fixed length. So, an array of structs must have elements of a fixed length. the code has to be like this because each record must contain both a advancedWidth (16 bits) and "left side bearing" (16bits) for it to be in the array, because otherwise it doesn't follow the longHorMetric struct (which is 32bits in length):

function parseHmtxTableAll(data, start, numMetrics, numGlyphs, glyphs) {
    let advanceWidth;
    let leftSideBearing;
    const p = new parse.Parser(data, start);
    for (let i = 0; i < numGlyphs; i += 1) {
        // If the font is monospaced, only one entry is needed. This last entry applies to all subsequent glyphs.
        if (i < numMetrics) {
            advanceWidth = p.parseUShort();  // 16 bits 
            leftSideBearing = p.parseShort(); // 16 bits
        }

        const glyph = glyphs.get(i);
        glyph.advanceWidth = advanceWidth;
        glyph.leftSideBearing = leftSideBearing;
    }
}

[ILOVEPIE]

now, that being said, we could have an issue where the documentation is trying to say the left side bearing defaults to zero after there's no more metric entries.

[ILOVEPIE]

but i think this is likely an oversight where they forgot to mention the other variable.

[sigurdle]

I fail to see the problem you're describing. In order to implement the spec:

function parseHmtxTableAll(data, start, numMetrics, numGlyphs, glyphs) {
    let advanceWidth;
    const p = new parse.Parser(data, start);
    for (let i = 0; i < numMetrics; i += 1) {
          advanceWidth = p.parseUShort();  // 16 bits 
         const   leftSideBearing = p.parseShort(); // 16 bits

        const glyph = glyphs.get(i);
        glyph.advanceWidth = advanceWidth;
        glyph.leftSideBearing = leftSideBearing;
    }
// If numGlyphs > numMetrics
    for (let i = numMetrics; i < numGlyphs; i += 1) {
        const glyph = glyphs.get(i);
        glyph.advanceWidth = advanceWidth;//same as from previous loop
        glyph.leftSideBearing = p.parseShort(); // 16 bits
    }

}

[ILOVEPIE]

That doesn't work, it breaks the rules of C-style structures which fonts are built using, you cannot change the size of an element of an array of structures as they must be the same type. longHorMetric must always be 32bits long, here you change it after numMetrics is up to 16 bits long which cannot happen.

[ILOVEPIE]

the struct's size is always the sum of its member's sizes.

[sigurdle]

Ahh jesus :)... I know perfectly well how C structs work.. I do NOT want to be condescending or "teachy", but It's not like a "font table" necessarily is a C-struct, read the spec, it sais it right there:

"If numberOfHMetrics is less than the total number of glyphs, then the hMetrics array is followed by an array for the left side bearing values of the remaining glyphs"

Again, you seem to have some misconception here.. is the misconception that you think that every "Table" in a font file necessarily is a(n array) of C-strcucts ?

[ILOVEPIE]

I'm not trying to be condescending. That was just the way I read it. I think you're reading too much into what i'm saying. I wasn't sure if you knew so I was just covering my bases.

[ILOVEPIE]

OK, just reread it, I misread it the first time, you are correct. Sorry about that, if you want to submit a PR, please feel free.

[sigurdle]

Sure, I didn't think you were trying to be condescending :) I just didn't want you to think I didn't know what I was talking about :)

[ILOVEPIE]

@sigurdle Were you going to submit a PR or did you want us to address this issue for you?