Bug 97676

Summary: [Chromium] FontHarfBuzz.cpp should not use drawTextOnPath()
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: PlatformAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, agl, dglazkov, ojan, schenney, senorblanco, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 97682, 97837    
Bug Blocks: 62841    
Attachments:
Description Flags
WIP
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing webkit.review.bot: commit-queue-

Description Kenichi Ishibashi 2012-09-26 06:37:57 PDT
FontHarfBuzz uses drawTextOnPath() to draw vertical text. According to Skia guys, it's slow than drawPosText(). We should use drawPosText() + setVerticalText() instead.
Comment 1 Kenichi Ishibashi 2012-09-26 06:58:49 PDT
Created attachment 165792 [details]
WIP
Comment 2 Kenichi Ishibashi 2012-09-27 07:08:27 PDT
Created attachment 165993 [details]
Patch
Comment 3 Kenichi Ishibashi 2012-09-27 07:12:58 PDT
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.
Comment 4 WebKit Review Bot 2012-09-27 13:26:48 PDT
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
Comment 5 Tony Chang 2012-09-27 13:35:20 PDT
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.
Comment 6 WebKit Review Bot 2012-09-27 14:06:01 PDT
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
Comment 7 Kenichi Ishibashi 2012-09-27 15:42:01 PDT
Created attachment 166079 [details]
Patch for landing
Comment 8 Kenichi Ishibashi 2012-09-27 15:44:50 PDT
Created attachment 166082 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-09-27 15:47:48 PDT
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
Comment 10 Kenichi Ishibashi 2012-09-27 15:49:58 PDT
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.
Comment 11 WebKit Review Bot 2012-09-27 15:56:06 PDT
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
Comment 12 Kenichi Ishibashi 2012-09-27 16:06:20 PDT
Created attachment 166086 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-09-27 16:51:30 PDT
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
Comment 14 Tony Chang 2012-09-27 16:53:07 PDT
(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?
Comment 15 Kenichi Ishibashi 2012-09-27 16:59:36 PDT
(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.
Comment 16 Adam Barth 2012-09-27 17:01:25 PDT
Yes, the bot is having trouble today for some reason.
Comment 17 Kenichi Ishibashi 2012-09-27 17:13:32 PDT
Committed r129823: <http://trac.webkit.org/changeset/129823>
Comment 18 Ojan Vafai 2012-09-27 18:04:37 PDT
This patch caused a bunch of image failures on Chromium Linux. They look like they just need rebaselines. Bashi, mind taking a look?
Comment 19 Ojan Vafai 2012-09-27 18:08:58 PDT
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.
Comment 20 WebKit Review Bot 2012-09-27 18:10:46 PDT
Re-opened since this is blocked by bug 97837
Comment 21 Kenichi Ishibashi 2012-09-27 19:23:26 PDT
I couldn't reproduce the failures on my linux. I need the fonts which are used by chromium canary bots to investigate the failures.
Comment 22 Ojan Vafai 2012-09-27 23:05:54 PDT
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?
Comment 23 Kenichi Ishibashi 2012-09-28 02:12:09 PDT
(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.
Comment 24 Tony Chang 2012-09-28 10:18:26 PDT
(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?
Comment 25 Tony Chang 2012-09-28 10:19:24 PDT
(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.
Comment 27 Tony Chang 2012-09-28 10:32:09 PDT
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.
Comment 28 Kenichi Ishibashi 2012-09-28 19:49:39 PDT
(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)
Comment 29 Tony Chang 2012-10-01 09:35:58 PDT
(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.
Comment 30 Kenichi Ishibashi 2012-10-01 20:53:50 PDT
(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.