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.

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

Fix Font Hinting #295

Merged
merged 16 commits into from
Oct 15, 2022
Merged

Fix Font Hinting #295

merged 16 commits into from
Oct 15, 2022

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #293 and #294

We were passing the multiplied pixel size when we shouldn't have. Some pre-cleartype fonts don't seem to work all that well so I've internally disabled hinting for them for now.

/// </para>
/// </summary>
private static readonly string[] BadHintList = { "Arial", "Times New Roman", "Tahoma", "Palatino Linotype", "Segoe UI" };
private bool? shouldHint;
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a LOT of knowledge in this document, unfortunately I couldn't find a way to get these fonts to hint well. They're all older ones that have known issues with hinting so to save effort I've disabled hinting of them for now.

http://rastertragedy.com/RTRCh4.htm#Sec1

Every single modern font I tested seems to work really well.

Copy link
Member

Choose a reason for hiding this comment

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

is there any metadata in the font we can use to determine if we should enable hinting? some created date maybe or so versioning flag difference? anything to save just hard coding a set of font names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not. 😔

There's some guidance for how to actually mitigate the issues I saw but I spent a few days and couldn't translate it into code. Nor could I find reference in FreeType that provided a complete workaround matching that guidance.

Accordingly I was looking for an approach that combines the advantages of overscaling before constraint application with the “safety” of overscaling after constraint application. To do so I “jury rigged” the rasterizer to dynamically re-interpret all rounding instructions to mean “round to sample boundary” if the rendering method is ClearType and if the direction of the projection vector p is not parallel to the long side of the LCD sub-pixels, else to keep the meaning “round to pixel boundary” as before:

  • Apply contextually interpreted constraints (“hints”) to outlines
  • Overscale constrained outlines in x-direction by 6x
  • Sample overscaled constrained outlines into overscaled bitmaps
  • Apply anti-aliasing filter to downsample overscaled bitmaps

Semantically, the two interpretations of rounding instructions are most similar. In fact, if you consider full-pixel bi-level rendering (colloquially “black-and-white” rendering) as “oversampling” at the rate of one sample per pixel, then the two interpretations are identical. Practically, this approach eliminates the “explosions” and it overcomes the absence of a TrueType instruction that would permit “finer grained” rounding than to the nearest 1/2 of a pixel (cf 4.3.0).

Copy link
Member Author

Choose a reason for hiding this comment

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

I do wonder whether we just just try tagging the chap who built the original interpreter to see if he knows how?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hate to do this and apologies in advance but @MikePopoloski would you have any insight into how to implement some of these workarounds?

Choose a reason for hiding this comment

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

Oh hello. No sorry, I haven't thought much about font rendering in like 7 years. Glad someone is making use of the interpreter though, it's a neat bit of code and there aren't (or at least weren't when I built it) too many open source implementations of a TT interpreter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for replying! That was a bit of a hit and hope tagging you like that.

I think there's very few people in the world who actually understand the TT interpreter. I've yet to find an implementation other than yours in the .NET ecosystem and elsewhere there just appears to be using Freetype.

I'll persevere though and continue to do my own research. Fongers crossed I can figure it all out. It's an interesting challenge!

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #295 (f704bbd) into main (6d831c6) will decrease coverage by 0%.
The diff coverage is 89%.

@@          Coverage Diff          @@
##            main    #295   +/-   ##
=====================================
- Coverage     83%     83%   -1%     
=====================================
  Files        222     224    +2     
  Lines      12281   12299   +18     
  Branches    1784    1788    +4     
=====================================
+ Hits       10272   10279    +7     
- Misses      1585    1600   +15     
+ Partials     424     420    -4     
Flag Coverage Δ
unittests 83% <89%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SixLabors.Fonts/ArrayBuilder{T}.cs 84% <ø> (ø)
src/SixLabors.Fonts/GlyphMetrics.cs 50% <0%> (ø)
src/SixLabors.Fonts/GlyphShapingData.cs 90% <0%> (-4%) ⬇️
src/SixLabors.Fonts/GlyphSubstitutionCollection.cs 89% <0%> (-1%) ⬇️
src/SixLabors.Fonts/MappedArraySlice{T}.cs 100% <ø> (ø)
src/SixLabors.Fonts/Tables/Cff/RefStack{T}.cs 41% <ø> (ø)
...nts/Tables/TrueType/Hinting/TrueTypeInterpreter.cs 50% <ø> (-2%) ⬇️
src/SixLabors.Fonts/ByteMemoryManager{T}.cs 40% <40%> (ø)
src/SixLabors.Fonts/ReadOnlyMappedArraySlice{T}.cs 83% <83%> (ø)
src/SixLabors.Fonts/StreamFontMetrics.TrueType.cs 97% <83%> (ø)
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JimBobSquarePants JimBobSquarePants requested a review from a team August 11, 2022 13:58
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Aug 11, 2022
@JimBobSquarePants
Copy link
Member Author

@tocsoft I've cracked it! 🎉

I had a good dig through the FreeType interpreter code and accompanying docs and figured out what they did to get their interpreter working well with legacy fonts. Basically, they never move anything horizontally when hinting at all which fixes most issues then there's a couple of additional tweaks for legacy fonts. No need for lists or targeted hacks - it just works!

Here's Tahoma (This was really bad before with odd offsetting of individual glyphs.)

HintingMode.HintXY
six_tahoma_hintxy

HintingMode.HintY
six_tahoma_hinty

HintingMode.None
six_tahoma_none

HintingMode.HintXY vs HintingMode.None (You can clearly see the hinting benefit in the 'o' here)
image

And here's Open Sans

HintingMode.HintXY (Both hint modes Y and XY produce identical results with this font)
six_open_sans_hintxy

HintingMode.None
six_open_sans_none

HintingMode.HintXY vs HintingMode.None (You can clearly see the hinting benefit in the 'o' here)
image

For most fonts there really miniscule difference between HintingMode.HintXY and HintingMode.HintY. There are some notable improvements in Times New Roman when using HintingMode.HintY in that the over-thinning of stems does not happen. I'm thinking of reworking the enum to rename HintingMode.HintY as HintingMode.Compatibility moving it to the third position as there's additional overheads (scaling of control point x-values before and after hinting) which, while SIMD optimized, you don't really want people using as a first-choice approach.

What do you think?

image

tocsoft
tocsoft previously approved these changes Oct 15, 2022
Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

This looks fantastic, congrats on getting it working without the black list.

@JimBobSquarePants
Copy link
Member Author

@tocsoft It gets better. Turns out I had my enum wired back-to-front in GlyphVector and I was oversampling in XY mode not Y. The default output was the best one!

I've deleted the oversampling code and changed the enum to None and Standard. I left the option as an enum in case we ever do additional hinting options.

tocsoft
tocsoft previously approved these changes Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Small font rendering really unclear (blurry, missing pixels etc)
3 participants