Fix valgrind warnings triggered in vertical mode.

Now that Chrome is rendering subpixel text, I was able to try running
the renderer process under valgrind, which turned up a number of
issues.

First, I was calculating the stride of vertical LCD glyphs wrong
(typo).

Secondly, I was going horribly wrong when a glyph was being blitted at
the edge of a bitmap. I suspected something was wrong with the code,
but I wasn't clear enough with the structure of the code when writing
it to figure out what the correct solution was.

http://codereview.appspot.com/97059

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

diff --git a/src/core/SkBlitter_ARGB32.cpp b/src/core/SkBlitter_ARGB32.cpp
index 247e255..d3e5714 100644 (file)
--- a/src/core/SkBlitter_ARGB32.cpp
+++ b/src/core/SkBlitter_ARGB32.cpp
@@ -24,6 +24,10 @@
 #if defined(SK_SUPPORT_LCDTEXT)
 namespace skia_blitter_support {
 // subpixel helper functions from SkBlitter_ARGB32_Subpixel.cpp
+uint32_t* adjustForSubpixelClip(const SkMask& mask,
+                                const SkIRect& clip, const SkBitmap& device,
+                                int* widthAdjustment, int* heightAdjustment,
+                                const uint32_t** alpha32);
 extern uint32_t BlendLCDPixelWithColor(const uint32_t alphaPixel, const uint32_t originalPixel,
                                        const uint32_t sourcePixel);
 extern uint32_t BlendLCDPixelWithOpaqueColor(const uint32_t alphaPixel, const uint32_t originalPixel,
@@ -219,22 +223,19 @@ void SkARGB32_Opaque_Blitter::blitMask(const SkMask& mask,
 #if defined(SK_SUPPORT_LCDTEXT)
     const bool      lcdMode = mask.fFormat == SkMask::kHorizontalLCD_Format;
     const bool      verticalLCDMode = mask.fFormat == SkMask::kVerticalLCD_Format;
-#else
-    const bool      lcdMode = false, verticalLCDMode = false;
 #endif
 
     // In LCD mode the masks have either an extra couple of rows or columns on the edges.
-    uint32_t*       device = fDevice.getAddr32(x - lcdMode, y - verticalLCDMode);
     uint32_t        srcColor = fPMColor;
 
 #if defined(SK_SUPPORT_LCDTEXT)
     if (lcdMode || verticalLCDMode) {
-        const uint32_t* alpha32 = mask.getAddrLCD(clip.fLeft, clip.fTop);
+        int widthAdjustment, heightAdjustment;
+        const uint32_t* alpha32;
+        uint32_t* device = adjustForSubpixelClip(mask, clip, fDevice, &widthAdjustment, &heightAdjustment, &alpha32);
 
-        if (lcdMode)
-            width += 2;  // we have extra columns on the left and right
-        else
-            height += 2;  // we have extra rows at the top and bottom
+        width += widthAdjustment;
+        height += heightAdjustment;
 
         unsigned devRB = fDevice.rowBytes() - (width << 2);
         unsigned alphaExtraRowWords = mask.rowWordsLCD() - width;
@@ -254,6 +255,7 @@ void SkARGB32_Opaque_Blitter::blitMask(const SkMask& mask,
     }
 #endif
 
+    uint32_t*      device = fDevice.getAddr32(x, y);
     const uint8_t* alpha = mask.getAddr(x, y);
     unsigned       maskRB = mask.fRowBytes - width;
     unsigned       devRB = fDevice.rowBytes() - (width << 2);
@@ -351,13 +353,9 @@ void SkARGB32_Black_Blitter::blitMask(const SkMask& mask, const SkIRect& clip) {
 #if defined(SK_SUPPORT_LCDTEXT)
         const bool      lcdMode = mask.fFormat == SkMask::kHorizontalLCD_Format;
         const bool      verticalLCDMode = mask.fFormat == SkMask::kVerticalLCD_Format;
-#else
-        const bool      lcdMode = false, verticalLCDMode = false;
 #endif
 
         // In LCD mode the masks have either an extra couple of rows or columns on the edges.
-        uint32_t*       device = fDevice.getAddr32(clip.fLeft - lcdMode,
-                                                   clip.fTop - verticalLCDMode);
         unsigned        width = clip.width();
         unsigned        height = clip.height();
 
@@ -366,11 +364,12 @@ void SkARGB32_Black_Blitter::blitMask(const SkMask& mask, const SkIRect& clip) {
 
 #if defined(SK_SUPPORT_LCDTEXT)
         if (lcdMode || verticalLCDMode) {
-            const uint32_t* alpha32 = mask.getAddrLCD(clip.fLeft, clip.fTop);
-            if (lcdMode)
-                width += 2;  // we have extra columns on the left and right
-            else
-                height += 2;  // we have extra rows at the top and bottom
+            int widthAdjustment, heightAdjustment;
+            const uint32_t* alpha32;
+            uint32_t* device = adjustForSubpixelClip(mask, clip, fDevice, &widthAdjustment, &heightAdjustment, &alpha32);
+
+            width += widthAdjustment;
+            height += heightAdjustment;
 
             unsigned deviceRB = fDevice.rowBytes() - (width << 2);
             unsigned alphaExtraRowWords = mask.rowWordsLCD() - width;
@@ -390,8 +389,9 @@ void SkARGB32_Black_Blitter::blitMask(const SkMask& mask, const SkIRect& clip) {
         }
 #endif
 
-        unsigned maskRB = mask.fRowBytes - width;
-        unsigned deviceRB = fDevice.rowBytes() - (width << 2);
+        uint32_t*      device = fDevice.getAddr32(clip.fLeft, clip.fTop);
+        unsigned       maskRB = mask.fRowBytes - width;
+        unsigned       deviceRB = fDevice.rowBytes() - (width << 2);
         const uint8_t* alpha = mask.getAddr(clip.fLeft, clip.fTop);
         do {
             unsigned w = width;

diff --git a/src/core/SkBlitter_ARGB32_Subpixel.cpp b/src/core/SkBlitter_ARGB32_Subpixel.cpp
index 668ccf5..5d334ae 100644 (file)
--- a/src/core/SkBlitter_ARGB32_Subpixel.cpp
+++ b/src/core/SkBlitter_ARGB32_Subpixel.cpp
@@ -28,10 +28,52 @@
    resulting in a new pixel value.
 */
 
+#include "SkBitmap.h"
 #include "SkColorPriv.h"
+#include "SkMask.h"
+#include "SkRect.h"
 
 namespace skia_blitter_support {
 
+/** Given a clip region which describes the desired location of a glyph and a
+    bitmap to which an LCD glyph is to be blitted, return a pointer to the
+    SkBitmap's pixels and output width and height adjusts for the glyph as well
+    as a pointer into the glyph.
+
+    Recall that LCD glyphs have extra rows (vertical mode) or columns
+    (horizontal mode) at the edges as a result of low-pass filtering. If we
+    wanted to put a glyph on the hard-left edge of bitmap, we would have to know
+    to start one pixel into the glyph, as well as to only add 1 to the recorded
+    glyph width etc. This function encapsulates that behaviour.
+
+    @param mask    The glyph to be blitted.
+    @param clip    The clip region describing the desired location of the glyph.
+    @param device  The SkBitmap target for the blit.
+    @param widthAdjustment  (output) a number to add to the glyph's nominal width.
+    @param heightAdjustment (output) a number to add to the glyph's nominal width.
+    @param alpha32 (output) a pointer into the 32-bit subpixel alpha data for the glyph
+*/
+uint32_t* adjustForSubpixelClip(const SkMask& mask,
+                                const SkIRect& clip, const SkBitmap& device,
+                                int* widthAdjustment, int* heightAdjustment,
+                                const uint32_t** alpha32) {
+    const bool lcdMode = mask.fFormat == SkMask::kHorizontalLCD_Format;
+    const bool verticalLCDMode = mask.fFormat == SkMask::kVerticalLCD_Format;
+    const int  leftOffset = clip.fLeft > 0 ? lcdMode : 0;
+    const int  topOffset = clip.fTop > 0 ? verticalLCDMode : 0;
+    const int  rightOffset = lcdMode && clip.fRight < device.width();
+    const int  bottomOffset = verticalLCDMode && clip.fBottom < device.height();
+
+    uint32_t* device32 = device.getAddr32(clip.fLeft - leftOffset, clip.fTop - topOffset);
+    *alpha32 = mask.getAddrLCD(clip.fLeft + (lcdMode && !leftOffset),
+                               clip.fTop + (verticalLCDMode && !topOffset));
+
+    *widthAdjustment = leftOffset + rightOffset;
+    *heightAdjustment = topOffset + bottomOffset;
+
+    return device32;
+}
+
 uint32_t BlendLCDPixelWithColor(const uint32_t alphaPixel, const uint32_t originalPixel,
                                 const uint32_t sourcePixel) {
     unsigned alphaRed = SkAlpha255To256(SkGetPackedR32(alphaPixel));

diff --git a/src/ports/SkFontHost_FreeType_Subpixel.cpp b/src/ports/SkFontHost_FreeType_Subpixel.cpp
index 2087ba8..bc01585 100644 (file)
--- a/src/ports/SkFontHost_FreeType_Subpixel.cpp
+++ b/src/ports/SkFontHost_FreeType_Subpixel.cpp
@@ -100,7 +100,7 @@ void CopyFreetypeBitmapToVerticalLCDMask(const SkGlyph& dest, const FT_Bitmap& s
     //                     |-------- A single pixel in the output
 
     uint8_t* output = reinterpret_cast<uint8_t*>(dest.fImage);
-    const unsigned outputPitch = SkAlign4(source.rows);
+    const unsigned outputPitch = dest.rowBytes();
     const uint8_t* input = source.buffer;
 
     // First we calculate the A8 mask.