Bug 122155 - [WK2] Font hinting should be disabled
Summary: [WK2] Font hinting should be disabled
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-01 04:43 PDT by Allan Sandfeld Jensen
Modified: 2013-10-01 10:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.70 KB, patch)
2013-10-01 05:04 PDT, Allan Sandfeld Jensen
darin: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-10-01 04:43:40 PDT
If font hinting is enabled on the platform, Qt will hint layed out text to document-pixels which may not be same as the final device pixels. This can leed to very poor character spacing at times.
Comment 1 Allan Sandfeld Jensen 2013-10-01 04:58:14 PDT
This probably also applies to GTK and EFL. Any platform that may enable full hinting by default
Comment 2 Allan Sandfeld Jensen 2013-10-01 05:04:26 PDT
Created attachment 213074 [details]
Patch
Comment 3 Allan Sandfeld Jensen 2013-10-01 05:08:11 PDT
This is a simple patch to just disable font-hinting in the specific case. A more general approach should probably be used to fix the same issue elsewhere Font::drawSimpleText is used with a graphicscontext with a transform more advanced than translation.

Ideally for perfect scaled hinting getGlyphsAndAdvancesForSimpleText and all the WidthIterator routines should be able to deal with a zoom factor.
Comment 4 Allan Sandfeld Jensen 2013-10-01 05:12:20 PDT
This is a simple patch to just disable font-hinting in the specific case. A more general approach should probably be used to fix the same issue elsewhere Font::drawSimpleText is used with a graphicscontext with a transform more advanced than translation.

Ideally for perfect scaled hinting getGlyphsAndAdvancesForSimpleText and all the WidthIterator routines should be able to deal with a zoom factor.
Comment 5 Build Bot 2013-10-01 05:38:34 PDT
Comment on attachment 213074 [details]
Patch

Attachment 213074 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2892074
Comment 6 Build Bot 2013-10-01 06:55:11 PDT
Comment on attachment 213074 [details]
Patch

Attachment 213074 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2905111
Comment 7 Martin Robinson 2013-10-01 08:57:03 PDT
Won't disabling font hinting seriously hurt the quality of font rendering?
Comment 8 Allan Sandfeld Jensen 2013-10-01 09:21:16 PDT
(In reply to comment #7)
> Won't disabling font hinting seriously hurt the quality of font rendering?

Depends. Apple for instance prefer no or very little hinting by default. It makes things look fuzzy but more print-like (especially if you squint or have a very high res screen). Which is why this issue probably doesn't affect them. Also if the content is already transformed and the text not aligned to pixel points you do not lose anything extra by disabling hinting. 

The patch does however only set preference to vertical hinting under the assumption that the rendering engine will still attempt to put the start point of the text-run on a device pixels, and even if it doesn't vertical hinting will not make things look worse, like horizontal hinting between characters can.
Comment 9 Martin Robinson 2013-10-01 09:26:17 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Won't disabling font hinting seriously hurt the quality of font rendering?
> 
> Depends. Apple for instance prefer no or very little hinting by default. It makes things look fuzzy but more print-like (especially if you squint or have a very high res screen). Which is why this issue probably doesn't affect them. Also if the content is already transformed and the text not aligned to pixel points you do not lose anything extra by disabling hinting. 

Wouldn't it make sense to keep this as a per-port decision then? I don't think GTK+ wants to disable hinting at the moment.

> The patch does however only set preference to vertical hinting under the assumption that the rendering engine will still attempt to put the start point of the text-run on a device pixels, and even if it doesn't vertical hinting will not make things look worse, like horizontal hinting between characters can.

That's actually another strange thing about the patch. The Qt implementation switches to vertical hinting, but the setting is called setShouldNotUseHinting.
Comment 10 Darin Adler 2013-10-01 09:55:04 PDT
Comment on attachment 213074 [details]
Patch

I don’t think this makes sense. We don’t want to add a global to Font when no port is looking to make this dynamic at runtime. Especially if this feature is for the sunsetting Qt port. Can you just put this unconditionally in FontPlatformDataQt.cpp? Or do this in a Qt fork since we’re near the end of Qt in the main WebKit tree.
Comment 11 Allan Sandfeld Jensen 2013-10-01 10:43:37 PDT
(In reply to comment #10)
> (From update of attachment 213074 [details])
> I don’t think this makes sense. We don’t want to add a global to Font when no port is looking to make this dynamic at runtime. Especially if this feature is for the sunsetting Qt port. Can you just put this unconditionally in FontPlatformDataQt.cpp? Or do this in a Qt fork since we’re near the end of Qt in the main WebKit tree.

The patch was made under the assumption it would be generally useful, though the strategy of the solution is probably the wrong one.