Bug 177601 – [Freetype] Simple and complex paths are not applied consistently

• Component: Text
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Created attachment 322075 [details]
Simple test

Due to bug #100050, when rendering text, the complex path is forced in case kerning or shaping is enabled and only part of the run is going to be rendered. This happens in the GTK+ port when selecting text (except when selecting the whole run, of course). The text is initially rendered using the simple path as returned by FontCascade::codePath() and then the selection is rendered using the complex path, overriding what FontCascade::codePath() returns in that case. This is very easy to reproduce with the attached test, you can see the effects in this video:

http://people.igalia.com/cgarcia/wk-text-selection.webm
http://people.igalia.com/cgarcia/wk-text-selection.mp4

This doesn't happen in mac, because the selection is rendered differently, so FontCascade::drawText always renders the full run (simple path) when selecting text. This is the most noticeable inconsistency, but it's not the only one. Similar exceptions are applied when calculating the text width, or getting the offset of a given position. The rendered text is the simple one, but the calculations are performed using the complex path, so depending on the kerning and ligatures we might end up with wrong results.

If the test has been rendered using the simple path, the selections and all other calculations should be performed with the simple path too.

[Carlos Garcia Campos]

Btw, this doesn't happen with WebKitGTK+ 2.18, it happens only in trunk since we enabled the complex text for the emojis support.

[Carlos Garcia Campos]

Created attachment 322076 [details]
Patch

[Michael Catanzaro]

Awesome!

I fear EWS is about to spit several Mac test failures at you, though.

[Build Bot]

Comment on attachment 322076 [details]
Patch

Attachment 322076 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4687093

New failing tests:
imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html
fast/inline/padding-ellipsis-right.html
svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg
platform/mac/fast/text/ligature-subdivision.html
svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg

[Build Bot]

Created attachment 322077 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322076 [details]
Patch

Attachment 322076 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4687105

New failing tests:
svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg
imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html
svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg
fast/inline/padding-ellipsis-right.html
svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg
platform/mac/fast/text/ligature-subdivision.html
svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg

[Build Bot]

Created attachment 322078 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322076 [details]
Patch

Attachment 322076 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4687130

New failing tests:
imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html
fast/inline/padding-ellipsis-right.html

[Build Bot]

Created attachment 322081 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Build Bot]

Comment on attachment 322076 [details]
Patch

Attachment 322076 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4687145

New failing tests:
imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html
fast/inline/padding-ellipsis-right.html
svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg
svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg
svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg
platform/mac/fast/text/ligature-subdivision.html
svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg

[Build Bot]

Created attachment 322082 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Awesome!
> 
> I fear EWS is about to spit several Mac test failures at you, though.

Yes, there are also failures in GTK+, most of them (if not all) just need to be rebaselined, but I want to get feedback/review first before doing the rebaseline.

[Carlos Garcia Campos]

(In reply to Build Bot from comment #4)
> Comment on attachment 322076 [details]
> Patch
> 
> Attachment 322076 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4687093
> 
> New failing tests:
> imported/w3c/web-platform-tests/css/css-ui-3/text-overflow-013.html
> fast/inline/padding-ellipsis-right.html
> svg/text/select-textLength-spacingAndGlyphs-stretch-3.svg
> svg/text/select-textLength-spacingAndGlyphs-squeeze-3.svg
> svg/text/select-textLength-spacingAndGlyphs-stretch-2.svg
> svg/text/select-textLength-spacingAndGlyphs-squeeze-1.svg
> svg/text/select-textLength-spacingAndGlyphs-stretch-1.svg
> svg/text/select-textLength-spacingAndGlyphs-squeeze-4.svg
> svg/text/select-textLength-spacingAndGlyphs-stretch-4.svg
> platform/mac/fast/text/ligature-subdivision.html
> svg/text/select-textLength-spacingAndGlyphs-squeeze-2.svg

Myles, could it be that these tests expect the partial run computations to be done by the complex text path? the non-svg tests pass by running WTR with --complex-text option and the svg ones fail but pass ( the right thing is selected,  they would need to be rebaselined). Actually I would say the svg tests are indeed a proof of this bug, the computation match when using --complex-text but the rendering doesn't.

[Carlos Garcia Campos]

Btw, don't pay attention to the image diffs of the svg test failures, because the pixel expectations are outdated.

[Carlos Garcia Campos]

Myles, could you review or comment about this patch? I have to make our first "preview" release of this cycle from trunk next week and selections are completely broken for us due to this bug. If this behavior is not desired for mac (I think it is), I can simply add GTK and WPE ifdefs keeping the current behavior in mac. Or I can do that temporarily until we figure out the test failures. If those tests are actually expected to be run with complex path enabled I can try to add a way to enable complex text for particular tests in WTR/DRT. Or I can simply mark them as expected failures and file a new bug report.

[Myles C. Maxfield]

I'm pretty busy today but I'll try to review it in the evening.

[Carlos Garcia Campos]

Ping. Myles, if you don't have time to look at this now, I can simply make this change harfbuzz only with a FIXME and then you can decide whether to adopt this in mac too when you find the time, because we definitely want (and need) this change (unless it's wrong for some reason).

[Myles C. Maxfield]

Comment on attachment 322076 [details]
Patch

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

How sure are you that there isn't a bug with your fast text codepath? Can you explain why we ever opt into the fast text codepath for a situation where we aren't sure that it would get the same results as the complex path?

> Source/WebCore/ChangeLog:20
> +        Due to bug #100050, when rendering text, the complex path is forced in case kerning or shaping is enabled and
> +        only part of the run is going to be rendered. This happens in the GTK+ port when selecting text (except when
> +        selecting the whole run, of course). The text is initially rendered using the simple path as returned by
> +        FontCascade::codePath() and then the selection is rendered using the complex path, overriding what
> +        FontCascade::codePath() returned in that case. This doesn't happen in mac, because the selection is rendered
> +        differently, so FontCascade::drawText always renders the full run (simple path) when selecting text. Selecting
> +        text is the most noticeable inconsistency, but it's not the only one. Similar exceptions are applied when
> +        calculating the text width, or getting the offset of a given position. The rendered text is the simple one, but
> +        the calculations are performed using the complex path, so depending on the kerning and ligatures we might end up
> +        with wrong results. If the text has been rendered using the simple path, the selections and all other
> +        calculations should be performed with the simple path too. This patch moves the condition to force complex text
> +        to FontCascade::codePath(), and only for ports not enabling advance text rendering mode by default. This ensures
> +        that all callers to FontCascade::codePath() will get a consistent result.

I'm not sure this is the correct approach. The source of truth is the complex text codepath; if the fast one gives results which are not the same as the complex path's results, then the fast path is wrong. It sounds like there's just a bug in your simple text codepath.

[Michael Catanzaro]

I think the fast and complex paths cannot, in general, produce matching results due to bug #167557.

[Michael Catanzaro]

Maybe more bugs too, of course :)

[Myles C. Maxfield]

(In reply to Michael Catanzaro from comment #19)
> I think the fast and complex paths cannot, in general, produce matching
> results due to bug #167557.

In that case, this fix should be Freetype-only.

I really want to get rid of one of our text codepaths.... then this wouldn't be an issue.... sigh.

[Carlos Garcia Campos]

Yes, the thing is that simple path doesn't support kerning and ligatures, so we don't get the same results in terms of size and position, as you can see in the screenshot I took. What we want is to ensure that the same conditions applied to decide whether a full run should go complex or simple is applied to a partial run, because in our case selections are always partial runs (except when selecting entire lines). I'll make this GTK/WPE only then.

[Michael Catanzaro]

Maybe it no longer makes sense for GTK/WPE to support the simple path. What Myles suggested we really need are performance tests to figure out if we still need the simple path.

Now, I have no clue, but I suppose it's probably not possible to add ligature and kerning support to our simple path? If it is possible, then that would be the proper fix. In the meantime, it's a real shame. We get complaints about poor kerning not irregularly.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #23)
> If it is possible, then that would be the proper fix.

I don't mean to suggest that you should not also make this change. I guess using #if USE(FREETYPE) conditions would be best.

[Myles C. Maxfield]

(In reply to Michael Catanzaro from comment #23)
> Maybe it no longer makes sense for GTK/WPE to support the simple path. What
> Myles suggested we really need are performance tests to figure out if we
> still need the simple path.
> 
> Now, I have no clue, but I suppose it's probably not possible to add
> ligature and kerning support to our simple path? If it is possible, then
> that would be the proper fix. In the meantime, it's a real shame. We get
> complaints about poor kerning not irregularly.

I don’t know if there is API for it in HarfBuzz but, even if there isn’t, I’d be willing to bet the HarfBuzz community would accept a patch to add such an API.

I would be very interested to hear any performance numbers you gather from disabling the fast text codepath.

[Myles C. Maxfield]

(In reply to Michael Catanzaro from comment #24)
> (In reply to Michael Catanzaro from comment #23)
> > If it is possible, then that would be the proper fix.
> 
> I don't mean to suggest that you should not also make this change. I guess
> using #if USE(FREETYPE) conditions would be best.

You could also make the “figure out which codepath to use” function platform-specific. Might be cleaner than spraying macros everywhere.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #25)
> (In reply to Michael Catanzaro from comment #23)
> > Maybe it no longer makes sense for GTK/WPE to support the simple path. What
> > Myles suggested we really need are performance tests to figure out if we
> > still need the simple path.
> > 
> > Now, I have no clue, but I suppose it's probably not possible to add
> > ligature and kerning support to our simple path? If it is possible, then
> > that would be the proper fix. In the meantime, it's a real shame. We get
> > complaints about poor kerning not irregularly.
> 
> I don’t know if there is API for it in HarfBuzz but, even if there isn’t,
> I’d be willing to bet the HarfBuzz community would accept a patch to add
> such an API.
> 
> I would be very interested to hear any performance numbers you gather from
> disabling the fast text codepath.

Chrome uses HarfBuzz and I believe they have deleted their fast text codepath.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #23)
> Maybe it no longer makes sense for GTK/WPE to support the simple path. What
> Myles suggested we really need are performance tests to figure out if we
> still need the simple path.

Exactly, we need a way to measure performance to make a decision.

> Now, I have no clue, but I suppose it's probably not possible to add
> ligature and kerning support to our simple path? If it is possible, then
> that would be the proper fix. In the meantime, it's a real shame. We get
> complaints about poor kerning not irregularly.

I have no idea how to do that.

[Carlos Garcia Campos]

(In reply to Myles C. Maxfield from comment #27)
> (In reply to Myles C. Maxfield from comment #25)
> > (In reply to Michael Catanzaro from comment #23)
> > > Maybe it no longer makes sense for GTK/WPE to support the simple path. What
> > > Myles suggested we really need are performance tests to figure out if we
> > > still need the simple path.
> > > 
> > > Now, I have no clue, but I suppose it's probably not possible to add
> > > ligature and kerning support to our simple path? If it is possible, then
> > > that would be the proper fix. In the meantime, it's a real shame. We get
> > > complaints about poor kerning not irregularly.
> > 
> > I don’t know if there is API for it in HarfBuzz but, even if there isn’t,
> > I’d be willing to bet the HarfBuzz community would accept a patch to add
> > such an API.
> > 
> > I would be very interested to hear any performance numbers you gather from
> > disabling the fast text codepath.
> 
> Chrome uses HarfBuzz and I believe they have deleted their fast text
> codepath.

And pango, which is what GTK+ uses for text shaping and rendering, uses harfbuzz unconditionally too.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #24)
> (In reply to Michael Catanzaro from comment #23)
> > If it is possible, then that would be the proper fix.
> 
> I don't mean to suggest that you should not also make this change. I guess
> using #if USE(FREETYPE) conditions would be best.

Yes, let's fix this for 2.19.1, then we can measure and decide.

[Carlos Garcia Campos]

Created attachment 325335 [details]
Patch

[Michael Catanzaro]

Comment on attachment 325335 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Freetype] Simple and complex paths are not applied consistently

FreeType!

[Carlos Garcia Campos]

Committed r224223: <https://trac.webkit.org/changeset/224223>

[Radar WebKit Bug Importer]

<rdar://problem/35569019>