1230357 - Reftest unicode-bidi-plaintext fails with a skia content backend

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mason Chang [Inactive] [:mchang]]

What seems to be happening is that we're getting these glyph offsets from layout:

[0] = (fX = 8, fY = 23)
[1] = (fX = 16.1166668, fY = 23)
[2] = (fX = 20.583334, fY = 23)
[3] = (fX = 27.4833336, fY = 23)

at [1]. In the working case, we're getting these offsets from layout:
[0] = (fX = 8.01666641, fY = 23)
[1] = (fX = 16.1333332, fY = 23)
[2] = (fX = 20.6000004, fY = 23)
[3] = (fX = 27.5, fY = 23)

The important point to look at is [3], where the x position is 27.48 versus 27.5. With a CG backend, we feed these points to CG via CTFontDrawGlyphs, and it handles it just fine. Skia on the other hand, actually rounds up: (Code pasted since this is in the updated version of skia):

SkDraw.cpp
1709     SkFindAndPlaceGlyph::ProcessPosText(
1710         paint.getTextEncoding(), text, byteLength,
1711         offset, *fMatrix, pos, scalarsPerPosition, textAlignment, cache,
1712         [&](const SkGlyph& glyph, SkPoint position, SkPoint rounding) {
1713             position += rounding; // The rounding is 0.5
1714             proc(d1g, SkScalarTo48Dot16(position.fX),    
                           SkScalarTo48Dot16(position.fY), glyph);
1715         }
1716     );

So in the broken case, it adds 0.5, the x position becomes 27.98, then gets concatenated to an int, becoming 27 so it's device pixel aligned, and then is rendered at the position. In the non-ref test, since the x position is 27.5, the rounding rounds it up to 28, hence looking correct.

I'm not sure if the right approach is to add support in skia for non-device pixel aligned glyphs or what. Any oped Lee? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp?from=DrawTargetSkia.cpp#621

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Enable subpixel text rendering when filling glyphs

Looks like we just have to enable subpixel text rendering. [1]. It fixes the spacing issue on this test case, but now we have a separate slightly different color issue.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/include/core/SkPaint.h?from=SkPaint.h#180

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Reftest failure with patch

In case you want to see the reftest failure, just load the log here:

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml

This seems fuzzable.

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 8695591 [details] [diff] [review]
Enable subpixel text rendering when filling glyphs

This looks okay. We should just do a try run on a few other platforms (Windows/Linux/Android) with this and Skia content enabled just to make sure it doesn't trigger any other reftest failures.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81f9899eb8e9

Comment 5

[Mason Chang [Inactive] [:mchang]]

Looks like mostly test tests/reftest/tests/layout/reftests/writing-mode/1090168-3.html, which is a change in spacing probably due to hinting. The test itself is a canvas test which draws some text, then rotates it via a transform.

Comment 6

[Mason Chang [Inactive] [:mchang]]

From irc, we should always enable subpixel text positioning with or without subpixel AA. Both Dwrite + OS X always enable it.

Try with always enabled + some fuzz: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7685b13452f4

Comment 7

[Mason Chang [Inactive] [:mchang]]

With fixed fuzz for b2g - https://treeherder.mozilla.org/#/jobs?repo=try&revision=df41bf267913

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d31795c801

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9d31795c801