New issue
Advanced search Search tips

Issue 908843 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 28
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Investigate binary size impact of Harfbuzz on Chrome for Android

Project Member Reported by cjgrant@google.com, Nov 27

Issue description

The Harfbuzz roll below caused a 46 KB size increase on Chrome for Android.

https://chromium.googlesource.com/chromium/src/+/c67f53b0b70f33c47159d37f7e59bb44399b0d09

This spawns two questions:

1. Are the AAT changes (which sound Mac related) necessary on Android, and if not, could they be conditionally be disabled?

2. How much of Harfbuzz (and RenderText in general) is used on Android?  VR uses it for basic text rendering, but what else uses it?
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=908843

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3b32b2f8b03dc7d75d63800919336ab61f5d0144211f76f934a2e4d921adcbf6


Bot(s) for this bug's original alert(s):

Android Builder Perf
Owner: drott@chromium.org
Status: Assigned (was: Untriaged)
Assigning to drott@chromium.org because this is the only CL in range:
Roll HarfBuzz to 2.1.1 plus AAT fixes

https://chromium.googlesource.com/external/github.com/harfbuzz/harfbuzz/+log/54d332dd9b0263..ae96c98dfaef3a7

Enables native AAT support in HarfBuzz without relying on CoreText.

Performance improvements compared to Mac OS CoreText implementation
ranging between 3.2x speedup for Hebrew using Lucida Grande up to 12.2x
speedup for Latin with the San Francisco system font. This should
unblock issue 862773 and other AAT performance issues we have seen.

For performance data, see: https://goo.gl/tZJU2y

The following HarfBuzz issues on AAT issues were reported to HarfBuzz
and have been ironed out in the HarfBuzz commit that we're rolling to:
https://github.com/harfbuzz/harfbuzz/issues?q=1263+1264+1278+1303+1305+1331+1342+1348+1343+1393+1405+1406

The rebaselines affect:

* Kerning differences, especially improved kerning with punctuation on
  Windows, unrelated to AAT implementation
* Slight difference in glyph coordinate rounding for AAT fonts
  due to internal computation with subpixel values, but screen placement
  at rounded pixels
* Combining mark placement differences between HarfBuzz AAT and CoreText
  AAT, compare https://github.com/harfbuzz/harfbuzz/issues/1264
* Changes in vertical origin computation in HarfBuzz, unrelated to AAT

IMPORTANT: If HarfBuzz AAT needs to be reverted, consider changing
    if (false) in BUILD.gn to
    if (is_mac)
to re-enable the hb-coretext backend for processing AAT instead of
reverting the whole CL.

Big kudos and a big thank you to behdad@ for the excellent work on
implementing AAT in HarfBuzz and this smooth collaboration in maturing
the implementation.

Binary-Size: Intended functionality add in HarfBuzz to support AAT fonts natively without using CoreText
Bug:  894354 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Iab082fbd47cd1df9e1fd9558a12ddf597db77f7b
Reviewed-on: https://chromium-review.googlesource.com/c/1275945
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610734}
Cc: -cjgrant@google.com
Owner: cjgrant@chromium.org
Claiming this for initial investigation.
Cc: cjgrant@chromium.org
Owner: drott@chromium.org
From a VR perspective, it looks like (at least) GFX' skia canvas and related code uses RenderText as well, so we wouldn't gain anything if VR cut its dependency.

Over to drott@ for input on Harfbuzz's utilization of AAT on Android.  Dominik, could you point out whether AAT benefits Android in any way, and if not, whether there's a switch we can flip to compile it out?
HarfBuzz is used to shape any text you see.

As upstream HarfBuzz maintainer, we don't support building without AAT, since the code is quite entangled with OpenType code, plus, we like to have all HarfBuzz-using clients using a certain version or later to support the same features.  That's one of the biggest advantages to HarfBuzz (fidelity of shaping across Android, Firefox, Chrome), and I suggest thinking twice before changing that.
Cc: e...@chromium.org
HarfBuzz added AAT shaping to vastly improve our performance for text shaping on Mac (3.2 to 12.2x speedup), unblocking issues such as issue 862773. Also, introducing AAT type font support for non Mac platforms enables additional typographic fidelity for example by using the trak AAT table for improved font-size specific letter-spacing.

We have previously attempted to build a custom configuration without AAT support. This has repeatedly lead to painful to debug failures with rolling to a new HarfBuzz version, and - as Behdad explains - is generally not supported by HarfBuzz upstream.

We would also much prefer not to introduce differences in what font technologies we support on our supported platforms.

So, for the additional maintenance burden in maintaining an unsupported build configuration, and for reasons of cross-platform consistency, we'd much prefer keeping the functionality enabled in Android.
Status: WontFix (was: Assigned)
Thanks for the thoughts!  Your points make sense, and it's fine.  Just wanted to be sure.

Sign in to add a comment