Re-land: RenderTextHarfBuzz: Set font render parameters in font data functions

Re-land: RenderTextHarfBuzz: Set font render parameters in font data functions

Originally landed at: https://codereview.chromium.org/480533002/

- Properly pass font render settings to Skia font data functions. Otherwise Skia always returns rounded values.

- If subpixel positioning is off, round the glyph positions to match Pango's rounding logic.

BUG=402715, 402374, 402347
TEST=On Linux, character positions in UI text (address bar, infobars, tab titles) should be identical with --enable-harfbuzz-rendertext and --disable-harfbuzz-rendertext.

Committed: https://crrev.com/04f49f48a13c337cb9eaef0146ec4f15ed91984a
Cr-Commit-Position: refs/heads/master@{#291767}

[ckocagil, 2014-08-19 04:42:52]

So I failed to realize that the linker error was legit and managed to get that
one reverted. whoops.

We lack a GetFontRenderParams() on Mac. Daniel, can you or someone write a
font_render_params_mac.cc for us? I was going to copy/paste the Android
implementation but I don't know what defaults would make sense on Mac.

[Daniel Erat, 2014-08-19 14:24:17]

On 2014/08/19 04:42:52, ckocagil wrote:
> So I failed to realize that the linker error was legit and managed to get that
> one reverted. whoops.
> 
> We lack a GetFontRenderParams() on Mac. Daniel, can you or someone write a
> font_render_params_mac.cc for us? I was going to copy/paste the Android
> implementation but I don't know what defaults would make sense on Mac.

i have no idea which defaults would be used on os x (or how to load the user's
config there) -- i only wrote the linux/chrome os version of FontRenderParams; i
believe that wangxianzhu@ added android and msw@ added windows.

does everything get drawn using RenderTextMac right now, or does much of the
text in the UI get drawn using something else? RenderTextMac looks like it uses
SkiaTextRenderer without calling SetFontRenderParams, so if you're trying to
match the existing behavior you could use a simple implementation (like the
android one) that just returns values that match SkiaTextRenderer's defaults.

[ckocagil, 2014-08-19 15:52:12]

On 2014/08/19 14:24:17, Daniel Erat wrote:
> On 2014/08/19 04:42:52, ckocagil wrote:
> > So I failed to realize that the linker error was legit and managed to get
that
> > one reverted. whoops.
> > 
> > We lack a GetFontRenderParams() on Mac. Daniel, can you or someone write a
> > font_render_params_mac.cc for us? I was going to copy/paste the Android
> > implementation but I don't know what defaults would make sense on Mac.
> 
> i have no idea which defaults would be used on os x (or how to load the user's
> config there) -- i only wrote the linux/chrome os version of FontRenderParams;
i
> believe that wangxianzhu@ added android and msw@ added windows.
> 
> does everything get drawn using RenderTextMac right now, or does much of the
> text in the UI get drawn using something else? RenderTextMac looks like it
uses
> SkiaTextRenderer without calling SetFontRenderParams, so if you're trying to
> match the existing behavior you could use a simple implementation (like the
> android one) that just returns values that match SkiaTextRenderer's defaults.

I don't think we even use views on Mac, what's the status of that project
anyway? And AFAIK there's no important non-views client of RenderText.

Done, PTAL.

[Daniel Erat, 2014-08-19 15:59:06]

lgtm

but do we need to support this at all? what breaks if we don't compile
RenderText for mac?

https://codereview.chromium.org/484883003/diff/40001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_mac.cc (right):

https://codereview.chromium.org/484883003/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_mac.cc:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
nit: remove (c)

https://codereview.chromium.org/484883003/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_mac.cc:34: CR_DEFINE_STATIC_LOCAL(
nit: i think it looked like this line can be unwrapped in the android file (and
thus, here) when i checked earlier

[ckocagil, 2014-08-19 16:12:56]

On 2014/08/19 15:59:06, Daniel Erat wrote:
> lgtm
> 
> but do we need to support this at all? what breaks if we don't compile
> RenderText for mac?

It turns out that yes, we do need to support RenderTextMac, since there's an
ongoing effort to enable toolkit-views on Mac:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/x1Vq02Z1c8c

[msw, 2014-08-19 17:26:22]

LGTM with a nit.

https://codereview.chromium.org/484883003/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/484883003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:887: #if defined(OS_WIN) || defined(OS_LINUX)
nit: Is this guard still needed now?

[ckocagil, 2014-08-19 17:34:25]

https://codereview.chromium.org/484883003/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/484883003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:887: #if defined(OS_WIN) || defined(OS_LINUX)
On 2014/08/19 17:26:21, msw wrote:
> nit: Is this guard still needed now?

Nope. Removed!

[ckocagil, 2014-08-19 17:34:30]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-19 17:35:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/484883003/80001

[ckocagil, 2014-08-19 18:19:31]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2014-08-19 21:39:53]

This actually breaks glyph positioning on Windows. I'll investigate.

[ckocagil, 2014-08-20 02:20:32]

+bungeman: could you help us out with Skia text rendering?

When subpixel positioning is enabled on Windows we get incorrect glyph
placements. I tracked this down to the following: Skia always assumes a 4x
subpixel resolution. To round the position to 1/4th of a pixel, it first adds
1/8, but D1G_RectClip floors it to a whole integer (instead of 1/4).

So we end up with a weirdly rounded integer: "x = floor(x + 0.125f)".

Is this an error? Or are we not supposed to enable subpixel positioning on
Windows, perhaps until it is implemented?

[bungeman-skia, 2014-08-20 13:45:15]

On 2014/08/20 02:20:32, ckocagil wrote:
> +bungeman: could you help us out with Skia text rendering?
> 
> When subpixel positioning is enabled on Windows we get incorrect glyph
> placements. I tracked this down to the following: Skia always assumes a 4x
> subpixel resolution. To round the position to 1/4th of a pixel, it first adds
> 1/8, but D1G_RectClip floors it to a whole integer (instead of 1/4).
> 
> So we end up with a weirdly rounded integer: "x = floor(x + 0.125f)".
> 
> Is this an error? Or are we not supposed to enable subpixel positioning on
> Windows, perhaps until it is implemented?

Half of the sampling frequency is added beforehand so that the subsequent code
can round by using floor. All glyph masks are on pixel boundaries (this is what
you are seeing in SkDraw1Glyph::Proc), and the subpixel part is used to select
which mask to use (the SkDrawCacheProc). Your report of 'incorrect glyph
placement' is a little vague, do you have an example? If you're using
DirectWrite, it's possible that if you request subpixel but the font doesn't
actually support it for rendering (it has embedded bitmaps at that size and you
requested embedded bitmaps). However, I've never actually ensured that the UI is
using DirectWrite and not GDI, perhaps I'll take a look today (note that GDI
doesn't do subpixel since it always hints, but if you ask for no hinting Skia
will draw at subpixel from outlines).

[bungeman-skia, 2014-08-20 14:46:50]

On 2014/08/20 13:45:15, bungeman1 wrote:
> On 2014/08/20 02:20:32, ckocagil wrote:
> > +bungeman: could you help us out with Skia text rendering?
> > 
> > When subpixel positioning is enabled on Windows we get incorrect glyph
> > placements. I tracked this down to the following: Skia always assumes a 4x
> > subpixel resolution. To round the position to 1/4th of a pixel, it first
adds
> > 1/8, but D1G_RectClip floors it to a whole integer (instead of 1/4).
> > 
> > So we end up with a weirdly rounded integer: "x = floor(x + 0.125f)".
> > 
> > Is this an error? Or are we not supposed to enable subpixel positioning on
> > Windows, perhaps until it is implemented?
> 
> Half of the sampling frequency is added beforehand so that the subsequent code
> can round by using floor. All glyph masks are on pixel boundaries (this is
what
> you are seeing in SkDraw1Glyph::Proc), and the subpixel part is used to select
> which mask to use (the SkDrawCacheProc). Your report of 'incorrect glyph
> placement' is a little vague, do you have an example? If you're using
> DirectWrite, it's possible that if you request subpixel but the font doesn't
> actually support it for rendering (it has embedded bitmaps at that size and
you
> requested embedded bitmaps). However, I've never actually ensured that the UI
is
> using DirectWrite and not GDI, perhaps I'll take a look today (note that GDI
> doesn't do subpixel since it always hints, but if you ask for no hinting Skia
> will draw at subpixel from outlines).

So after taking a look, the UI is still using GDI, so that probably explains
what you're seeing. This can be seen trivially by setting
SK_SHOW_TEXT_BLIT_COVERAGE to 1 in SkFontHost_win.cpp. This will cause any
transparent areas in a glyph drawn from GDI to be a minimum of 10. Such text
will show the bounding rectangle faintly (in the text color) behind the glyph.

The issue here is with <chromium>/src/skia/ext/fontmgr_default_win.cc introduced
with https://codereview.chromium.org/344653002 . You may need to do some work to
either modify that file, create an SkFontMgr in the browser process and set it
as the default by calling that files SetDefaultSkiaFactory similar to the
renderer process, or just create an SkFontMgr in the browser process for gfx and
use it directly. Note that currently the SkFontMgr on Linux is rather limited in
what it actually implements, but I'm currently working on that.

The short of it is that if you want subpixel positioned glyphs in the UI, first
you need to get the UI on DirectWrite when available since GDI does not support
subpixel positioned glyphs in general.

[tomhudson, 2014-08-20 16:14:35]

On 2014/08/20 02:20:32, ckocagil wrote:
> Is this an error? Or are we not supposed to enable subpixel positioning on
> Windows, perhaps until it is implemented?

I believe there's no error here. Trying to understand the Skia code myself, I
wrote
https://docs.google.com/document/d/185r16wsCMqmGxThN0NDV414PRToN02IHWkBe2QS0i...;
please comment there if there's anything unclear or you still don't understand
the "weirdly rounded integer".

That aside, from what Ben says it sounds like there is a problem in how you're
set up on Windows.

[ckocagil, 2014-08-21 13:40:22]

Reading Tom's document on subpixel positioning this much more clearer. Thanks! I
will find a workaround here until I can hook up gfx with a subpixel font
factory.

However, I think it would be better if this mismatch between the subpixel
rendering flag and what the Skia embedder's font manager implements was handled
more gracefully. There could be a "virtual bool supportsSubpixel() = 0" in
SkScalerContext, and SkScalerContext::isSubpixel() would then return
"supportsSubpixel() && has_subpixel_bit_set". Does this sound reasonable?

[ckocagil, 2014-08-22 21:38:27]

PTAL. I disabled subpixel positioning on Win for UI (we need
http://crbug.com/389649 for that), and added an OS_LINUX guard around the
Pango-like rounding code.

[msw, 2014-08-22 21:59:36]

https://codereview.chromium.org/484883003/diff/100001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/484883003/diff/100001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1158: run->width = std::floor(run->width + 0.5f);
I wonder if this should still be contingent on if
(!run->render_params.subpixel_positioning).

[ckocagil, 2014-08-22 22:22:40]

https://codereview.chromium.org/484883003/diff/100001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/484883003/diff/100001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1158: run->width = std::floor(run->width + 0.5f);
On 2014/08/22 21:59:36, msw wrote:
> I wonder if this should still be contingent on if
> (!run->render_params.subpixel_positioning).

Done. Subpixel positioning on Linux seems to be working when you enable it with
--enable-browser-text-subpixel-positioning.

[msw, 2014-08-22 22:26:23]

lgtm; thanks, Cem!

[ckocagil, 2014-08-25 17:45:01]

Daniel, can you take another look? Thanks.

[Daniel Erat, 2014-08-25 18:27:12]

lgtm

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_mac.cc (right):

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
ui/gfx/font_render_params_mac.cc:31: // Customized font rendering settings are
not yet implemented.
do macs support per-family/size/style customization of rendering settings? if
not, this comment doesn't seem accurate.

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
for_web_contents;
(this is fine, but note that blink doesn't look at this now afaik and i'll be
deleting it from FontRenderParams (hopefully) soon)

[ckocagil, 2014-08-25 20:49:12]

This CL should be ready now. Land ahoy!

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_mac.cc (right):

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
ui/gfx/font_render_params_mac.cc:31: // Customized font rendering settings are
not yet implemented.
On 2014/08/25 18:27:12, Daniel Erat wrote:
> do macs support per-family/size/style customization of rendering settings? if
> not, this comment doesn't seem accurate.

I don't know. I changed to comment, it should be better now.

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/484883003/diff/120001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:38: params_->subpixel_positioning =
for_web_contents;
On 2014/08/25 18:27:12, Daniel Erat wrote:
> (this is fine, but note that blink doesn't look at this now afaik and i'll be
> deleting it from FontRenderParams (hopefully) soon)

Okay, I'm not touching it in this CL then.

[ckocagil, 2014-08-25 20:49:29]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-25 20:51:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/484883003/160001

[commit-bot: I haz the power, 2014-08-25 22:14:29]

Committed patchset #8 (160001) as 3d95ea64e7c8f5df67c1620dfdb9409f7685dc3c

[commit-bot: I haz the power, 2014-09-10 02:37:48]

Patchset 8 (id:??) landed as
https://crrev.com/04f49f48a13c337cb9eaef0146ec4f15ed91984a
Cr-Commit-Position: refs/heads/master@{#291767}