Bug 176351 - [GTK] Bump freetype version to 2.8.0
Summary: [GTK] Bump freetype version to 2.8.0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 176501
Blocks: 149703 170500
  Show dependency treegraph
 
Reported: 2017-09-05 00:29 PDT by Carlos Garcia Campos
Modified: 2017-09-07 03:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.34 KB, patch)
2017-09-06 04:39 PDT, Carlos Garcia Campos
clopez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-09-05 00:29:49 PDT
Freetype 2.8.0 was released in February and it's the latest stable version. Upgrading freetype usually requires a major rebaseline of the tests, so it's better to upgrade to something newer.
Comment 1 Carlos Garcia Campos 2017-09-05 11:15:35 PDT
I've been working on this and I've realized that after the 2.8 upgrade we have some failures that don't look correct. The problem is not new, though see bug #102374. That bug was closed because a patch was added for freetype 2.4.11, it was a cherry-pick of a freetype commit (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=e0469372be3870a5ad60b2c4586e9c281357bd28), but that was reverted in freetype just a few revisions later (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=87dc86d68c93c2b5b836501ece57583d1d6d1a2e), so when upgrading to 2.8 I decided not to include the patch. 

So, with 2.8 now we are getting:

ascent: 15.000000, descent: 4.000000, height: 18.000000

while with previous patched version we got:

ascent: 14.000000, descent: 3.000000, height: 17.000000

this means that now ascent + descent > height and we are getting a lineGap of -1.

I don't want to keep using a patch that has been reverted upstream. This patch is also the reason why we get different results in many layout tests when not using the internal jhbuild. Also, I don't know why, but I tried to backport the patch to 2.8 just to check what happened and I was still getting -1, so that's no longer a solution.

I'm not a fonts expert but the problem seems to be the rounding done by cairo when using hint metrics. When not using them what we get is:

ascent: 14.257812, descent: 3.460938, height: 18.398438

where ascent + descent <= height and we get a line gap of 0.679688

I think we have several options here:

a) Try to clamp the line gap to be 0 when we get a negative value
b) Create a temporary cairo font with hint metrics disable only to get the metrics (this is the patch proposed by Dominik in bug #102374).
c) Use freetype api directly to get the metrics instead of cairo.

I don't know if these are valid solutions or if there's a better one. Thoughts?
Comment 2 Carlos Garcia Campos 2017-09-06 04:39:44 PDT
Created attachment 320008 [details]
Patch

This patch upgrades to 2.8 and removes the patch we were using. Instead it uses Dominiks's patch (slightly modified just ot use smart pointers and a fe other cosmetic changes). With this I'm getting 5886 failures. The differences are changes in the text height in 1px. I'm preparing the rebaseline in blocks.
Comment 3 Carlos Garcia Campos 2017-09-06 06:02:13 PDT
I have the rebaselines ready in 7 patches:

$ ls -lh 000*.patch
-rw-r--r-- 1 cgarcia cgarcia 36M sep  6 14:55 0001-rebaseline-1.patch
-rw-r--r-- 1 cgarcia cgarcia 20M sep  6 14:55 0002-rebaseline-2.patch
-rw-r--r-- 1 cgarcia cgarcia 27M sep  6 14:55 0003-rebaseline-3.patch
-rw-r--r-- 1 cgarcia cgarcia 42M sep  6 14:55 0004-rebaseline-4.patch
-rw-r--r-- 1 cgarcia cgarcia 32M sep  6 14:55 0005-rebaseline-5.patch
-rw-r--r-- 1 cgarcia cgarcia 39M sep  6 14:56 0006-rebaseline-6.patch
-rw-r--r-- 1 cgarcia cgarcia 42M sep  6 14:56 0007-rebaseline-7.patch

Hopefully future freetype upgrades will not be so painful now that we don't patch it anymore.
Comment 4 Carlos Alberto Lopez Perez 2017-09-06 06:39:22 PDT
Comment on attachment 320008 [details]
Patch

Thanks! getting rid of patches on our jhbuild is always a good thing.
Comment 5 Carlos Garcia Campos 2017-09-06 08:11:06 PDT
Committed r221670: <http://trac.webkit.org/changeset/221670>
Comment 6 Michael Catanzaro 2017-09-06 18:28:03 PDT
This killed the WPE bot :(
Comment 7 Carlos Garcia Campos 2017-09-06 22:02:37 PDT
(In reply to Michael Catanzaro from comment #6)
> This killed the WPE bot :(

hmm, right, they are also using the freetype 2.4.11 patch. As a workaround we could ifdef the code introduced in this patch. Then I suggests someone bumps freetype to 2.8 in wpe as well, removing the patch and ifdefs and then rebaselining. Not fun at all :-(
Comment 8 Carlos Alberto Lopez Perez 2017-09-07 03:42:32 PDT
(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > This killed the WPE bot :(
> 
> hmm, right, they are also using the freetype 2.4.11 patch. As a workaround
> we could ifdef the code introduced in this patch. Then I suggests someone
> bumps freetype to 2.8 in wpe as well, removing the patch and ifdefs and then
> rebaselining. Not fun at all :-(

I think we can do this now, not need to ifdef the code.