Bug 174403 – [GTK][FreeType] Enable BCI on webfonts

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

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.

[Alicia Boya García]

Created attachment 315186 [details]
GMail login screen, with BCI enabled

[Alicia Boya García]

Created attachment 315188 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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'.

[Carlos Garcia Campos]

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).

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

I've just run the tests and this doesn't seem to cause any failure :-)

[Adrian Perez]

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.

[Alicia Boya García]

Created attachment 315222 [details]
Patch

[Michael Catanzaro]

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.

[Alicia Boya García]

Created attachment 315256 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Michael Catanzaro]

(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

[WebKit Commit Bot]

Comment on attachment 315256 [details]
Patch

Clearing flags on attachment: 315256

Committed r219422: <http://trac.webkit.org/changeset/219422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.