829523 - To hint or not to hint, that is the question

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

From bug 816614 comment 42
-----
I had a bug for the hinting a few months back (https://bugzilla.mozilla.org/show_bug.cgi?id=804147). See examples of pre and post hinting rendering within. The main issue is that our current hinting snaps to the nearest pixel which causes the fonts to look too sharp and even distorts certain glyphs like the "e" at different sizes.

My ideal solution is keep the hinting off (atleast at our current shipping resolution) and keep the correct glyph spacing on.
----

jfkthame's attachment 700325 [details] [diff] [review] seems to improve glyph spacing with hinting disabled, so that patch might give us the option of disabling hinting.

How can we make the final call here?

Comment 1

[Jonathan Kew [:jfkthame]]

Note that attachment 700325 [details] [diff] [review] by itself is too much of a blunt instrument: it breaks a number of existing android reftests, and as karlt pointed out, it'd be bad for rendering to non-raster backends (printing, pdf, ...). So it needs some refinement, but at least it shows that it's possible for us to render the unhinted text nicely.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - disable hinting globally on b2g

If we decide to use unhinted rendering throughout b2g, the simplest approach would be to make FontHintingEnabled() return false unconditionally. We could remove the existing hinted-everywhere-except-browser code altogether, but I'd be inclined to leave it in the file (unused) for now; we might still want that option in some future environment. So this patch alone would give us unhinted fonts throughout b2g, but with poor glyph spacing; the following patch will fix that.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - FT2FontEntry shouldn't set CAIRO_HINT_METRICS_OFF even if the platform doesn't want hinting; the "hint metrics" option is unrelated to hinting of the actual glyphs

Given that cairo's HINT_METRICS isn't really about hinting as such, it seems to me that FT2FontEntry is wrong to set HINT_METRICS_OFF, and we should simply remove that piece of code. This will fix the glyph spacing problems in unhinted text on b2g. (Note that it -will- also affect text rendering on Android, but it's hard to say whether it gets better or worse... to my eye, some examples look better one way, some the other, more or less at random.)

Comment 4

[Karl Tomlinson (:karlt)]

The other option to consider for hinting is FT_LOAD_TARGET_LIGHT which can stay closer to unhinted glyph shapes than the hinting instructions in the font might.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 701184 [details] [diff] [review]
pt 2 - FT2FontEntry shouldn't set CAIRO_HINT_METRICS_OFF even if the platform doesn't want hinting; the "hint metrics" option is unrelated to hinting of the actual glyphs

Sorry, I don't have time to review this properly.
I would have expected CAIRO_HINT_METRICS_OFF to still help for zoomed content
(bug 816614 comment 35), but any additional adjustments just add more noise.  Imagine snapping at a small size and drawing much larger (assuming we do that).

Still, I'll let someone else consider how zooming is down and analyse the differences and decide.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 701180 [details] [diff] [review]
pt 1 - disable hinting globally on b2g

I would just remove all this cruft and |return false;| with an explanatory comment.

r+ on code change, but awaiting decision from elsewhere.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - FT2FontEntry shouldn't set CAIRO_HINT_METRICS_OFF even if the platform doesn't want hinting

In testing on tryserver, this seems to introduce a frequent (but not quite permanent) failure in layout/reftests/scrolling/text-2.html; marking this as fuzzy, as it looks like a few residual antialiasing pixels at the edge of the scrolled area. On the bright side, it makes layout/reftests/text/475092-pos.html start passing, so we can update the annotation there accordingly.

Comment 8

[Patryk Adamczyk [:patryk] UX]

Are there different hinting filters we can try and see some screenshots? The current hinting setting just doesn't work as it snaps to the nearest pixel.

Comment 9

[Jonathan Kew [:jfkthame]]

We could try freetype's "light" hinting, as Karl suggested in comment 4. I'll create a build with that, and post some comparison screenshots.

Comment 10

[Jonathan Kew [:jfkthame]]

I've made some screenshots (from an Unagi phone) comparing our hinting options:

    http://people.mozilla.org/~jkew/b2g-hinting/

This allows you to view all the screenshots in a big table, or to switch between the various renderings of a specific screen. The rendering differences are much more obvious on some screens than others, depending on the particular font sizes being used.

Personally, I find that in some cases I prefer "None", and in others I prefer "Slight"; I don't much like the "Full" setting as it alters the glyph shapes much more severely, without (IMO) actually improving readability at the sizes/resolution we're using. But a lot of this is quite subjective, of course...

Patryk & co, please review the options and let us know what setting you'd like the platform to use.

Comment 11

[Patryk Adamczyk [:patryk] UX]

I'd prefer OFF. I agree with you Jonathan its a toss up between slight and off. However slight does seems to exhibit weird character distortions, look at "Offline" under Wifi in the Settings lights screenshot. The 2nd "f" is severely distorted.

So I'd opt for "OFF" hinting. Can we still turn on Spacing? 
Or does no hinting, mean no spacing.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #11)
> I'd prefer OFF. I agree with you Jonathan its a toss up between slight and
> off. However slight does seems to exhibit weird character distortions, look
> at "Offline" under Wifi in the Settings lights screenshot. The 2nd "f" is
> severely distorted.

Yes, that's the most glaring issue I noticed with it. If it hadn't been for that glitch, I think I'd have favoured the "slight" setting.

(FWIW, my guess is that it reflects a limitation of the FreeType auto-hinter, which is used by the "slight" setting: I think it's not recognizing that the "fi" ligature should get similar treatment to the lowercase Latin letters with ascenders, in order to maintain consistency. Because that glyph doesn't have a direct Unicode mapping, the auto-hinter is probably unable to determine how to handle it.)

> So I'd opt for "OFF" hinting. Can we still turn on Spacing? 
> Or does no hinting, mean no spacing.

Yes, we can have "correct" spacing even with hinting off; that should be what you see in the "None" screenshots here.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: disable font hinting on b2g, but retain pixel-snapped metrics except in the browser app.

So what we really need to do here is to distinguish between actual font hinting, which we want to turn off globally for b2g, and what cairo (confusingly) calls 'hinted' (really pixel-snapped) metrics, which we want to keep enabled except in the actual web browser, where non-reflowing zoom means that it's as likely to be unhelpful as helpful. Controlling both these things from the single FontHintingEnabled() setting cannot work correctly for all circumstances, so this patch introduces a separate RequiresLinearZoom() setting that returns FALSE for b2g in general, but TRUE in the browser app, even though FontHintingEnabled() is FALSE for both cases.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 701180 [details] [diff] [review]
pt 1 - disable hinting globally on b2g

This is superseded by attachment 701974 [details] [diff] [review].

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d533ba255bbb

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 701974 [details] [diff] [review]
disable font hinting on b2g, but retain pixel-snapped metrics except in the browser app.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 816614

User impact if declined: less-pleasing font rendering as a side-effect of 816614

Testing completed: UX reviewed screenshots of the various font-hinting options for B2G, chose this as the preferred rendering

Risk to taking this patch (and alternatives if risky): low risk

String or UUID changes made by this patch: none

Comment 17

[Ed Morley [:emorley]]

Backed out for turning reftest-6 orange:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18813331&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18815350&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/28a2d814fda4

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: disable font hinting on b2g, but retain pixel-snapped metrics except in the browser app.

Sigh... that reftest fails because of three antialiasing pixels at the edge of one of the elements, overlapping the border of the neighboring element. Marking it accordingly as fuzzy-if(B2G,...). Carrying over r=cjones for the actual code patch.

Comment 19

[Jonathan Kew [:jfkthame]]

Re-landed with the problem reftest marked fuzzy.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a103f1717df1

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 702309 [details] [diff] [review]
disable font hinting on b2g, but retain pixel-snapped metrics except in the browser app.

[Approval Request Comment]
See comment 16; just moving approval request to the updated patch.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a103f1717df1

Comment 22

[Andrew Overholt [:overholt]]

Triage decided today that this should be approved for uplift and tracking but not tef+.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This bug depends on bug 816614, which is still pan-handling for approval.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Not anymore!)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0d88421632bd

(Made sure to preserve the test-pref(layout.css.flexbox.enabled,true) in the reftest manifest)

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: followup - use MOZ_WIDGET_GONK for the platform conditional, rather than MOZ_B2G

According to glandium and WG9s (in #developers a few minutes ago), it would be better to use MOZ_WIDGET_GONK here. Shouldn't have any effect on the final build, provided both symbols are defined as expected.

Comment 27

[Karl Tomlinson (:karlt)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> I've made some screenshots (from an Unagi phone) comparing our hinting
> options:
> 
>     http://people.mozilla.org/~jkew/b2g-hinting/

That makes comparison easy, thanks.

I wonder why none of the options seem to have any effect on the horizontal position of vertical stems.

Comment 28

[Karl Tomlinson (:karlt)]

Oh, horizontal hinting is disabled with FT_LOAD_TARGET_LIGHT:
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/autofit/aflatin.c?id=3a55340029783ec9ab738698e3e3ab2cd74c882e#n1575

It seems FT_LOAD_FORCE_AUTOHINT would be required to get horizontal hinting with this font, but that would still have the problem in ligatures without Unicode points.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 704229 [details] [diff] [review]
followup - use MOZ_WIDGET_GONK for the platform conditional, rather than MOZ_B2G

Really doesn't matter but W.  We need to get away from WIDGET_GONK for feature testing, but MOZ_B2G isn't any better either.

Comment 30

[Jonathan Kew [:jfkthame]]

Followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae2b53bd0b7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ae2b53bd0b7