Closed Bug 829523 Opened 7 years ago Closed 7 years ago
To hint or not to hint, that is the question
7.53 KB, patch
|Details | Diff | Splinter Review|
2.08 KB, patch
|Details | Diff | Splinter Review|
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?
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.
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.
Attachment #701180 - Flags: review?(jones.chris.g)
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.)
Attachment #701180 - Attachment description: bug--pt1 → pt 1 - disable hinting globally on b2g
Attachment #701184 - Attachment description: bug--pt2 → 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
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 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 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.
Attachment #701180 - Flags: review?(jones.chris.g) → review+
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.
Attachment #701184 - Attachment is obsolete: true
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.
Whiteboard: visual design
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.
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.
Whiteboard: visual design → visual design, ptracking
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.
(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.
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.
Attachment #701974 - Flags: review?(jones.chris.g)
Comment on attachment 701180 [details] [diff] [review] pt 1 - disable hinting globally on b2g This is superseded by attachment 701974 [details] [diff] [review].
Attachment #701180 - Attachment is obsolete: true
Attachment #701475 - Attachment is obsolete: true
7 years ago
Attachment #701974 - Flags: review?(jones.chris.g) → review+
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
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.
Re-landed with the problem reftest marked fuzzy. https://hg.mozilla.org/integration/mozilla-inbound/rev/a103f1717df1
Assignee: nobody → 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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
7 years ago
Attachment #702309 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Triage decided today that this should be approved for uplift and tracking but not tef+.
This bug depends on bug 816614, which is still pan-handling for approval.
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)
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.
Attachment #704229 - Flags: review?(jones.chris.g)
Whiteboard: visual design, ptracking → visual design
(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.
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 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.
Attachment #704229 - Flags: review?(jones.chris.g) → review+
You need to log in before you can comment on or make changes to this bug.