Subpixel text 3/8 of a pixel too far to the right.

http://codereview.appspot.com/5502097/


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

5 files changed, 166 insertions, 31 deletions

diff --git a/gyp/tests.gyp b/gyp/tests.gyp
index 27a34a014c..011030c36c 100644
--- a/gyp/tests.gyp
+++ b/gyp/tests.gyp
@@ -29,6 +29,7 @@
         '../tests/DataRefTest.cpp',
         '../tests/DequeTest.cpp',
         '../tests/DrawBitmapRectTest.cpp',
+        '../tests/DrawTextTest.cpp',
         '../tests/EmptyPathTest.cpp',
         '../tests/FillPathTest.cpp',
         '../tests/FlateTest.cpp',
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index c6fd4068d4..d1b291fd11 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -13,6 +13,7 @@
 #include "SkCanvas.h"
 #include "SkColorPriv.h"
 #include "SkDevice.h"
+#include "SkFixed.h"
 #include "SkMaskFilter.h"
 #include "SkPaint.h"
 #include "SkPathEffect.h"
@@ -1477,9 +1478,9 @@ static bool needsRasterTextBlit(const SkDraw& draw) {
 SkDraw1Glyph::Proc SkDraw1Glyph::init(const SkDraw* draw, SkBlitter* blitter,
                                       SkGlyphCache* cache) {
     fDraw = draw;
-	fBounder = draw->fBounder;
-	fBlitter = blitter;
-	fCache = cache;
+    fBounder = draw->fBounder;
+    fBlitter = blitter;
+    fCache = cache;
 
     if (hasCustomD1GProc(*draw)) {
         // todo: fix this assumption about clips w/ custom
@@ -1577,17 +1578,21 @@ void SkDraw::drawText(const char text[], size_t byteLength,
 
     SkFixed fxMask = ~0;
     SkFixed fyMask = ~0;
-    if (paint.isSubpixelText()) {
+    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;
     }
-    // apply the bias here, so we don't have to add 1/2 in the loop
-    fx += SK_FixedHalf;
-    fy += SK_FixedHalf;
 
     SkAAClipBlitter     aaBlitter;
     SkAutoBlitterChoose blitterChooser;
@@ -1606,7 +1611,7 @@ void SkDraw::drawText(const char text[], size_t byteLength,
     SkDraw1Glyph::Proc  proc = d1g.init(this, blitter, cache);
 
     while (text < stop) {
-        const SkGlyph& glyph  = glyphCacheProc(cache, &text, fx & fxMask, fy & fyMask);
+        const SkGlyph& glyph = glyphCacheProc(cache, &text, fx & fxMask, fy & fyMask);
 
         fx += autokern.adjust(glyph);
 
@@ -1757,12 +1762,12 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
     
     const char*        stop = text + byteLength;
     AlignProc          alignProc = pick_align_proc(paint.getTextAlign());
-	SkDraw1Glyph	   d1g;
-	SkDraw1Glyph::Proc  proc = d1g.init(this, blitter, cache);
+    SkDraw1Glyph       d1g;
+    SkDraw1Glyph::Proc proc = d1g.init(this, blitter, cache);
     TextMapState       tms(*matrix, constY);
     TextMapState::Proc tmsProc = tms.pickProc(scalarsPerPosition);
 
-    if (paint.isSubpixelText()) {
+    if (cache->isSubpixel()) {
         // maybe we should skip the rounding if linearText is set
         SkAxisAlignment roundBaseline = SkComputeAxisAlignmentForHText(*matrix);
 
@@ -1771,8 +1776,13 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
 
                 tmsProc(tms, pos);
 
+#ifdef SK_DRAW_POS_TEXT_IGNORE_SUBPIXEL_LEFT_ALIGN_FIX
                 SkFixed fx = SkScalarToFixed(tms.fLoc.fX);
                 SkFixed fy = SkScalarToFixed(tms.fLoc.fY);
+#else
+                SkFixed fx = SkScalarToFixed(tms.fLoc.fX) + (SK_FixedHalf >> SkGlyph::kSubBits);
+                SkFixed fy = SkScalarToFixed(tms.fLoc.fY) + (SK_FixedHalf >> SkGlyph::kSubBits);
+#endif
                 SkFixed fxMask = ~0;
                 SkFixed fyMask = ~0;
 
@@ -1807,8 +1817,8 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
                     {
                         SkIPoint fixedLoc;
                         alignProc(tms.fLoc, *glyph, &fixedLoc);
-                        fx = fixedLoc.fX;
-                        fy = fixedLoc.fY;
+                        fx = fixedLoc.fX + (SK_FixedHalf >> SkGlyph::kSubBits);
+                        fy = fixedLoc.fY + (SK_FixedHalf >> SkGlyph::kSubBits);
 
                         if (kX_SkAxisAlignment == roundBaseline) {
                             fyMask = 0;
@@ -1840,8 +1850,10 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
                 SkIPoint fixedLoc;
                 alignProc(tms.fLoc, glyph, &fixedLoc);
 
-                proc(d1g, fixedLoc.fX + SK_FixedHalf,
-                     fixedLoc.fY + SK_FixedHalf, glyph);
+                proc(d1g,
+                     fixedLoc.fX + SK_FixedHalf,
+                     fixedLoc.fY + SK_FixedHalf,
+                     glyph);
             }
             pos += scalarsPerPosition;
         }
diff --git a/src/core/SkDrawProcs.h b/src/core/SkDrawProcs.h
index 74aa9bb01a..33b870d2c8 100644
--- a/src/core/SkDrawProcs.h
+++ b/src/core/SkDrawProcs.h
@@ -14,19 +14,20 @@ class SkAAClip;
 class SkBlitter;
 
 struct SkDraw1Glyph {
-    const SkDraw*   fDraw;
-	SkBounder*		fBounder;
-	const SkRegion*	fClip;
-	const SkAAClip*	fAAClip;
-	SkBlitter*		fBlitter;
-	SkGlyphCache*	fCache;
-	SkIRect			fClipBounds;
-	
-    // The fixed x,y have been pre-rounded (i.e. 1/2 has already been added),
-    // so the impls need just trunc down to an int
-	typedef void (*Proc)(const SkDraw1Glyph&, SkFixed x, SkFixed y, const SkGlyph&);
-	
-	Proc init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache);
+    const SkDraw* fDraw;
+    SkBounder* fBounder;
+    const SkRegion* fClip;
+    const SkAAClip* fAAClip;
+    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.
+    typedef void (*Proc)(const SkDraw1Glyph&, SkFixed x, SkFixed y, const SkGlyph&);
+    
+    Proc init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache);
 };
 
 struct SkDrawProcs {
diff --git a/src/device/xps/SkXPSDevice.cpp b/src/device/xps/SkXPSDevice.cpp
index e2e23fa1ab..84dcb8e9c9 100644
--- a/src/device/xps/SkXPSDevice.cpp
+++ b/src/device/xps/SkXPSDevice.cpp
@@ -2176,9 +2176,15 @@ static void xps_draw_1_glyph(const SkDraw1Glyph& state,
     SkASSERT(skGlyph.fWidth > 0 && skGlyph.fHeight > 0);
     
     SkXPSDrawProcs* procs = static_cast<SkXPSDrawProcs*>(state.fDraw->fProcs);
-    //Draw pre-adds half for floor rounding.
-    x -= SK_FixedHalf;
-    y -= SK_FixedHalf;
+
+    //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;
+    }
     
     XPS_GLYPH_INDEX* xpsGlyph = procs->xpsGlyphs.append();
     uint16_t glyphID = skGlyph.getGlyphID();
diff --git a/tests/DrawTextTest.cpp b/tests/DrawTextTest.cpp
new file mode 100644
index 0000000000..aefe2f9fca
--- /dev/null
+++ b/tests/DrawTextTest.cpp
@@ -0,0 +1,115 @@
+/*

+ * Copyright 2011 Google Inc.

+ *

+ * Use of this source code is governed by a BSD-style license that can be

+ * found in the LICENSE file.

+ */

+#include "SkTypes.h"

+

+#include "Test.h"

+#include "SkBitmap.h"

+#include "SkCanvas.h"

+#include "SkColor.h"

+#include "SkPaint.h"

+#include "SkPoint.h"

+#include "SkRect.h"

+

+///////////////////////////////////////////////////////////////////////////////

+

+static const SkColor bgColor = SK_ColorWHITE;

+

+static void create(SkBitmap* bm, SkIRect bound, SkBitmap::Config config) {

+    bm->setConfig(config, bound.width(), bound.height());

+    bm->allocPixels();

+}

+

+static void drawBG(SkCanvas* canvas) {

+    canvas->drawColor(bgColor);

+}

+

+/** Assumes that the ref draw was completely inside ref canvas --

+    implies that everything outside is "bgColor".

+    Checks that all overlap is the same and that all non-overlap on the

+    ref is "bgColor".

+ */

+static bool compare(const SkBitmap& ref, const SkIRect& iref,

+                    const SkBitmap& test, const SkIRect& itest)

+{

+    const int xOff = itest.fLeft - iref.fLeft;

+    const int yOff = itest.fTop - iref.fTop;

+

+    SkAutoLockPixels alpRef(ref);

+    SkAutoLockPixels alpTest(test);

+

+    for (int y = 0; y < test.height(); ++y) {

+        for (int x = 0; x < test.width(); ++x) {

+            SkColor testColor = test.getColor(x, y);

+            int refX = x + xOff;

+            int refY = y + yOff;

+            SkColor refColor;

+            if (refX >= 0 && refX < ref.width() &&

+                refY >= 0 && refY < ref.height())

+            {

+                refColor = ref.getColor(refX, refY);

+            } else {

+                refColor = bgColor;

+            }

+            if (refColor != testColor) {

+                return false;

+            }

+        }

+    }

+    return true;

+}

+

+static void test_drawText(skiatest::Reporter* reporter) {

+

+    SkPaint paint;

+    paint.setColor(SK_ColorGRAY);

+    paint.setTextSize(SkIntToScalar(20));

+    

+    SkIRect drawTextRect = SkIRect::MakeWH(64, 64);

+    SkBitmap drawTextBitmap;

+    create(&drawTextBitmap, drawTextRect, SkBitmap::kARGB_8888_Config);

+    SkCanvas drawTextCanvas(drawTextBitmap);

+

+    SkIRect drawPosTextRect = SkIRect::MakeWH(64, 64);

+    SkBitmap drawPosTextBitmap;

+    create(&drawPosTextBitmap, drawPosTextRect, SkBitmap::kARGB_8888_Config);

+    SkCanvas drawPosTextCanvas(drawPosTextBitmap);

+

+    for (float offsetY = 0.0f; offsetY < 1.0f; offsetY += (1.0f / 16.0f)) {

+        for (float offsetX = 0.0f; offsetX < 1.0f; offsetX += (1.0f / 16.0f)) {

+            SkPoint point = SkPoint::Make(SkFloatToScalar(25.0f + offsetX),

+                                          SkFloatToScalar(25.0f + offsetY));

+

+            for (int align = 0; align < SkPaint::kAlignCount; ++align) {

+                paint.setTextAlign(static_cast<SkPaint::Align>(align));

+

+                for (unsigned int flags = 0; flags < (1 << 3); ++flags) {

+                    static const unsigned int antiAliasFlag = 1;

+                    static const unsigned int subpixelFlag = 1 << 1;

+                    static const unsigned int lcdFlag = 1 << 2;

+

+                    paint.setAntiAlias(SkToBool(flags & antiAliasFlag));

+                    paint.setSubpixelText(SkToBool(flags & subpixelFlag));

+                    paint.setLCDRenderText(SkToBool(flags & lcdFlag));

+

+                    // Test: drawText and drawPosText draw the same.

+                    drawBG(&drawTextCanvas);

+                    drawTextCanvas.drawText("A", 1, point.fX, point.fY, paint);

+

+                    drawBG(&drawPosTextCanvas);

+                    drawPosTextCanvas.drawPosText("A", 1, &point, paint);

+

+                    REPORTER_ASSERT(reporter,

+                        compare(drawTextBitmap, drawTextRect,

+                                drawPosTextBitmap, drawPosTextRect));

+                }

+            }

+        }

+    }

+}

+

+#include "TestClassDef.h"

+DEFINE_TESTCLASS("DrawText_DrawPosText", DrawTextTestClass, test_drawText)