improve bitmap font support (FreeType only)

src/ports/SkFontHost_FreeType_common.cpp

Index: src/ports/SkFontHost_FreeType_common.cpp
diff --git a/src/ports/SkFontHost_FreeType_common.cpp b/src/ports/SkFontHost_FreeType_common.cpp
index 2c486847b6d8f4e0fe9c0f108707bf5662756e1a..2b5f90d990eee36ba7860baeabbca76e850891ca 100644
--- a/src/ports/SkFontHost_FreeType_common.cpp
+++ b/src/ports/SkFontHost_FreeType_common.cpp
@@ -6,6 +6,9 @@
  * found in the LICENSE file.
  */
 
+#include "SkBitmap.h"
+#include "SkCanvas.h"
+#include "SkColor.h"
 #include "SkColorPriv.h"
 #include "SkFDot6.h"
 #include "SkFontHost_FreeType_common.h"
@@ -60,7 +63,7 @@ static void copyFT2LCD16(const SkGlyph& glyph, const FT_Bitmap& bitmap,
     const uint8_t* src = bitmap.buffer;
 
     switch (bitmap.pixel_mode) {
-        case FT_PIXEL_MODE_MONO: {
+        case FT_PIXEL_MODE_MONO:
             for (int y = 0; y < glyph.fHeight; ++y) {
                 for (int x = 0; x < width; ++x) {
                     dst[x] = -bittst(src, x);
@@ -68,8 +71,8 @@ static void copyFT2LCD16(const SkGlyph& glyph, const FT_Bitmap& bitmap,
                 dst = (uint16_t*)((char*)dst + dstRB);
                 src += bitmap.pitch;
             }
-        } break;
-        case FT_PIXEL_MODE_GRAY: {
+            break;
+        case FT_PIXEL_MODE_GRAY:
             for (int y = 0; y < glyph.fHeight; ++y) {
                 for (int x = 0; x < width; ++x) {
                     dst[x] = grayToRGB16(src[x]);
@@ -77,8 +80,8 @@ static void copyFT2LCD16(const SkGlyph& glyph, const FT_Bitmap& bitmap,
                 dst = (uint16_t*)((char*)dst + dstRB);
                 src += bitmap.pitch;
             }
-        } break;
-        default: {
+            break;
+        default:
             SkASSERT(lcdIsVert || (glyph.fWidth * 3 == bitmap.width));
             for (int y = 0; y < glyph.fHeight; y++) {
                 if (lcdIsVert) {    // vertical stripes
@@ -115,7 +118,124 @@ static void copyFT2LCD16(const SkGlyph& glyph, const FT_Bitmap& bitmap,
                 }
                 dst = (uint16_t*)((char*)dst + dstRB);
             }
-        } break;
+            break;
+    }
+}
+
+// copies an FT_Bitmap's pixel data into a buffer with identical dimensions
+static void copyFTBitmap(const FT_Bitmap& srcFTBitmap, uint8_t* dst, uint8_t dstFormat,

[reed1, 2013/11/13 20:21:46]

Why are we passing uint8_t for the format type, instead of SkMask::Format ?

[reed1, 2013/11/13 20:21:46]

Since we are copying into a bitmap, why not pass in a bitmap?

e.g.

copyFTBitmap(const FT_Bitmap& src, SkBitmap* dst);

The caller seems to already have it on the stack. That would remove the need for
these separate parameters (dst, dstFormat, dstRowBytes).

[violets, 2013/11/19 18:38:06]

SkGlyph::fMaskFormat, the value being passed into this function, is declared as
uint8_t. C/C++ treats enums as strong types, so implicit conversion to
SkMask::Format is not provided by the compiler. Any time we try to feed
SkGlyph::fMaskFormat or SkScalerContext::Rec::fMaskFormat to a function that
expects SkMask::Format, the compiler will throw an error if there is not an
explicit static_cast<SkMask::Format>. That gets a little cluttery, so I figured
it probably best to just work with the data type SkGlyph::fMaskFormat is
actually declared as.

If you'd rather I go the other way and do the necessary casting, I can totally
do that, but I thought I'd verify first since it does get a little messy...

[violets, 2013/11/19 18:38:06]

One of the two calls to this function passes in glyph.fImage, which is a flat
uint8_t buffer. If I alter this function to take an SkBitmap, the second
invocation will require an SkBitmap that currently does not exist be
constructed.

That seems a little heavyweight compared to simply operating on uint8_t buffers,
is that really what we want?

[reed1, 2013/11/20 21:52:12]

I think that may be worth it, allowing us to have this function signature be
much clearer, *especially* since we are dealing with two domains (FT and Skia),
it only helps to be explicit whose format we are talking about.

[reed1, 2013/11/20 21:52:12]

Ah, good point. Creating a bitmap just for that does seem overkill. However,
consider taking SkMask instead, since those are free to put on the stack (unlike
SkBitmap), but that is just a suggestion.

+                         size_t dstRowBytes) {
+    const uint8_t* src = reinterpret_cast<const uint8_t*>(srcFTBitmap.buffer);
+    size_t width = srcFTBitmap.width;
+    size_t height = srcFTBitmap.rows;
+    size_t srcRowBytes = srcFTBitmap.pitch;
+
+    if ((SkMask::kA8_Format == dstFormat &&
+         FT_PIXEL_MODE_GRAY == srcFTBitmap.pixel_mode) ||
+        (SkMask::kBW_Format == dstFormat &&
+         FT_PIXEL_MODE_MONO == srcFTBitmap.pixel_mode)) {
+        // TODO: test 1bpp bitmap font into kBW_Format glyph
+        size_t minRowBytes = SkMin32(srcRowBytes, dstRowBytes);
+        size_t extraRowBytes = dstRowBytes - minRowBytes;
+        for (int y = height - 1; y >= 0; --y) {
+            memcpy(dst, src, minRowBytes);
+            memset(dst + minRowBytes, 0, extraRowBytes);
+            src += srcRowBytes;
+            dst += dstRowBytes;
+        }
+    } else if (SkMask::kA8_Format == dstFormat &&
+               FT_PIXEL_MODE_MONO == srcFTBitmap.pixel_mode) {
+        // TODO: test 1bpp bitmap font into kA8_Format glyph
+        for (size_t y = 0; y < height; ++y) {
+            uint8_t byte = 0;
+            int bits = 0;
+            const uint8_t* src_row = src;
+            uint8_t* dst_row = dst;
+            for (size_t x = 0; x < width; ++x) {
+                if (!bits) {
+                    byte = *src_row++;
+                    bits = 8;
+                }
+                *dst_row++ = byte & 0x80 ? 0xff : 0;
+                bits--;
+                byte <<= 1;
+            }
+            src += srcRowBytes;
+            dst += dstRowBytes;
+        }
+#ifdef FT_LOAD_COLOR
+    } else if (SkMask::kARGB32_Format == dstFormat &&
+               FT_PIXEL_MODE_BGRA == srcFTBitmap.pixel_mode) {
+        size_t minWidth = SkMin32(width, SkMin32(srcRowBytes, dstRowBytes) / 4);
+        size_t extraRowBytes = dstRowBytes - (4 * minWidth);
+        for (size_t y = 0; y < height; ++y) {
+            const uint8_t* src_row = src;
+            uint8_t* dst_row = dst;
+            for (size_t x = 0; x < minWidth; ++x) {
+                uint8_t blue = *src_row++;
+                uint8_t green = *src_row++;
+                uint8_t red = *src_row++;
+                uint8_t alpha = *src_row++;
+                *dst_row++ = red;
+                *dst_row++ = green;
+                *dst_row++ = blue;
+                *dst_row++ = alpha;
+            }
+            memset(dst_row, 0, extraRowBytes);
+            src += srcRowBytes;
+            dst += dstRowBytes;
+        }
+#endif
+    } else {
+        SkDEBUGFAIL("unsupported combination of FT_PIXEL_MODE and SkMask::Format");
+    }
+}
+
+inline uint8_t skFormatForFTPixelMode(char pixel_mode) {

[reed1, 2013/11/13 20:21:46]

1. Shouldn't this return SkMask::Format, instead of uint8_t?
2. What type is pixel_mode in FreeType?

[violets, 2013/11/19 18:38:06]

1. I think that this should return the same type as SkGlyph::fMaskFormat, to
save us a bunch of unnecessary explicit casts.
2. pixel_mode is defined as type "char" in FreeType.

[reed1, 2013/11/20 21:52:12]

I still vote for a named type, at least for the return type., *esp* if FT does
not give us a real typename for its type.

+    switch (pixel_mode) {
+        case FT_PIXEL_MODE_GRAY:
+            return SkMask::kA8_Format;
+        case FT_PIXEL_MODE_MONO:
+            return SkMask::kBW_Format;
+#ifdef FT_LOAD_COLOR
+        case FT_PIXEL_MODE_BGRA:
+            return SkMask::kARGB32_Format;
+#endif
+        default:
+            SkDEBUGFAIL("unsupported FT_PIXEL_MODE");
+            return SkMask::kA8_Format;
+    }
+}
+
+inline SkBitmap::Config skConfigForFTPixelMode(char pixel_mode) {

[reed1, 2013/11/13 20:21:46]

What is the type of pixel_mode? Is there an enum for this in FreeType?

[violets, 2013/11/19 18:38:06]

pixel_mode is type "char". There is an associated enum, FT_Pixel_Mode, but
FreeType never declares variables of that type, so converting between the two
requires explicit casts.

+    switch (pixel_mode) {
+        case FT_PIXEL_MODE_GRAY:
+            return SkBitmap::kA8_Config;
+        case FT_PIXEL_MODE_MONO:
+            return SkBitmap::kA1_Config;
+#ifdef FT_LOAD_COLOR
+        case FT_PIXEL_MODE_BGRA:
+            return SkBitmap::kARGB_8888_Config;
+#endif
+        default:
+            SkDEBUGFAIL("unsupported FT_PIXEL_MODE");
+            return SkBitmap::kA8_Config;
+    }
+}
+
+inline SkBitmap::Config skConfigForFormat(uint8_t format) {

[reed1, 2013/11/13 20:21:46]

Can we declare format to be SkMask::Format ?

[violets, 2013/11/19 18:38:06]

We can, but the uint8_t data that we pass into this function will then need an
explicit cast. Do we still want to do that?

[reed1, 2013/11/20 21:52:12]

I do.

I'm also fine if we had a helper to SkGlyph...

SkMask::Format format() const { return (SkMask::Format)fFormat; }

+    switch (format) {
+        case SkMask::kA8_Format:
+            return SkBitmap::kA8_Config;
+        case SkMask::kBW_Format:
+            return SkBitmap::kA1_Config;
+#ifdef FT_LOAD_COLOR
+        case SkMask::kARGB32_Format:
+            return SkBitmap::kARGB_8888_Config;
+#endif
+        default:
+            SkDEBUGFAIL("unsupported FT_PIXEL_MODE");
+            return SkBitmap::kA8_Config;
     }
 }
 
@@ -124,107 +244,74 @@ void SkScalerContext_FreeType_Base::generateGlyphImage(FT_Face face, const SkGly
     const bool doVert = SkToBool(fRec.fFlags & SkScalerContext::kLCD_Vertical_Flag);
 
     switch ( face->glyph->format ) {
-        case FT_GLYPH_FORMAT_OUTLINE: {
-            FT_Outline* outline = &face->glyph->outline;
-            FT_BBox     bbox;
-            FT_Bitmap   target;
-
-            if (fRec.fFlags & SkScalerContext::kEmbolden_Flag) {
-                emboldenOutline(face, outline);
-            }
-
-            int dx = 0, dy = 0;
-            if (fRec.fFlags & SkScalerContext::kSubpixelPositioning_Flag) {
-                dx = SkFixedToFDot6(glyph.getSubXFixed());
-                dy = SkFixedToFDot6(glyph.getSubYFixed());
-                // negate dy since freetype-y-goes-up and skia-y-goes-down
-                dy = -dy;
-            }
-            FT_Outline_Get_CBox(outline, &bbox);
-            /*
-                what we really want to do for subpixel is
-                    offset(dx, dy)
-                    compute_bounds
-                    offset(bbox & !63)
-                but that is two calls to offset, so we do the following, which
-                achieves the same thing with only one offset call.
-            */
-            FT_Outline_Translate(outline, dx - ((bbox.xMin + dx) & ~63),
-                                          dy - ((bbox.yMin + dy) & ~63));
+        case FT_GLYPH_FORMAT_OUTLINE:
+            {

[bungeman-skia, 2013/11/15 00:19:48]

This introduces an unnecessary amount of indentation which isn't actually
helping much. The '{' should go after the ':' on the previous line and the body
un-indented one level.

[violets, 2013/11/19 18:38:06]

Done.

+                FT_Outline* outline = &face->glyph->outline;
+                FT_BBox     bbox;
+                FT_Bitmap   target;
+
+                if (fRec.fFlags & SkScalerContext::kEmbolden_Flag &&
+                    !(face->style_flags & FT_STYLE_FLAG_BOLD)) {
+                    emboldenOutline(face, outline);
+                }
 
-            if (SkMask::kLCD16_Format == glyph.fMaskFormat) {
-                FT_Render_Glyph(face->glyph, doVert ? FT_RENDER_MODE_LCD_V : FT_RENDER_MODE_LCD);
-                if (fPreBlend.isApplicable()) {
-                    copyFT2LCD16<true>(glyph, face->glyph->bitmap, doBGR, doVert,
-                                       fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
+                int dx = 0, dy = 0;
+                if (fRec.fFlags & SkScalerContext::kSubpixelPositioning_Flag) {
+                    dx = SkFixedToFDot6(glyph.getSubXFixed());
+                    dy = SkFixedToFDot6(glyph.getSubYFixed());
+                    // negate dy since freetype-y-goes-up and skia-y-goes-down
+                    dy = -dy;
+                }
+                FT_Outline_Get_CBox(outline, &bbox);
+                /*
+                    what we really want to do for subpixel is
+                        offset(dx, dy)
+                        compute_bounds
+                        offset(bbox & !63)
+                    but that is two calls to offset, so we do the following, which
+                    achieves the same thing with only one offset call.
+                */
+                FT_Outline_Translate(outline, dx - ((bbox.xMin + dx) & ~63),
+                                              dy - ((bbox.yMin + dy) & ~63));
+
+                if (SkMask::kLCD16_Format == glyph.fMaskFormat) {
+                    FT_Render_Glyph(face->glyph,
+                                    doVert ? FT_RENDER_MODE_LCD_V : FT_RENDER_MODE_LCD);
+                    if (fPreBlend.isApplicable()) {
+                        copyFT2LCD16<true>(glyph, face->glyph->bitmap, doBGR, doVert,
+                                           fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
+                    } else {
+                        copyFT2LCD16<false>(glyph, face->glyph->bitmap, doBGR, doVert,
+                                            fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
+                    }
                 } else {
-                    copyFT2LCD16<false>(glyph, face->glyph->bitmap, doBGR, doVert,
-                                        fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
+                    target.width = glyph.fWidth;
+                    target.rows = glyph.fHeight;
+                    target.pitch = glyph.rowBytes();
+                    target.buffer = reinterpret_cast<uint8_t*>(glyph.fImage);
+                    target.pixel_mode = compute_pixel_mode(
+                                                    (SkMask::Format)fRec.fMaskFormat);
+                    target.num_grays = 256;
+
+                    memset(glyph.fImage, 0, glyph.rowBytes() * glyph.fHeight);
+                    FT_Outline_Get_Bitmap(face->glyph->library, outline, &target);
                 }
-            } else {
-                target.width = glyph.fWidth;
-                target.rows = glyph.fHeight;
-                target.pitch = glyph.rowBytes();
-                target.buffer = reinterpret_cast<uint8_t*>(glyph.fImage);
-                target.pixel_mode = compute_pixel_mode(
-                                                (SkMask::Format)fRec.fMaskFormat);
-                target.num_grays = 256;
-
-                memset(glyph.fImage, 0, glyph.rowBytes() * glyph.fHeight);
-                FT_Outline_Get_Bitmap(face->glyph->library, outline, &target);
             }
-        } break;
+            break;
 
-        case FT_GLYPH_FORMAT_BITMAP: {
-            if (fRec.fFlags & SkScalerContext::kEmbolden_Flag) {
+        case FT_GLYPH_FORMAT_BITMAP:
+            if (fRec.fFlags & SkScalerContext::kEmbolden_Flag &&
+                !(face->style_flags & FT_STYLE_FLAG_BOLD)) {
                 FT_GlyphSlot_Own_Bitmap(face->glyph);
                 FT_Bitmap_Embolden(face->glyph->library, &face->glyph->bitmap, kBitmapEmboldenStrength, 0);
             }
-            SkASSERT_CONTINUE(glyph.fWidth == face->glyph->bitmap.width);
-            SkASSERT_CONTINUE(glyph.fHeight == face->glyph->bitmap.rows);
-            SkASSERT_CONTINUE(glyph.fTop == -face->glyph->bitmap_top);
-            SkASSERT_CONTINUE(glyph.fLeft == face->glyph->bitmap_left);
-
-            const uint8_t*  src = (const uint8_t*)face->glyph->bitmap.buffer;
-            uint8_t*        dst = (uint8_t*)glyph.fImage;
-
-            if (face->glyph->bitmap.pixel_mode == FT_PIXEL_MODE_GRAY ||
-                (face->glyph->bitmap.pixel_mode == FT_PIXEL_MODE_MONO &&
-                 glyph.fMaskFormat == SkMask::kBW_Format)) {
-                unsigned    srcRowBytes = face->glyph->bitmap.pitch;
-                unsigned    dstRowBytes = glyph.rowBytes();
-                unsigned    minRowBytes = SkMin32(srcRowBytes, dstRowBytes);
-                unsigned    extraRowBytes = dstRowBytes - minRowBytes;
-
-                for (int y = face->glyph->bitmap.rows - 1; y >= 0; --y) {
-                    memcpy(dst, src, minRowBytes);
-                    memset(dst + minRowBytes, 0, extraRowBytes);
-                    src += srcRowBytes;
-                    dst += dstRowBytes;
-                }
-            } else if (face->glyph->bitmap.pixel_mode == FT_PIXEL_MODE_MONO &&
-                       glyph.fMaskFormat == SkMask::kA8_Format) {
-                for (int y = 0; y < face->glyph->bitmap.rows; ++y) {
-                    uint8_t byte = 0;
-                    int bits = 0;
-                    const uint8_t* src_row = src;
-                    uint8_t* dst_row = dst;
-
-                    for (int x = 0; x < face->glyph->bitmap.width; ++x) {
-                        if (!bits) {
-                            byte = *src_row++;
-                            bits = 8;
-                        }
-
-                        *dst_row++ = byte & 0x80 ? 0xff : 0;
-                        bits--;
-                        byte <<= 1;
-                    }
 
-                    src += face->glyph->bitmap.pitch;
-                    dst += glyph.rowBytes();
-                }
-            } else if (SkMask::kLCD16_Format == glyph.fMaskFormat) {
+            if (SkMask::kLCD16_Format == glyph.fMaskFormat) {
+                // special-case kLCD16_Format - no scaling currently supported
+                SkASSERT_CONTINUE(glyph.fWidth == face->glyph->bitmap.width);
+                SkASSERT_CONTINUE(glyph.fHeight == face->glyph->bitmap.rows);
+                SkASSERT_CONTINUE(glyph.fTop == -face->glyph->bitmap_top);
+                SkASSERT_CONTINUE(glyph.fLeft == face->glyph->bitmap_left);
                 if (fPreBlend.isApplicable()) {
                     copyFT2LCD16<true>(glyph, face->glyph->bitmap, doBGR, doVert,
                                        fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
@@ -233,14 +320,48 @@ void SkScalerContext_FreeType_Base::generateGlyphImage(FT_Face face, const SkGly
                                         fPreBlend.fR, fPreBlend.fG, fPreBlend.fB);
                 }
             } else {
-                SkDEBUGFAIL("unknown glyph bitmap transform needed");
+                if (glyph.fWidth != face->glyph->bitmap.width ||
+                    glyph.fHeight != face->glyph->bitmap.rows ||
+                    glyph.fTop != -face->glyph->bitmap_top ||
+                    glyph.fLeft != face->glyph->bitmap_left) {
+                    // glyph image needs scaling
+                    // start by copying FT2 image into an SkBitmap
+                    SkBitmap unscaledBitmap;
+                    unscaledBitmap.setConfig(skConfigForFTPixelMode(face->glyph->bitmap.pixel_mode),
+                                             face->glyph->bitmap.width,
+                                             face->glyph->bitmap.rows);
+                    unscaledBitmap.allocPixels();
+                    copyFTBitmap(face->glyph->bitmap,
+                                 reinterpret_cast<uint8_t*>(unscaledBitmap.getPixels()),
+                                 skFormatForFTPixelMode(face->glyph->bitmap.pixel_mode),
+                                 unscaledBitmap.rowBytes());
+                    // wrap the destination SkGlyph's image data into a bitmap
+                    SkBitmap dstBitmap;
+                    dstBitmap.setConfig(skConfigForFormat(glyph.fMaskFormat),
+                                glyph.fWidth, glyph.fHeight, glyph.rowBytes());
+                    dstBitmap.setPixels(glyph.fImage);
+                    // scale unscaledBitmap into dstBitmap
+                    SkCanvas canvas(dstBitmap);
+                    canvas.clear(SK_ColorTRANSPARENT);
+                    canvas.scale(SkIntToScalar(glyph.fWidth)
+                                / SkIntToScalar(face->glyph->bitmap.width),
+                                SkIntToScalar(glyph.fHeight)
+                                / SkIntToScalar(face->glyph->bitmap.rows));
+                    SkPaint paint;
+                    paint.setFilterLevel(SkPaint::kLow_FilterLevel);
+                    canvas.drawBitmap(unscaledBitmap, 0, 0, &paint);
+                } else {
+                    // no scaling needed - directly copy glyph data
+                    copyFTBitmap(face->glyph->bitmap, reinterpret_cast<uint8_t*>(glyph.fImage),
+                                 glyph.fMaskFormat, glyph.rowBytes());
+                }
             }
-        } break;
+            break;
 
-    default:
-        SkDEBUGFAIL("unknown glyph format");
-        memset(glyph.fImage, 0, glyph.rowBytes() * glyph.fHeight);
-        return;
+        default:
+            SkDEBUGFAIL("unknown glyph format");
+            memset(glyph.fImage, 0, glyph.rowBytes() * glyph.fHeight);
+            return;
     }
 
 // We used to always do this pre-USE_COLOR_LUMINANCE, but with colorlum,