Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(633)

Issue 1773403002: Update SVG text layout to use shaped glyph data & go fast (O(n^2)->O(n)) (Closed)

Created:
4 years, 9 months ago by pdr.
Modified:
4 years, 9 months ago
Reviewers:
fs
CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jbroman, jchaffraix+rendering, Justin Novosad, kinuko+watch, kouhei+svg_chromium.org, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@1773353003
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Update SVG text layout to use shaped glyph data & go fast (O(n^2)->O(n)) When HarfBuzz shapes a run of text it caches the resulting glyph widths and glyph advances. This patch uses the per-glyph output directly in SVG's text layout which improves both rendering quality and speed. Shaping is the process where glyphs are positioned and sized, including forming combined glyphs such as 'f' and 'i' forming 'fi'. SVG's text layout used an O(n^2) algorithm for calculating glyph positions where an entire run would be shaped for each character, and the differences would be used for glyph widths. This approach is slow and produces poor glyph data. Using the shaper's glyph advances, we get improved positioning (see the 'e' in lengthAdjust-text-metrics.html). By only shaping text once we also get a nice speed improvement. This patch improves the SVGCubics benchmark by 30%. BUG=589525 Committed: https://crrev.com/304ec1544273ed8d62c693da6dd2c63727805cdd Cr-Commit-Position: refs/heads/master@{#380534}

Patch Set 1 #

Patch Set 2 : Minor cleanup of comments and tests #

Total comments: 11

Patch Set 3 : Rebase after dependant patch landed, no other changes. #

Patch Set 4 : ffix ligature bug, address reviewer comments, add rebaseline entries #

Patch Set 5 : Make the grapheme position synthesis loop longer but easier to grok, fix test indentation #

Patch Set 6 : Add a missed rebaseline to test expectations #

Total comments: 2

Patch Set 7 : static_cats #

Patch Set 8 : Fix TestExpectations collision #

Messages

Total messages: 47 (24 generated)
pdr.
Need to wait for https://codereview.chromium.org/1773353003/ to get the test updates, but this is otherwise ready ...
4 years, 9 months ago (2016-03-09 07:05:40 UTC) #6
fs
Yes, I like! LGTM w/ some comments. https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp (right): https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp#newcode48 third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:48: // TODO(pdr): ...
4 years, 9 months ago (2016-03-09 11:31:45 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/80001
4 years, 9 months ago (2016-03-10 04:48:31 UTC) #9
pdr.
Thanks! PTAL Here are the updated tests from the 3rd patchset: https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_rel_ng/194125/layout-test-results/results.html https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/192875/layout-test-results/results.html https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel_ng/186773/layout-test-results/results.html The ...
4 years, 9 months ago (2016-03-10 04:55:15 UTC) #10
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/194395)
4 years, 9 months ago (2016-03-10 05:59:08 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/100001
4 years, 9 months ago (2016-03-10 07:13:14 UTC) #14
pdr.
https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp (right): https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp#newcode143 third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:143: // code below synthesizes a position when two characters ...
4 years, 9 months ago (2016-03-10 07:21:48 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-10 09:25:18 UTC) #17
fs
LGTM w/ uNit https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp (right): https://codereview.chromium.org/1773403002/diff/20001/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp#newcode143 third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:143: // code below synthesizes a position ...
4 years, 9 months ago (2016-03-10 09:55:57 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/120001
4 years, 9 months ago (2016-03-10 17:30:01 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/194657)
4 years, 9 months ago (2016-03-10 20:05:33 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/120001
4 years, 9 months ago (2016-03-10 20:40:15 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/155542)
4 years, 9 months ago (2016-03-10 20:50:29 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/140001
4 years, 9 months ago (2016-03-10 20:56:22 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/194856)
4 years, 9 months ago (2016-03-10 21:52:31 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/140001
4 years, 9 months ago (2016-03-10 22:05:21 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/128945) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, ...
4 years, 9 months ago (2016-03-10 22:13:32 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/140001
4 years, 9 months ago (2016-03-10 23:26:59 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/195025)
4 years, 9 months ago (2016-03-11 00:41:28 UTC) #40
pdr.
It looks like there is an infrastructure issue: https://bugs.chromium.org/p/chromium/issues/detail?id=593891
4 years, 9 months ago (2016-03-11 01:02:42 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773403002/140001
4 years, 9 months ago (2016-03-11 03:39:43 UTC) #43
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 9 months ago (2016-03-11 05:34:51 UTC) #45
commit-bot: I haz the power
4 years, 9 months ago (2016-03-11 05:36:15 UTC) #47
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/304ec1544273ed8d62c693da6dd2c63727805cdd
Cr-Commit-Position: refs/heads/master@{#380534}

Powered by Google App Engine
This is Rietveld 408576698