Closed Bug 1230357 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Text, defect)

40 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

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
Flags: needinfo?(lsalzman)
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
Attachment #8695591 - Flags: review?(lsalzman)
Flags: needinfo?(lsalzman)
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.
Blocks: 1230366
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.
Attachment #8695591 - Flags: review?(lsalzman) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/e9d31795c801
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
See Also: → 1250787
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: