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)
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mchang, Assigned: mchang)
References
Details
(Whiteboard: gfx-noted)
Attachments
(2 files)
995 bytes,
patch
|
lsalzman
:
review+
|
Details | Diff | Splinter Review |
77.83 KB,
text/plain
|
Details |
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
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(lsalzman)
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(lsalzman)
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81f9899eb8e9
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
With fixed fuzz for b2g - https://treeherder.mozilla.org/#/jobs?repo=try&revision=df41bf267913
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9d31795c801
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•