|Summary:||[GTK][FreeType] Enable BCI on webfonts|
|Product:||WebKit||Reporter:||Alicia Boya García <aboya>|
|Severity:||Normal||CC:||aperez, bugs-noreply, buildbot, cgarcia, commit-queue, mcatanzaro, mmaxfield, mrobinson, svillar|
|Version:||WebKit Nightly Build|
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 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 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 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.