Bug 97676 – [Chromium] FontHarfBuzz.cpp should not use drawTextOnPath()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kenichi Ishibashi]

FontHarfBuzz uses drawTextOnPath() to draw vertical text. According to Skia guys, it's slow than drawPosText(). We should use drawPosText() + setVerticalText() instead.

[Kenichi Ishibashi]

Created attachment 165792 [details]
WIP

[Kenichi Ishibashi]

Created attachment 165993 [details]
Patch

[Kenichi Ishibashi]

Hi Tony,

Could you take a look?

Instead using ipafont(c.f. https://bugs.webkit.org/show_bug.cgi?id=97682#c4), I'd like to add a fast path for fonts which have vhea/vmtx table when drawing vertical text.

This patch doesn't change behavior on LayoutTests.

[WebKit Review Bot]

Comment on attachment 165993 [details]
Patch

Attachment 165993 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14001061

New failing tests:
fast/text/international/text-spliced-font.html

[Tony Chang]

Comment on attachment 165993 [details]
Patch

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

The EWS bot failure looks real.

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:100
> +            canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);

Can we use sizeof(GlyphBufferGlyph) instead of hardcoding 2?

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:121
> +            canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);

Ditto.

[WebKit Review Bot]

Comment on attachment 165993 [details]
Patch

Attachment 165993 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14060007

New failing tests:
fast/text/international/text-spliced-font.html

[Kenichi Ishibashi]

Created attachment 166079 [details]
Patch for landing

[Kenichi Ishibashi]

Created attachment 166082 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166082 [details]
Patch for landing

Rejecting attachment 166082 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14038829

[Kenichi Ishibashi]

Comment on attachment 165993 [details]
Patch

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

Thank you for review!

I should have disabled setVerticalText() in drawComplexText() because we don't support vertical text on complex path now. It's will be supported after hb-ng transition.

>> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:100
>> +            canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);
> 
> Can we use sizeof(GlyphBufferGlyph) instead of hardcoding 2?

Done.

>> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:121
>> +            canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint);
> 
> Ditto.

Done.

[WebKit Review Bot]

Comment on attachment 166082 [details]
Patch for landing

Rejecting attachment 166082 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14034956

[Kenichi Ishibashi]

Created attachment 166086 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166086 [details]
Patch for landing

Rejecting attachment 166086 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14066002

[Tony Chang]

(In reply to comment #13)
> (From update of attachment 166086 [details])
> Rejecting attachment 166086 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/14066002

Hmm, we saw the same thing on a different patch.  Maybe a bug in the commit queue?

[Kenichi Ishibashi]

(In reply to comment #14)
> Hmm, we saw the same thing on a different patch.  Maybe a bug in the commit queue?

That might be.. I'll land the patch by hand.

[Adam Barth]

Yes, the bot is having trouble today for some reason.

[Kenichi Ishibashi]

Committed r129823: <http://trac.webkit.org/changeset/129823>

[Ojan Vafai]

This patch caused a bunch of image failures on Chromium Linux. They look like they just need rebaselines. Bashi, mind taking a look?

[Ojan Vafai]

Actually, on closer inspection, the new results look bad to me. The glyphs are oddly compressed and small. I'm going to rollout as I haven't gotten a hold of bashi.

[WebKit Review Bot]

Re-opened since this is blocked by bug 97837

[Kenichi Ishibashi]

I couldn't reproduce the failures on my linux. I need the fonts which are used by chromium canary bots to investigate the failures.

[Ojan Vafai]

Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use?

[Kenichi Ishibashi]

(In reply to comment #22)
> Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use?

Sorry, my previous comment wasn't enough. I believe Linux DRT uses "kochi mincho" and "kochi gothic" for vertical text tests. I'm guessing that kochi mincho font on bots would be different from mine. I need the fonts which are actually used on the bots to investigate the cause.

[Tony Chang]

(In reply to comment #23)
> (In reply to comment #22)
> > Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use?
> 
> Sorry, my previous comment wasn't enough. I believe Linux DRT uses "kochi mincho" and "kochi gothic" for vertical text tests. I'm guessing that kochi mincho font on bots would be different from mine. I need the fonts which are actually used on the bots to investigate the cause.

Are you using Lucid or Precise?  All the bots run Lucid and running install-build-deps.sh should get the right version of the fonts.

Also, I suspect that the failure of fast/text/international/text-spliced-font.html on the EWS bot is real. Ojan, did more than that one test fail?

[Tony Chang]

(In reply to comment #24)

> Ojan, did more than that one test fail?

Nevermind, I found the list of failing tests on the other bug.

[Tony Chang]

It looks like the glyphs are too far to the left and getting cut off:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux/results/layout-test-results/editing/selection/vertical-rl-ltr-extend-line-forward-p-actual.png

[Tony Chang]

I checked on the build.chromium.org bots. They have:
ii  ttf-kochi-gothic                     20030809-6                                      Kochi Subst Gothic Japanese TrueType font wi
ii  ttf-kochi-mincho                     20030809-6                                      Kochi Subst Mincho Japanese TrueType font wi

Which is the same as on my machine.

[Kenichi Ishibashi]

(In reply to comment #27)
> I checked on the build.chromium.org bots. They have:
> ii  ttf-kochi-gothic                     20030809-6                                      Kochi Subst Gothic Japanese TrueType font wi
> ii  ttf-kochi-mincho                     20030809-6                                      Kochi Subst Mincho Japanese TrueType font wi
> 
> Which is the same as on my machine.

I'm using Lucid and the version of ttf-kochi-{gothic,mincho} is the same. Hmm..
I wonder why the failures appears on chromium canary bots but don't appear on EWS bots.

How can I get the skia version the bots are using? Skia guys has improved vertical text rendering (e.g.  r5677)

[Tony Chang]

(In reply to comment #28)
> (In reply to comment #27)
> > I checked on the build.chromium.org bots. They have:
> > ii  ttf-kochi-gothic                     20030809-6                                      Kochi Subst Gothic Japanese TrueType font wi
> > ii  ttf-kochi-mincho                     20030809-6                                      Kochi Subst Mincho Japanese TrueType font wi
> > 
> > Which is the same as on my machine.
> 
> I'm using Lucid and the version of ttf-kochi-{gothic,mincho} is the same. Hmm..
> I wonder why the failures appears on chromium canary bots but don't appear on EWS bots.
> 
> How can I get the skia version the bots are using? Skia guys has improved vertical text rendering (e.g.  r5677)

You can look at the update step on the bots.  E.g., on http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/31607/steps/update/logs/stdio , the skia revision is in this line:

122>________ running 'svn update /mnt/data/b/build/slave/Webkit_Linux/build/src/third_party/skia/gyp --revision 5746 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/data/b/build/slave/Webkit_Linux/build'

The build.chromium.org use a full chromium checkout while the EWS bots use a WebKit only checkout.  Maybe that mattered at the time?  You could try uploading again or landing again to see if things are different now.

[Kenichi Ishibashi]

(In reply to comment #29)
> You can look at the update step on the bots.  E.g., on http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/31607/steps/update/logs/stdio , the skia revision is in this line:
> 
> 122>________ running 'svn update /mnt/data/b/build/slave/Webkit_Linux/build/src/third_party/skia/gyp --revision 5746 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/data/b/build/slave/Webkit_Linux/build'
> 
> The build.chromium.org use a full chromium checkout while the EWS bots use a WebKit only checkout.  Maybe that mattered at the time?  You could try uploading again or landing again to see if things are different now.

Thank you for the information. I used tryserver and confirmed the patch doesn't pass the vertical text tests. I'm investigating the cause.