Added OpenType OS/2 and HHEA table parsing for Skia Glyph Typeface to fix text rendering issues

[AngryCarrot789]

What does the pull request do?

This PR implements parsing of OpenType's OS/2 and HHEA tables for the Skia implementation of IGlyphTypeface. It uses the tables' metrics to calculate more accurate font metrics (specifically ascent, descent and line gap).

Rather than using HarfBuzz's OpenType metrics for the ascent and descent, this PR tries to use usWinAscent and usWinDescent from the OS/2 table, and it calculate the line gap from that and the HHEA table metrics

What is the current behavior?

Fonts such as Consolas appear to have a vertical offset (excessive empty space below the actual text). This is visible in things like the AvalonEdit TextEditor (the avalonia remake version). This is also visible in the ControlCatalog project of this repo, in the ComboBox tab, and then selecting Consolas on the 2nd combo box on the right (you will see excessive white space below the text)

What is the updated/expected behavior with this PR?

Most if not all fonts should be rendered correctly now

Fixed issues

This fixes the problems I mentioned in a discussion: #15262
I think this also fixes #10658

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047252-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[kekekeks]

Do we need both tables? libgdiplus is using only one of those with os2 table taking priority:
https://github.com/mono/libgdiplus/blob/94a49875487e296376f209fe64b921c6020f74c0/src/font.c#L757-L792

[kekekeks]

Hmm, actually, it analyzes the os2 table if it exists and uses its values instead of hhea if certain flags are set.

We probably want to just port that logic as is.

[AngryCarrot789]

I implemented the libgdiplus code in GlyphTypefaceImpl and this is what I got:

Maybe I implemented it incorrectly? I noticed that sTypoDescender was in the order of 65000 for fonts like Inter (which seems to be the first loaded font family), whereas usWinDescent was around 200.

This is the code I wrote:

private static bool TryReadFontMetrics(SKTypeface typeface, out int ascent, out int descent, out int lineGap)
{
    const int TagOs2 = 1330851634; // pre-computed value for HarfBuzzSharp.Tag.Parse("OS/2")
    const int TagHhea = 1751672161; // pre-computed value for HarfBuzzSharp.Tag.Parse("hhea")

    // See: https://learn.microsoft.com/en-us/typography/opentype/spec/recom#baseline-to-baseline-distances
    // See Also: https://github.com/mono/libgdiplus/blob/94a49875487e296376f209fe64b921c6020f74c0/src/font.c#L757-L792
    if (typeface.TryGetTableData(TagOs2, out byte[]? os2Table) && typeface.TryGetTableData(TagHhea, out byte[]? hheaTable))
    {
        if (ReadHHEATable(hheaTable, out int hheaAscender, out int hheaDescender, out int hheaLineGap))
        {
            bool hasOs2 = ReadOS2Table(os2Table, out OS2TableInfo os2);

            const int fsSelectionUseTypoMetrics = (1 << 7);
            if (hasOs2 && (os2.usWinAscent <= 0 || os2.usWinDescent <= 0))
            {
                // System.Diagnostics.Debugger.Break();
            }

            if (hasOs2 && (os2.fsSelection & fsSelectionUseTypoMetrics) != 0)
            {
                /* Use the typographic Ascender, Descender, and LineGap values for everything. */
                // This is the most common case using these values. sTypoDescender is huge, same with hheaDescender
                lineGap = os2.sTypoAscender - os2.sTypoDescender + os2.sTypoLineGap;
                descent = os2.sTypoDescender;
                ascent = os2.sTypoAscender;
            }
            else
            {
                /* Calculate the LineSpacing for both the hhea table and the OS/2 table. */
                int hhea_linespacing = hheaAscender + Math.Abs(hheaDescender) + hheaLineGap;
                int os2_linespacing = hasOs2 ? (os2.usWinAscent + os2.usWinDescent) : 0;

                /* The LineSpacing is the maximum of the two sumations. */
                lineGap = Math.Max(hhea_linespacing, os2_linespacing);

                /* If the OS/2 table exists, use usWinDescent as the
                 * CellDescent. Otherwise use hhea's Descender value. */
                descent = hasOs2 ? os2.usWinDescent : hheaDescender;

                /* If the OS/2 table exists, use usWinAscent as the
                 * CellAscent. Otherwise use hhea's Ascender value. */
                ascent = hasOs2 ? os2.usWinAscent : hheaAscender;
            }

            return true;
        }
    }

    ascent = descent = lineGap = 0;
    return false;
}

private static bool ReadOS2Table(byte[]? array, out OS2TableInfo os2)
{
    os2 = default;
    if (array == null || array.Length < 78)
    {
        return false;
    }

    // 62 is the offset to the fsSelection field in the OS/2 table
    var span = new Span<byte>(array, 62, 16);

    os2.fsSelection = BinaryPrimitives.ReadUInt16BigEndian(span);
    os2.sTypoAscender = BinaryPrimitives.ReadUInt16BigEndian(span.Slice(6));
    os2.sTypoDescender = BinaryPrimitives.ReadUInt16BigEndian(span.Slice(8));
    os2.sTypoLineGap = BinaryPrimitives.ReadUInt16BigEndian(span.Slice(10));
    os2.usWinAscent = BinaryPrimitives.ReadUInt16BigEndian(span.Slice(12));
    os2.usWinDescent = BinaryPrimitives.ReadUInt16BigEndian(span.Slice(14));
    return true;
}

private struct OS2TableInfo
{
    public ushort fsSelection;
    public ushort sTypoAscender;
    public ushort sTypoDescender;
    public ushort sTypoLineGap;
    public ushort usWinAscent;
    public ushort usWinDescent;
}

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047258-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047264-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[robloo]

This indeed seems to fix some of the issues. However, I'm still seeing some problematic fonts and a few side-effects.

If we can use an existing algorithm we should. This is more complicated than it seems but I think we are narrowing in on a solution.

• "Calibri" looks good now
• "Cambria Math" is way too tall now (although centered)
• "Courier New" and "Javanese Text" seem wrong still
• "Microsoft Himalaya" has the same issue this attempted to fix

[AngryCarrot789]

This indeed seems to fix some of the issues. However, I'm still seeing some problematic fonts and a few side-effects.

If we can use an existing algorithm we should. This is more complicated than it seems but I think we are narrowing in on a solution.

• "Calibri" looks good now
• "Cambria Math" is way too tall now (although centered)
• "Courier New" and "Javanese Text" seem wrong still
• "Microsoft Himalaya" has the same issue this attempted to fix

I noticed cambria math too, but I assumed that was how it's supposed to be, based on notepad:

Microsoft Himalaya also looks weird in notepad with the excessive whitespaces:

[robloo]

@AngryCarrot789 Yea, I should have double checked this with other sources. If Notepad looks the same (and that's using the super old and well tested Win32 control) we should be good.

[AngryCarrot789]

This PR works fine for me on KDE Neon's Plasma 6 (text editor uses Oxanium font)

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047296-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[Gillibald]

This is what SixLabors.Fonts uses: https://github.com/SixLabors/Fonts/blob/80974d925832c32405d7a623464b742c6299f33f/src/SixLabors.Fonts/StreamFontMetrics.cs#L404

https://www.w3.org/TR/css-inline-3/#ascent-descent

[AngryCarrot789]

@Gillibald That code seems pretty much the same as code I tried in response to @kekekeks, where each line appeared to take up 1000s of pixels, and the only unusual part is that the sTypoDescent (from OS/2) and the ascent field in the hhea table were massive (in the order of 60,000 for a lot of fonts). I don't think I'm reading the OS/2 table incorrectly so I'm not sure what's up. Using the windows metrics does seem to work on windows and, from testing, X11 (ubuntu, KDE Plasma 6)

[jp2masa]

the only unusual part is that the sTypoDescent (from OS/2) and the ascent field in the hhea table were massive (in the order of 60,000 for a lot of fonts).

Considering that those values are 16-bit integers, it sounds like an endianness problem, so probably worth double-checking.

[AngryCarrot789]

@jp2masa If you look at the code I posted, all the fields are read in big endian (which is what the tables are always set at), so sTypoDescent would have to be the only field that is specifically little-endian

[jp2masa]

I see, the problem seems to be that you're reading uint16 instead of int16, i.e. you should use ReadInt16BigEndian instead of ReadUInt16BigEndian.

[AngryCarrot789]

@jp2masa Ah I didn't notice that :P I pushed a new update that uses the libgdiplus technique

[AngryCarrot789]

Even after fixing the Unsigned/Signed issue, text still did not render correctly so I put the code back to just using either OS/2 windows metrics or the normal HarfBuzz metrics

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047381-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[jp2masa]

Looking at the naming, there is a mismatch: lineGap/lineSpacing. I think you need to adapt the gdi code, although the else logic isn't very straightforward: probably doing lineGap = Math.Max(hhea_linespacing, os2_linespacing) - ascent - descent would work.

[Gillibald]

This is what harfbuzz does: https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-ot-metrics.cc#L81

It uses OS/2 values if they are available and the flag indicates it should be used and falls back to hhea.

So it is already doing the right thing.

So I wonder why all this is needed.

[AngryCarrot789]

I might try and figure this out on the weekend. For now I'm gonna stick with 11.2.999-cibuild0047381-alpha, which works perfectly fine using only the usWinAscent/usWinDescent metrics (as far as I can see)

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0047984-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[cla-avalonia]

• All contributors have signed the CLA.

[AngryCarrot789]

@cla-avalonia agree

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048348-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]