Bug 174403

Summary: [GTK][FreeType] Enable BCI on webfonts
Product: WebKit Reporter: Alicia Boya García <aboya>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, buildbot, cgarcia, commit-queue, mcatanzaro, mmaxfield, mrobinson, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=140994
Attachments:
Description Flags
GMail login screen, with BCI disabled
none
GMail login screen, with BCI enabled
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-cq-02 for mac-elcapitan none

Description Alicia Boya García 2017-07-11 16:19:18 PDT
Created attachment 315185 [details]
GMail login screen, with BCI disabled

Initially embedded font hints were ignored for webfonts because of several
reasons:

* Other browsers ignored them too. This is no longer true; e.g. Chrome respects
  them.

* The FreeType hint interpreter used produced to produce bad results for many
  hinted fonts, but the situation has improved recently.
  https://www.freetype.org/freetype2/docs/subpixel-hinting.html

* Enabling embedded hints for webfonts exposes the FreeType byte code
  interpreter to potentially-malicious input. This one is still true, of
  course.

Given most of the past reasons are no longer valid, except for the last, I
think it's time to reconsider the issue. 

Enabling the BCI gets us better font rendering in most cases and makes it more
consistent with system fonts. See the attached screenshots which were taken
within the jhbuild minibrowser. Look for the letter 't'.

On the other hand, like with so many other features, it's undeniable that
enabling this we're widening our attack surface, even if it was already quite
big.
Comment 1 Alicia Boya García 2017-07-11 16:20:11 PDT
Created attachment 315186 [details]
GMail login screen, with BCI enabled
Comment 2 Alicia Boya García 2017-07-11 16:38:49 PDT
Created attachment 315188 [details]
Patch
Comment 3 Michael Catanzaro 2017-07-11 19:38:51 PDT
We've discussed this fairly extensively internally, and consensus seems to be that we should try this again now. The problems from bug #140994 seem to have disappeared, probably due to changes in FreeType.

I also discussed this extensively with Behdad Esfahbod at the Web Engines Hackfest last year. He seemed to think that either approach is reasonable.

Be sure to set the cq? flag (or use 'webkit-patch --request-commit) if you want your patches to be committed. I could give cq+ myself now, but I think now is a bad time to land this as it could break tests and I'm not going to be watching the bots later tonight. So let's try landing tomorrow instead.
Comment 4 Michael Catanzaro 2017-07-11 19:40:13 PDT
Comment on attachment 315188 [details]
Patch

Oh, well there is only one problem: no ChangeLog entry! You will have to upload one before this can be committed. (I see from our internal chat that you've already found the prepare-ChangeLog script.) Other than that, this is perfect.

Tomorrow we'll find out if this requires any tests to be rebaselined.
Comment 5 Michael Catanzaro 2017-07-11 19:41:37 PDT
Also, a tip: since I've given r+, you don't have to request review again if you use 'webkit-patch apply-from-bug' to download the patch with my name already listed as reviewer, and then upload the new version using 'webkit-patch upload --no-review --request-commit'.
Comment 6 Carlos Garcia Campos 2017-07-11 23:45:28 PDT
We should run the tests before landing this, and prepare a rebaseline if needed as part of this patch or in a follow up (we might need to split the rebaseline if it's too large). If this is going to cause a lot of failures, maybe it's a good idea to also bump freetype (and even fontconfig too) version in the same commit to have a single rebaseline (even if split).
Comment 7 Carlos Garcia Campos 2017-07-12 00:33:49 PDT
Comment on attachment 315188 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315188&action=review

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:38
> +    : m_fontFace(cairo_ft_font_face_create_for_ft_face(freeTypeFace, 0))

I think FT_LOAD_DEFAULT is 0 and it's more descriptive.
Comment 8 Carlos Garcia Campos 2017-07-12 01:26:16 PDT
I've just run the tests and this doesn't seem to cause any failure :-)
Comment 9 Adrian Perez 2017-07-12 02:07:43 PDT
As discussed by chat, I also agree we should be enabling the BCI nowadays.
Just a comment below, for the sake of completeness...

(In reply to Michael Catanzaro from comment #3)

> We've discussed this fairly extensively internally, and consensus seems to
> be that we should try this again now. The problems from bug #140994 seem to
> have disappeared, probably due to changes in FreeType.

In particular, FreeType 2.7 introduced a new subpixel hinting mode, which
is similar to “emulating a modern version of ClearType”. This is one of the
changes (probably the main one) to font rendering that makes us now obtain
better results with the BCI enabled. Full 2.7 release notes, for reference:

  https://sourceforge.net/projects/freetype/files/freetype2/2.7/

The following document is also very interesting to better understand the
whole story of BCI-hinting vs. autohinting in FreeType:

  https://www.freetype.org/freetype2/docs/text-rendering-general.html

The TL;DR is:

  - Use always “slight” hinting. It uses the BCI when a font contains
    hints natively, and falls back to the autohinter if not. And always
    uses subpixel hinting from FreeType 2.7 onwards. BTW, “slight” is
    already the default setting when using GNOME.

  - The new default LCD filter causes less color fringes. As long as we
    use FreeType 2.6.2 or newer with the default filter settings, nothing
    else needs to be done.

  - The output from FreeType rendering is a coverage map (linear: 50% gray
    means a glyph covers it 50%), and not the final value of pixels. It has
    to be blended and gamma-corrected when painting (screens are non-linear:
    a 50% gray has to be painted darker than 50% gray!). Toolkits and graphics
    libraries rarely support this out of the box, so there's nothing we can
    do about this.
Comment 10 Alicia Boya García 2017-07-12 02:49:11 PDT
Created attachment 315222 [details]
Patch
Comment 11 Michael Catanzaro 2017-07-12 08:22:13 PDT
Comment on attachment 315222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315222&action=review

> Source/WebCore/ChangeLog:14
> +        Reviewed by NOBODY (OOPS!).

Ah, just one last problem: this needs to go above the description of the change, not below it.
Comment 12 Alicia Boya García 2017-07-12 10:22:07 PDT
Created attachment 315256 [details]
Patch
Comment 13 WebKit Commit Bot 2017-07-12 11:52:35 PDT
Comment on attachment 315256 [details]
Patch

Rejecting attachment 315256 [details] from commit-queue.

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html
Full output: http://webkit-queues.webkit.org/results/4108618
Comment 14 WebKit Commit Bot 2017-07-12 11:52:37 PDT
Created attachment 315264 [details]
Archive of layout-test-results from webkit-cq-02 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 15 Michael Catanzaro 2017-07-12 13:22:27 PDT
(In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 315256 [details]
> Patch
> 
> Rejecting attachment 315256 [details] from commit-queue.
> 
> New failing tests:
> storage/indexeddb/modern/new-database-after-user-delete.html
> Full output: http://webkit-queues.webkit.org/results/4108618

Reported bug #174433
Comment 16 WebKit Commit Bot 2017-07-12 13:44:44 PDT
Comment on attachment 315256 [details]
Patch

Clearing flags on attachment: 315256

Committed r219422: <http://trac.webkit.org/changeset/219422>
Comment 17 WebKit Commit Bot 2017-07-12 13:44:46 PDT
All reviewed patches have been landed.  Closing bug.