Always round text position correctly.

https://codereview.appspot.com/7383049/

Will require rebaseline of fontscaler GM.

Must add SK_IGNORE_SUBPIXEL_AXIS_ALIGN_FIX to Chromium
until ~150 layout tests can be rebaselined.



git-svn-id: http://skia.googlecode.com/svn/trunk@7842 2bbb7eff-a529-9590-31e7-b0007b416f81

4 files changed, 79 insertions, 84 deletions

diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index 7c3d7c57d2..92bd32c638 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -1606,6 +1606,12 @@ SkDraw1Glyph::Proc SkDraw1Glyph::init(const SkDraw* draw, SkBlitter* blitter,
     fBlitter = blitter;
     fCache = cache;
 
+    if (cache->isSubpixel()) {
+        fHalfSampleX = fHalfSampleY = (SK_FixedHalf >> SkGlyph::kSubBits);
+    } else {
+        fHalfSampleX = fHalfSampleY = SK_FixedHalf;
+    }
+
     if (hasCustomD1GProc(*draw)) {
         // todo: fix this assumption about clips w/ custom
         fClip = draw->fClip;
@@ -1662,15 +1668,13 @@ void SkDraw::drawText(const char text[], size_t byteLength,
 
     SkDrawCacheProc glyphCacheProc = paint.getDrawCacheProc();
 
-    const SkMatrix* matrix = fMatrix;
-
-    SkAutoGlyphCache    autoCache(paint, &fDevice->fLeakyProperties, matrix);
+    SkAutoGlyphCache    autoCache(paint, &fDevice->fLeakyProperties, fMatrix);
     SkGlyphCache*       cache = autoCache.getCache();
 
     // transform our starting point
     {
         SkPoint loc;
-        matrix->mapXY(x, y, &loc);
+        fMatrix->mapXY(x, y, &loc);
         x = loc.fX;
         y = loc.fY;
     }
@@ -1692,33 +1696,13 @@ void SkDraw::drawText(const char text[], size_t byteLength,
         y -= stopY;
     }
 
-    SkFixed fx = SkScalarToFixed(x);
-    SkFixed fy = SkScalarToFixed(y);
     const char* stop = text + byteLength;
 
-    SkFixed fxMask = ~0;
-    SkFixed fyMask = ~0;
-    if (cache->isSubpixel()) {
-        SkAxisAlignment baseline = SkComputeAxisAlignmentForHText(*matrix);
-        if (kX_SkAxisAlignment == baseline) {
-            fyMask = 0;
-        } else if (kY_SkAxisAlignment == baseline) {
-            fxMask = 0;
-        }
-
-    // apply bias here to avoid adding 1/2 the sampling frequency in the loop
-        fx += SK_FixedHalf >> SkGlyph::kSubBits;
-        fy += SK_FixedHalf >> SkGlyph::kSubBits;
-    } else {
-        fx += SK_FixedHalf;
-        fy += SK_FixedHalf;
-    }
-
     SkAAClipBlitter     aaBlitter;
     SkAutoBlitterChoose blitterChooser;
     SkBlitter*          blitter = NULL;
     if (needsRasterTextBlit(*this)) {
-        blitterChooser.choose(*fBitmap, *matrix, paint);
+        blitterChooser.choose(*fBitmap, *fMatrix, paint);
         blitter = blitterChooser.get();
         if (fRC->isAA()) {
             aaBlitter.init(blitter, &fRC->aaRgn());
@@ -1730,6 +1714,22 @@ void SkDraw::drawText(const char text[], size_t byteLength,
     SkDraw1Glyph        d1g;
     SkDraw1Glyph::Proc  proc = d1g.init(this, blitter, cache);
 
+    SkFixed fxMask = ~0;
+    SkFixed fyMask = ~0;
+    if (cache->isSubpixel()) {
+        SkAxisAlignment baseline = SkComputeAxisAlignmentForHText(*fMatrix);
+        if (kX_SkAxisAlignment == baseline) {
+            fyMask = 0;
+            d1g.fHalfSampleY = SK_FixedHalf;
+        } else if (kY_SkAxisAlignment == baseline) {
+            fxMask = 0;
+            d1g.fHalfSampleX = SK_FixedHalf;
+        }
+    }
+
+    SkFixed fx = SkScalarToFixed(x) + d1g.fHalfSampleX;
+    SkFixed fy = SkScalarToFixed(y) + d1g.fHalfSampleY;
+
     while (text < stop) {
         const SkGlyph& glyph = glyphCacheProc(cache, &text, fx & fxMask, fy & fyMask);
 
@@ -1855,17 +1855,15 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
         return;
     }
 
-    const SkMatrix* matrix = fMatrix;
-
     SkDrawCacheProc     glyphCacheProc = paint.getDrawCacheProc();
-    SkAutoGlyphCache    autoCache(paint, &fDevice->fLeakyProperties, matrix);
+    SkAutoGlyphCache    autoCache(paint, &fDevice->fLeakyProperties, fMatrix);
     SkGlyphCache*       cache = autoCache.getCache();
 
     SkAAClipBlitterWrapper wrapper;
     SkAutoBlitterChoose blitterChooser;
     SkBlitter* blitter = NULL;
     if (needsRasterTextBlit(*this)) {
-        blitterChooser.choose(*fBitmap, *matrix, paint);
+        blitterChooser.choose(*fBitmap, *fMatrix, paint);
         blitter = blitterChooser.get();
         if (fRC->isAA()) {
             wrapper.init(*fRC, blitter);
@@ -1877,29 +1875,33 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
     AlignProc          alignProc = pick_align_proc(paint.getTextAlign());
     SkDraw1Glyph       d1g;
     SkDraw1Glyph::Proc proc = d1g.init(this, blitter, cache);
-    TextMapState       tms(*matrix, constY);
+    TextMapState       tms(*fMatrix, constY);
     TextMapState::Proc tmsProc = tms.pickProc(scalarsPerPosition);
 
     if (cache->isSubpixel()) {
         // maybe we should skip the rounding if linearText is set
-        SkAxisAlignment roundBaseline = SkComputeAxisAlignmentForHText(*matrix);
+        SkAxisAlignment baseline = SkComputeAxisAlignmentForHText(*fMatrix);
+
+        SkFixed fxMask = ~0;
+        SkFixed fyMask = ~0;
+        if (kX_SkAxisAlignment == baseline) {
+            fyMask = 0;
+#ifndef SK_IGNORE_SUBPIXEL_AXIS_ALIGN_FIX
+            d1g.fHalfSampleY = SK_FixedHalf;
+#endif
+        } else if (kY_SkAxisAlignment == baseline) {
+            fxMask = 0;
+#ifndef SK_IGNORE_SUBPIXEL_AXIS_ALIGN_FIX
+            d1g.fHalfSampleX = SK_FixedHalf;
+#endif
+        }
 
         if (SkPaint::kLeft_Align == paint.getTextAlign()) {
             while (text < stop) {
-
                 tmsProc(tms, pos);
 
-                SkFixed fx = SkScalarToFixed(tms.fLoc.fX) + (SK_FixedHalf >> SkGlyph::kSubBits);
-                SkFixed fy = SkScalarToFixed(tms.fLoc.fY) + (SK_FixedHalf >> SkGlyph::kSubBits);
-
-                SkFixed fxMask = ~0;
-                SkFixed fyMask = ~0;
-
-                if (kX_SkAxisAlignment == roundBaseline) {
-                    fyMask = 0;
-                } else if (kY_SkAxisAlignment == roundBaseline) {
-                    fxMask = 0;
-                }
+                SkFixed fx = SkScalarToFixed(tms.fLoc.fX) + d1g.fHalfSampleX;
+                SkFixed fy = SkScalarToFixed(tms.fLoc.fY) + d1g.fHalfSampleY;
 
                 const SkGlyph& glyph = glyphCacheProc(cache, &text,
                                                       fx & fxMask, fy & fyMask);
@@ -1912,38 +1914,28 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
         } else {
             while (text < stop) {
                 const char* currentText = text;
-                const SkGlyph* glyph = &glyphCacheProc(cache, &text, 0, 0);
+                const SkGlyph& metricGlyph = glyphCacheProc(cache, &text, 0, 0);
 
-                if (glyph->fWidth) {
-                    SkDEBUGCODE(SkFixed prevAdvX = glyph->fAdvanceX;)
-                    SkDEBUGCODE(SkFixed prevAdvY = glyph->fAdvanceY;)
+                if (metricGlyph.fWidth) {
+                    SkDEBUGCODE(SkFixed prevAdvX = metricGlyph.fAdvanceX;)
+                    SkDEBUGCODE(SkFixed prevAdvY = metricGlyph.fAdvanceY;)
 
-                    SkFixed fx, fy;
-                    SkFixed fxMask = ~0;
-                    SkFixed fyMask = ~0;
                     tmsProc(tms, pos);
+                    SkIPoint fixedLoc;
+                    alignProc(tms.fLoc, metricGlyph, &fixedLoc);
 
-                    {
-                        SkIPoint fixedLoc;
-                        alignProc(tms.fLoc, *glyph, &fixedLoc);
-                        fx = fixedLoc.fX + (SK_FixedHalf >> SkGlyph::kSubBits);
-                        fy = fixedLoc.fY + (SK_FixedHalf >> SkGlyph::kSubBits);
-
-                        if (kX_SkAxisAlignment == roundBaseline) {
-                            fyMask = 0;
-                        } else if (kY_SkAxisAlignment == roundBaseline) {
-                            fxMask = 0;
-                        }
-                    }
+                    SkFixed fx = fixedLoc.fX + d1g.fHalfSampleX;
+                    SkFixed fy = fixedLoc.fY + d1g.fHalfSampleY;
 
                     // have to call again, now that we've been "aligned"
-                    glyph = &glyphCacheProc(cache, &currentText,
-                                            fx & fxMask, fy & fyMask);
-                    // the assumption is that the advance hasn't changed
-                    SkASSERT(prevAdvX == glyph->fAdvanceX);
-                    SkASSERT(prevAdvY == glyph->fAdvanceY);
+                    const SkGlyph& glyph = glyphCacheProc(cache, &currentText,
+                                                          fx & fxMask, fy & fyMask);
+                    // the assumption is that the metrics haven't changed
+                    SkASSERT(prevAdvX == glyph.fAdvanceX);
+                    SkASSERT(prevAdvY == glyph.fAdvanceY);
+                    SkASSERT(glyph.fWidth);
 
-                    proc(d1g, fx, fy, *glyph);
+                    proc(d1g, fx, fy, glyph);
                 }
                 pos += scalarsPerPosition;
             }
@@ -1958,8 +1950,8 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
                     tmsProc(tms, pos);
 
                     proc(d1g,
-                         SkScalarToFixed(tms.fLoc.fX) + SK_FixedHalf,
-                         SkScalarToFixed(tms.fLoc.fY) + SK_FixedHalf,
+                         SkScalarToFixed(tms.fLoc.fX) + SK_FixedHalf, //d1g.fHalfSampleX,
+                         SkScalarToFixed(tms.fLoc.fY) + SK_FixedHalf, //d1g.fHalfSampleY,
                          glyph);
                 }
                 pos += scalarsPerPosition;
@@ -1976,8 +1968,8 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
                     alignProc(tms.fLoc, glyph, &fixedLoc);
 
                     proc(d1g,
-                         fixedLoc.fX + SK_FixedHalf,
-                         fixedLoc.fY + SK_FixedHalf,
+                         fixedLoc.fX + SK_FixedHalf, //d1g.fHalfSampleX,
+                         fixedLoc.fY + SK_FixedHalf, //d1g.fHalfSampleY,
                          glyph);
                 }
                 pos += scalarsPerPosition;
diff --git a/src/core/SkDrawProcs.h b/src/core/SkDrawProcs.h
index ec086dd7da..68cc9ff08c 100644
--- a/src/core/SkDrawProcs.h
+++ b/src/core/SkDrawProcs.h
@@ -21,10 +21,18 @@ struct SkDraw1Glyph {
     SkBlitter* fBlitter;
     SkGlyphCache* fCache;
     SkIRect fClipBounds;
-
-    // The fixed x,y are pre-rounded, so impls just trunc them down to ints.
-    // i.e. half the sampling frequency has been added.
-    // e.g. 1/2 or 1/(2^(SkGlyph::kSubBits+1)) has already been added.
+    /** Half the sampling frequency of the rasterized glyph in x. */
+    SkFixed fHalfSampleX;
+    /** Half the sampling frequency of the rasterized glyph in y. */
+    SkFixed fHalfSampleY;
+
+    /** Draws one glyph.
+     *
+     *  The x and y are pre-biased, so implementations may just truncate them.
+     *  i.e. half the sampling frequency has been added.
+     *  e.g. 1/2 or 1/(2^(SkGlyph::kSubBits+1)) has already been added.
+     *  This added bias can be found in fHalfSampleX,Y.
+     */
     typedef void (*Proc)(const SkDraw1Glyph&, SkFixed x, SkFixed y, const SkGlyph&);
 
     Proc init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache);
diff --git a/src/device/xps/SkXPSDevice.cpp b/src/device/xps/SkXPSDevice.cpp
index 2f01123c5e..30ac3305ee 100644
--- a/src/device/xps/SkXPSDevice.cpp
+++ b/src/device/xps/SkXPSDevice.cpp
@@ -2182,13 +2182,8 @@ static void xps_draw_1_glyph(const SkDraw1Glyph& state,
     SkXPSDrawProcs* procs = static_cast<SkXPSDrawProcs*>(state.fDraw->fProcs);
 
     //Draw pre-adds half the sampling frequency for floor rounding.
-    if (state.fCache->isSubpixel()) {
-        x -= (SK_FixedHalf >> SkGlyph::kSubBits);
-        y -= (SK_FixedHalf >> SkGlyph::kSubBits);
-    } else {
-        x -= SK_FixedHalf;
-        y -= SK_FixedHalf;
-    }
+    x -= state.fHalfSampleX;
+    y -= state.fHalfSampleY;
 
     XPS_GLYPH_INDEX* xpsGlyph = procs->xpsGlyphs.append();
     uint16_t glyphID = skGlyph.getGlyphID();
diff --git a/tests/FontHostTest.cpp b/tests/FontHostTest.cpp
index 4cd7812eeb..5c43128257 100644
--- a/tests/FontHostTest.cpp
+++ b/tests/FontHostTest.cpp
@@ -145,7 +145,7 @@ static void test_advances(skiatest::Reporter* reporter) {
     char txt[] = "long.text.with.lots.of.dots.";
 
     for (size_t i = 0; i < SK_ARRAY_COUNT(faces); i++) {
-        SkTypeface* face = SkTypeface::CreateFromName(faces[i], SkTypeface::kNormal);
+        SkAutoTUnref<SkTypeface> face(SkTypeface::CreateFromName(faces[i], SkTypeface::kNormal));
         paint.setTypeface(face);
 
         for (size_t j = 0; j  < SK_ARRAY_COUNT(settings); j++) {