Bug 94028 – Correct glyph positioning in high-DPI mode without using --enable-text-subpixel-positioning

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Terry Anderson]

Currently some glyphs are improperly spaced within regular text when running chromium with the --force-device-scale-factor=2 flag.  Using --enable-text-subpixel-positioning resolves the glyph spacing issue but introduces other problems such as awkward line breaking in some instances.

[Terry Anderson]

Created attachment 158430 [details]
Patch

[Terry Anderson]

(In reply to comment #1)
> Created an attachment (id=158430) [details]
> Patch

This patch is for discussion only and is clearly not landable as-is. But when you apply this patch and run chromium with --force-device-scale-factor=2 (and without using --enable-text-subpixel-positioning), the glyph positioning problems are resolved.

The multiplication and division by |scaleFactor| in platformWidthForGlyph() does not seem like the right thing to do, as it would require exposing the device scale factor to SimpleFontDataSkia.  Instead, the font size should be computed using the device scale factor in the first place so that |m_platformData| always stores the correct size.  It seems this should be done in StyleResolver::styleForDocument(), where the size of |fontDescription| is set.

If the above change is made, then the device scale factor should not be applied to the text a second time, otherwise it will be rendered twice as large. This scaling is currently applied somewhere in layout or rendering, but I am still trying to find exactly where this happens.

Any comments and suggestions are welcome.

[Terry Anderson]

Here is an alternate suggestion from vollick@ that I am currently investigating: instead of exposing the device scale factor, try to plumb through the xScale of the current transformation matrix to SimpleFontDataSkia::platformWidthForGlyph() and then use this to ensure the function returns a correct width measurement. If this is possible, then the approach in the attached patch can be used.

[Alexandre Elias]

Adding johnme@ and wangxianzhu@ who may have some knowledge about this.

[Xianzhu Wang]

We are using subpixel positioning on Android otherwise the spacing is not good because of pixel snapping. We haven't noticed any line breaking issue.

[John Mellor]

(In reply to comment #2)
> Instead, the font size should be computed using the device scale factor in the first place so that |m_platformData| always stores the correct size.  It seems this should be done in StyleResolver::styleForDocument(), where the size of |fontDescription| is set.

I really don't think StyleResolver or RenderStyle::fontDescription() should have to worry about deviceScaleFactors. The render tree doesn't (and can't) worry about pageScaleFactor, since pinch zooms mustn't cause a layout. Instead the render tree should continue to stick to CSS pixels, to remain close to the original HTML/CSS styles, and use floats or LayoutUnits where subpixel accuracy is required (to compensate for it not being rendered at one device pixel to CSS pixel).

I'm afraid I don't know much about what happens in Skia or HarfBuzz. What does --enable-text-subpixel-positioning do exactly? Unless it has unavoidable disadvantages, given the current trend towards touch screens and pinch zooming (on mobile devices, but also on Windows 8, etc), it seems it might be best to consolidate on such an approach and fix any remaining bugs?

[Daniel Erat]

--enable-text-subpixel-positioning is a Chromium flag that affects text rendering in both Chrome's UI code and within WebKit, making us not round glyph widths and positions to integer values.  https://bugs.webkit.org/show_bug.cgi?id=88263 tracked the WebKit side of this.

[Terry Anderson]

(In reply to comment #3)
> Here is an alternate suggestion from vollick@ that I am currently investigating: instead of exposing the device scale factor, try to plumb through the xScale of the current transformation matrix to SimpleFontDataSkia::platformWidthForGlyph() and then use this to ensure the function returns a correct width measurement. If this is possible, then the approach in the attached patch can be used.

In order to get this plumbing to work, danakj@ made the suggestion to try storing the transformation matrix's xScale (obtained via the GraphicsContext parameter in Font::drawText()) as a member of WidthIterator. This would allow the scale factor to be passed through to SimpleFontData::widthForGlyph() and then to SimpleFontData::platformWidthForGlyph().

However, this appears to be a dead end since it isn't possible to have this value available in all places where a WidthIterator is instantiated (Font::floatWidthForSimpleText() in FontFastPath.cpp is one such example).

It looks like I'm back to square one. rjkroege@, do you have any suggestions for an alternate approach to this problem?

[Emil A Eklund]

Getting enable-text-subpixel-positioning to work correctly really seems like the right way forward. Could you point us to some of the problems with it you mentioned?

[Daniel Erat]

#9: http://crbug.com/132571 and http://crbug.com/137969 tracked two issues that showed up on Google websites once subpixel positioning was enabled, although I believe that at least one and maybe both of them were actually caused by bugs in WebKit that have since been fixed.

http://crosbug.com/31840 tracks an unresolved issue in hterm.

The underlying concern is that there may be many pages out there that use the clientWidth property of a single fixed-width-font glyph to extrapolate the width of longer runs of text.  Unless/until a sizable number of users are using subpixel positioning, there is a risk that enabling it will lead to subtle layout issues.

[Emil A Eklund]

(In reply to comment #10)
> The underlying concern is that there may be many pages out there that use the clientWidth property of a single fixed-width-font glyph to extrapolate the width of longer runs of text.  Unless/until a sizable number of users are using subpixel positioning, there is a risk that enabling it will lead to subtle layout issues.

That has always been problematic and imprecise, I don't see how using subpixel positioning for text would make it worse but perhaps I'm missing something?

[Daniel Erat]

> That has always been problematic and imprecise, I don't see how using subpixel positioning for text would make it worse but perhaps I'm missing something?

At least on Chrome OS, glyphs are typically located at integer coordinates and have integer advance widths; if you use clientWidth to get the width of a single space and multiply it by 5, the resulting value will match the width of an element containing five spaces.  Subpixel-positioned glyphs can have fractional positions and dimensions.

[Rick Byers]

We need high-DPI text to be rock-solid in M23 (i.e. a few weeks).  Dan made a mostly theoretical argument that sub-pixel positioning may break some sites or have more bugs, and so it was better to fix the hinting issue in the short run rather than rely on sub-pixel positioning.  So I went looking for issues that showed up only with sub-pixel text positioning turned on and quickly found this one: http://code.google.com/p/chromium/issues/detail?id=138960

It looks like maybe that's been fixed recently between r125842 and r125909.  It would be great to know what change fixed it (what the likelihood of similar issues is, etc.).

We've gone through some effort to make layout completely unaware of the scaling done for high-DPI, and that's served us well (we had a ton of bugs originally using an approach more like android where it wasn't as transparent).  It seems like ensuring we _can_ run without having to enable sub-pixel text positioning fits with that philosophy.

I agree the hterm case (assumptions about fixed-width fonts) is already pretty broken - this may not be a compelling argument in itself.  I'm worried about what other scenarios we may find and what other bugs may be lurking.

Thoughts?

[Emil A Eklund]

(In reply to comment #12)
> At least on Chrome OS, glyphs are typically located at integer coordinates and have integer advance widths; if you use clientWidth to get the width of a single space and multiply it by 5, the resulting value will match the width of an element containing five spaces.  Subpixel-positioned glyphs can have fractional positions and dimensions.

Even so this will only work at zoom levels that are evenly divisible by one. At a zoom level of 1.1, 1.25 or 0.9 clientWidth will return a rounded value.
Say you have a glyph that is 5px wide and the zoom level is 1.1, clientWidth will then return 5/1.1 (~4.54) which will be rounded to 5.

[Emil A Eklund]

(In reply to comment #13)
> It looks like maybe that's been fixed recently between r125842 and r125909.  It would be great to know what change fixed it (what the likelihood of similar issues is, etc.).

Pretty narrow range, should be easy to spot: http://trac.webkit.org/log/trunk/Source/WebCore?rev=125909&stop_rev=125842

[Terry Anderson]

Created attachment 159824 [details]
Patch

[Terry Anderson]

Comment on attachment 159824 [details]
Patch

Second patch for feedback. This appears to fix the problem and does not use a hard-coded scale factor. Tests still to be written.

[Rick Byers]

By the way, I'm OK to try to leave sub-pixel positioning enabled, as long as we have high confidence that we can turn it off quickly if need be due to a new bug or website compat issue (i.e. I think we should try to fix this hinting issue regardless, even if the decision is to leave sub-pixel positioning on).

[Build Bot]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13565079

[Build Bot]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13561104

[Gustavo Noronha (kov)]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13557137

[Early Warning System Bot]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13571052

[Early Warning System Bot]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13566090

[Gyuyoung Kim]

Comment on attachment 159824 [details]
Patch

Attachment 159824 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13565105

[Dana Jansens]

Comment on attachment 159824 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:249
>          width = SkScalarRound(width);

Why not "width = SkScalarRound(width * scaleFactor) / scaleFactor" and drop the changes to setupPaint?

[Behdad Esfahbod]

(In reply to comment #25)
> (From update of attachment 159824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159824&action=review
> 
> > Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:249
> >          width = SkScalarRound(width);
> 
> Why not "width = SkScalarRound(width * scaleFactor) / scaleFactor" and drop the changes to setupPaint?

Because that assumes that glyph width is linear in font size.  That simply is not true with hinting.  That's the crux of this bug.  There are no shortcuts.

See:
http://www.rastertragedy.com/

[Behdad Esfahbod]

(In reply to comment #17)
> (From update of attachment 159824 [details])
> Second patch for feedback. This appears to fix the problem and does not use a hard-coded scale factor. Tests still to be written.

Terry, I'm afraid this introduce bugs with cursor positioning, etc, where we don't have the scale factor available?

[Behdad Esfahbod]

(In reply to comment #9)
> Getting enable-text-subpixel-positioning to work correctly really seems like the right way forward. Could you point us to some of the problems with it you mentioned?

Let me give it a try:

Text subpixel positioning is an option.  Code should work correctly both with and without it (and with deviceScaleFactor=1 and !=1).  Whether we decide to enable text subpixel positioning in high-DPI mode is a decision that should be made independently of webkit.

The bug being discussed in this bug is that currently, with deviceScaleFactor != 1, webkit (at least the port we are talking about) is measuring text at a given size, then multiply it by deviceScaleFactor, assuming that this would give the same result as measuring text at a font size scaled by deviceScaleFactor.  This assumption is only true when hinting is off, and subpixel positioning is on.   We should fix the code to not make such an assumption to begin with.

[John Mellor]

(In reply to comment #28)
> The bug being discussed in this bug is that currently, with deviceScaleFactor != 1, webkit (at least the port we are talking about) is measuring text at a given size, then multiply it by deviceScaleFactor, assuming that this would give the same result as measuring text at a font size scaled by deviceScaleFactor.  This assumption is only true when hinting is off, and subpixel positioning is on.   We should fix the code to not make such an assumption to begin with.

But presumably that assumption is necessary for pageScaleFactor: since layout can't change when pinch zooming, and text gets redrawn for different zoom levels, it needs to be possible to measure text unscaled, then scale it by a scale factor, then hint it at the new scale such that runs of text wrap in the same places (and are a roughly consistent size).

Though I guess as long as the wrapping doesn't change and nothing is out by more than a pixel or so that doesn't require things to be exactly the same size...

[Terry Anderson]

Three things left to address:(In reply to comment #27)
> (In reply to comment #17)
> > (From update of attachment 159824 [details] [details])
> > Second patch for feedback. This appears to fix the problem and does not use a hard-coded scale factor. Tests still to be written.
> 
> Terry, I'm afraid this introduce bugs with cursor positioning, etc, where we don't have the scale factor available?

AFAICT from my testing, cursor positioning works fine when this patch is applied, even when switching between different page zoom levels. Can you point me to a specific instance that you think would not work correctly?

[Terry Anderson]

Three things I still need to address:

1. The call to widthForGlyph() from offsetToMiddleOfGlyph() in FontFastPath.cpp should also have the scale factor passed in as an argument.

2. The logic of this patch should also be applied to fonts which follow the complex path to avoid incorrect spacing in between text runs at low page zoom levels.

3. The EWS failures.

[Adam Barth]

My (possibly mistaken) understanding of this issue is that we need to use subpixel positioning for text in the long term to make everything actually work (e.g., pinch-to-zoom of 1.1).  There's some amount of fear that subpixel text positioning will cause compatibility problems, but no one has any actual examples of problems.

According to http://code.google.com/p/chromium/issues/detail?id=115888, we need to stop defining SK_DRAW_POS_TEXT_IGNORE_SUBPIXEL_LEFT_ALIGN_FIX, but that's mostly a matter of rebaselining some LayoutTests.  There are some bugs in browser UI, but they don't need to block us turning it on for web content.

It sounds like the best way to proceed here is to turn on subpixel text positioning.

[Adam Barth]

We discussed this a bit more on #webkit.  There appear to be some contradictory constraints.  behdad is going to figure out what we need to do on win8 because that's the platform that appears to have the most constraints.

[Rick Byers]

(In reply to comment #32)
> There's some amount of fear that subpixel text positioning will cause compatibility problems, but no one has any actual examples of problems.

One clarification: we have no examples of issues which are still a problem today.  We have:
 - at least one bug that was fixed: https://bugs.webkit.org/show_bug.cgi?id=90097
 - one bug we _think_ appears to be fixed: http://crbug.com/137969
 - one poorly written app whose problems were made much worse by sub-pixel text positioning, but was eventually fixed: http://code.google.com/p/chromium-os/issues/detail?id=31840

We have very little testing/usage of this code path, so it's reasonable to expect there to be more issues.

Also, this code isn't used for rendering text on Mac, right?  On retina Mac, both Safari and chromium appear to do correct hinting without sub-pixel text positioning.  Shouldn't we be choosing the same solution for both platforms (eg. move Mac to sub-pixel text positioning at the same time, if that's what we're going to rely on for ChromeOS)?

[Adam Barth]

The more we discuss this topic, the more it sounds like we'll need to support both subpixel and non-subpixel text positioning for a while at least.  I suspect we'll end up using subpixel text positioning, but we might want to get there more gradually.

[Terry Anderson]

Created attachment 160212 [details]
Patch

[Build Bot]

Comment on attachment 160212 [details]
Patch

Attachment 160212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13565841

[Adam Barth]

Comment on attachment 160212 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1706
> +#if PLATFORM(CHROMIUM)

We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to this code.

[Adam Barth]

Comment on attachment 160212 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:235
> +#if PLATFORM(CHROMIUM)

Does anyone besides Chromium use graphics/skia?  In any case, we shouldn't have ifdefs for this sort of thing.  It's not anything specific to the Chromium port.

[Robert Kroeger]

(In reply to comment #39)
> (From update of attachment 160212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160212&action=review
> 
> > Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:235
> > +#if PLATFORM(CHROMIUM)
> 
> Does anyone besides Chromium use graphics/skia?  In any case, we shouldn't have ifdefs for this sort of thing.  It's not anything specific to the Chromium port.

Android (did?)

[Xianzhu Wang]

(In reply to comment #40)
> Android (did?)

The PLATFORM(ANDROID) has been removed (https://bugs.webkit.org/show_bug.cgi?id=66741). Now Android is an OS of PLATFORM(CHROMIUM).

[Terry Anderson]

Created attachment 160249 [details]
Patch

[Adam Barth]

@tony: Would you be willing to help review this patch?

[Tony Chang]

If I understand the patch correctly, you need the scale factor when drawing text.  To do this, you set the scale factor (which comes from Page) on every FontDescription during CSS style resolution.  This seems like it would use a lot of memory to duplicate the same number and I suspect setting this during style resolution is some sort of layering violation.

mitz might be able to provide a better way to design this.  I'm not sure what the right design is, but you might be able to pass the scale factor into GraphicsContext::drawText.

[Tony Chang]

Comment on attachment 160249 [details]
Patch

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

> Source/WebCore/platform/graphics/FontDescription.cpp:39
>      // FXIME: Make them fit into one word.

Nit: May as well fix the FXIME typo while you're here.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:151
> +float SimpleFontData::platformWidthForGlyph(Glyph glyph, float scaleFactor) const
>  {
> +    UNUSED_PARAM(scaleFactor);

If you don't name the float, you don't need to use UNUSED_PARAM.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:419
> +float SimpleFontData::platformWidthForGlyph(Glyph glyph, float scaleFactor) const
>  {
> +    UNUSED_PARAM(scaleFactor);

Ditto.

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:54
> +float SimpleFontData::platformWidthForGlyph(Glyph glyph, float scaleFactor) const
>  {
> +    UNUSED_PARAM(scaleFactor);

Ditto.

> Source/WebCore/platform/graphics/win/SimpleFontDataCGWin.cpp:125
> +float SimpleFontData::platformWidthForGlyph(Glyph glyph, float scaleFactor) const
>  {
> +    UNUSED_PARAM(scaleFactor);

Ditto.

[Behdad Esfahbod]

(In reply to comment #44)
> If I understand the patch correctly, you need the scale factor when drawing text.

My understanding is that it's needed during layout (ie. text measurement),  not rendering.  Don't know why that is exactly, but rendering doesn't seem to have any problem figuring out the right (scaled) size.  But layout doesn't.  And the discrepancy introduces layout errors.

[John Mellor]

(In reply to comment #46)
> My understanding is that it's needed during layout (ie. text measurement),  not rendering.  Don't know why that is exactly, but rendering doesn't seem to have any problem figuring out the right (scaled) size.  But layout doesn't.  And the discrepancy introduces layout errors.

But as I tried to say in comment #29, at least for pinch zoom (pageScaleFactor) it's not possible at layout time to know at what scale factor it will be drawn, since it will potentially be drawn at multiple scales per layout.

So it seems we have to make the pageScaleFactor at least not be needed during layout, and if we already have that, then presumably we could use the same approach for deviceScaleFactor too.

[Behdad Esfahbod]

Thanks for all the comments.  I'm putting a document together that should help us resolve this.  Stay tuned.

[Adam Barth]

The ieblog claims that IE10 uses subpixel text positioning:

http://blogs.msdn.com/b/ie/archive/2012/08/28/optical-zooming-in-legacy-document-modes.aspx

[Behdad Esfahbod]

Yes, I confirmed that on Win8, IE uses subpixel positioning (in both Desktop and Metro modes).  iOS Safari also does subpixel positioning.  Mac OS X has been doing subpixel positioning for years, but ironically, not in Safari.

[Terry Anderson]

This issue has been addressed with https://bugs.webkit.org/show_bug.cgi?id=96137.