Bug 92098 - [GTK] Bring Harfbuzz-ng support to Gtk
Summary: [GTK] Bring Harfbuzz-ng support to Gtk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
: 20783 26741 (view as bug list)
Depends on:
Blocks: 20783 104576
  Show dependency treegraph
 
Reported: 2012-07-24 04:39 PDT by Dominik Röttsches (drott)
Modified: 2012-12-11 01:42 PST (History)
12 users (show)

See Also:


Attachments
Patch (12.96 KB, patch)
2012-10-31 15:22 PDT, Martin Robinson
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-07-24 04:39:35 PDT
As a successor the pango backend, let's enable harfbuzz for gtk to get better text shaping support for complex fonts.
Work to enable this on EFL is tracked in bug 91853.
Comment 1 Grzegorz Czajkowski 2012-08-23 04:22:23 PDT
This bug removes Pango from WebKit-Gtk spelling implementation (https://bugs.webkit.org/show_bug.cgi?id=94320).
Comment 2 Martin Robinson 2012-10-26 17:22:30 PDT
I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly.
Comment 3 Dominik Röttsches (drott) 2012-10-29 01:32:16 PDT
(In reply to comment #2)
> I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly.

Does this change depending on the Harfbuzz version? Looking at their mailing list, there's lots of changes going on.
Comment 4 Martin Robinson 2012-10-29 10:41:42 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly.
> 
> Does this change depending on the Harfbuzz version? Looking at their mailing list, there's lots of changes going on.

I see this with the latest harfbuzz release (0.9.5).
Comment 5 Martin Robinson 2012-10-30 13:34:54 PDT
(In reply to comment #4)

> I see this with the latest harfbuzz release (0.9.5).

The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs.

I guess we should be choosing the appropriate fallback font earlier in the process.

bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path?
Comment 6 Kenichi Ishibashi 2012-10-30 19:08:43 PDT
Adding behdad.
(In reply to comment #5)
> (In reply to comment #4)
> 
> > I see this with the latest harfbuzz release (0.9.5).
> 
> The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs.
> 
> I guess we should be choosing the appropriate fallback font earlier in the process.
> 
> bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path?

(In reply to comment #5)
> (In reply to comment #4)
> 
> > I see this with the latest harfbuzz release (0.9.5).
> 
> The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs.
> 
> I guess we should be choosing the appropriate fallback font earlier in the process.
> 
> bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path?

Are you trying to use HarfBuzzShaper on GTK? If so, HarfBuzzShaper::collectHarfBuzzRuns() doesn't select fonts properly?
Comment 7 Martin Robinson 2012-10-31 12:40:51 PDT
Mystery solved (I hope). When the Fontconfig FontCache returns a fallback font, it returns a different font each time -- even if the Fontconfig pattern returns the same font. This is quite bad, as it means that we're keeping many duplicate copies of fallback fonts in the cache. I've fixed this locally and the results look a lot better.

The reason this was causing a problem for Harfbuzz was that HarfBuzzShaper saw each fallback font as a different font and created a separate text run for each character.
Comment 8 Martin Robinson 2012-10-31 15:22:36 PDT
Created attachment 171729 [details]
Patch
Comment 9 Martin Robinson 2012-10-31 15:23:03 PDT
I didn't include the new layout test results because the made the patch a bit too big.
Comment 10 Martin Robinson 2012-10-31 16:27:50 PDT
It looks like the bot is missing ICU pkg-config files.
Comment 11 Philippe Normand 2012-11-01 00:30:56 PDT
(In reply to comment #10)
> It looks like the bot is missing ICU pkg-config files.

The bot has libicu-dev 4.8.1.1-9. Seems like the harfbuzz headers are not found instead when building WebCore.

Is a clean build required?
Comment 12 Dominik Röttsches (drott) 2012-11-01 01:29:34 PDT
(In reply to comment #9)
> I didn't include the new layout test results because the made the patch a bit too big.

Glad you found the reason. You may ping our gardener on IRC once you land this, so that we can take care of the EFL rebaselining. http://trac.webkit.org/wiki/EFLWebKitBuildBots
Comment 13 Martin Robinson 2012-11-01 05:53:50 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > It looks like the bot is missing ICU pkg-config files.
> 
> The bot has libicu-dev 4.8.1.1-9. Seems like the harfbuzz headers are not found instead when building WebCore.
> 
> Is a clean build required?

In Debian, up until very recently, the libicu-dev package did not install pkg-config files. See here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=687339 I'm not sure what the best way to resolve this is. Locally I install libicu manually.
Comment 14 Dominik Röttsches (drott) 2012-11-06 06:11:08 PST
(In reply to comment #13)
> I'm not sure what the best way to resolve this is.

I'm proposing
http://lists.freedesktop.org/archives/harfbuzz/2012-November/002612.html

Since we need to bump the version in bug 101323 as well.
Comment 15 Gustavo Noronha (kov) 2012-12-09 04:53:55 PST
Comment on attachment 171729 [details]
Patch

LGTM, and I think the bots have all been updated some time ago, haven't they?
Comment 16 Martin Robinson 2012-12-10 06:51:49 PST
Committed r137146: <http://trac.webkit.org/changeset/137146>
Comment 17 Chris Dumez 2012-12-10 09:18:56 PST
Looks life this patch affected the results for EFL port?

About 50 test cases seem to require rebaselining after this patch? Was that expected?
Comment 18 Chris Dumez 2012-12-10 09:20:01 PST
Ok, I see from https://bugs.webkit.org/show_bug.cgi?id=92098#c12 that the rebaseline was expected. I guess I'll take care of it then.
Comment 19 Martin Robinson 2012-12-11 01:36:05 PST
*** Bug 26741 has been marked as a duplicate of this bug. ***
Comment 20 Martin Robinson 2012-12-11 01:42:32 PST
*** Bug 20783 has been marked as a duplicate of this bug. ***