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
Added OpenType OS/2 and HHEA table parsing for Skia Glyph Typeface to fix text rendering issues #15344
base: master
Are you sure you want to change the base?
Conversation
You can test this PR using the following package version. |
if (typeface.TryGetTableData(GetIntTag("OS/2"), out byte[]? os2Table) && | ||
typeface.TryGetTableData(GetIntTag("hhea"), out byte[]? hheaTable)) | ||
{ | ||
if (ReadOS2Table(os2Table, out int usWinAscent, out int usWinDescent) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
…usage. Pre computed the OS2 and HHEA table tags
You can test this PR using the following package version. |
You can test this PR using the following package version. |
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.
|
@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. |
You can test this PR using the following package version. |
@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) |
Considering that those values are 16-bit integers, it sounds like an endianness problem, so probably worth double-checking. |
@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 |
I see, the problem seems to be that you're reading uint16 instead of int16, i.e. you should use |
@jp2masa Ah I didn't notice that :P I pushed a new update that uses the libgdiplus technique |
You can test this PR using the following package version. |
Looking at the naming, there is a mismatch: |
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. |
I might try and figure this out on the weekend. For now I'm gonna stick with |
You can test this PR using the following package version. |
|
@cla-avalonia agree |
You can test this PR using the following package version. |
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
andusWinDescent
from the OS/2 table, and it calculate the line gap from that and the HHEA table metricsWhat 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