Simplify draw one glyph

src/core/SkDraw.cpp

Index: src/core/SkDraw.cpp
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index e1a49d39b4a582265dda15c430ffbf95578bc297..a921d7f5cae243d4f81dc352fe1ddbd37453e6bb 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -1423,170 +1423,117 @@ void SkDraw::drawText_asPaths(const char text[], size_t byteLength,
 #endif
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
-struct SkDraw1Glyph {
-    const SkDraw* fDraw;
-    const SkRegion* fClip;
-    const SkAAClip* fAAClip;
-    SkBlitter* fBlitter;
-    SkGlyphCache* fCache;
-    const SkPaint* fPaint;
-    SkIRect fClipBounds;
-    /** Half the sampling frequency of the rasterized glyph in x. */
-    SkScalar fHalfSampleX;
-    /** Half the sampling frequency of the rasterized glyph in y. */
-    SkScalar 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&, Sk48Dot16 x, Sk48Dot16 y, const SkGlyph&);
-
-    Proc init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache,
-              const SkPaint&);
-
-    // call this instead of fBlitter->blitMask() since this wrapper will handle
-    // the case when the mask is ARGB32_Format
-    //
-    void blitMask(const SkMask& mask, const SkIRect& clip) const {
-        if (SkMask::kARGB32_Format == mask.fFormat) {
-            this->blitMaskAsSprite(mask);
+
+class DrawOneGlyph {
+public:
+    DrawOneGlyph(const SkDraw& draw, const SkPaint& paint, SkGlyphCache* cache,
+                 SkBlitter* blitter)

[bungeman-skia, 2015/11/30 21:42:29]

nit: Will this fit on the previous line? It looks really close...

[herb_g, 2015/11/30 22:11:43]

Done.

+        : fUseRegionToDraw(UseRegionDrawOneGlyph(draw.fRC))
+        , fGlyphCache(cache)
+        , fBlitter(blitter)
+        , fClip(fUseRegionToDraw ? &draw.fRC->bwRgn() : nullptr)
+        , fDraw(draw)
+        , fPaint(paint) {
+        const SkRasterClip& rasterClip = *draw.fRC;
+
+        if (rasterClip.isBW()) {
+            fClipBounds = rasterClip.bwRgn().getBounds();
         } else {
-            fBlitter->blitMask(mask, clip);
+            fClipBounds = rasterClip.aaRgn().getBounds();
         }
     }
 
-    // mask must be kARGB32_Format
-    void blitMaskAsSprite(const SkMask& mask) const;
-};
-
-static void D1G_RectClip(const SkDraw1Glyph& state, Sk48Dot16 fx, Sk48Dot16 fy,
-                         const SkGlyph& glyph) {
-    // Prevent glyphs from being drawn outside of or straddling the edge of device space.
-    if ((fx >> 16) > INT_MAX - (INT16_MAX + UINT16_MAX) ||
-        (fx >> 16) < INT_MIN - (INT16_MIN + 0 /*UINT16_MIN*/) ||
-        (fy >> 16) > INT_MAX - (INT16_MAX + UINT16_MAX) ||
-        (fy >> 16) < INT_MIN - (INT16_MIN + 0 /*UINT16_MIN*/))
-    {
-        return;
-    }
+    void drawOneGlyph(const SkGlyph& glyph, SkPoint position) {

[bungeman-skia, 2015/11/30 21:42:29]

Is there a reason not to make this

void operator()(const SkGlyph& glyph, SkPoint position)

it looks strange that everywhere this is called it looks like
'drawOneGlyph.drawOneGlyph'.

[herb_g, 2015/11/30 22:11:43]

Done - made it obey the FindAndPlaceGlyph interface so no lambda is needed.

+        Sk48Dot16 fx = SkScalarTo48Dot16(position.fX);
+        Sk48Dot16 fy = SkScalarTo48Dot16(position.fY);
+        // Prevent glyphs from being drawn outside of or straddling the edge of device space.
+        if ((fx >> 16) > INT_MAX - (INT16_MAX + UINT16_MAX) ||
+            (fx >> 16) < INT_MIN - (INT16_MIN + 0 /*UINT16_MIN*/) ||
+            (fy >> 16) > INT_MAX - (INT16_MAX + UINT16_MAX) ||
+            (fy >> 16) < INT_MIN - (INT16_MIN + 0 /*UINT16_MIN*/)) {
+            return;
+        }
 
-    int left = Sk48Dot16FloorToInt(fx);
-    int top = Sk48Dot16FloorToInt(fy);
-    SkASSERT(glyph.fWidth > 0 && glyph.fHeight > 0);
-    SkASSERT((nullptr == state.fClip && state.fAAClip) ||
-             (state.fClip && nullptr == state.fAAClip && state.fClip->isRect()));
+        int left = Sk48Dot16FloorToInt(fx);
+        int top  = Sk48Dot16FloorToInt(fy);
+        SkASSERT(glyph.fWidth > 0 && glyph.fHeight > 0);
 
-    left += glyph.fLeft;
-    top  += glyph.fTop;
+        left += glyph.fLeft;
+        top  += glyph.fTop;
 
-    int right   = left + glyph.fWidth;
-    int bottom  = top + glyph.fHeight;
+        int right   = left + glyph.fWidth;
+        int bottom  = top  + glyph.fHeight;
 
-    SkMask        mask;
-    SkIRect        storage;
-    SkIRect*    bounds = &mask.fBounds;
+        SkMask mask;
+        mask.fBounds.set(left, top, right, bottom);
 
-    mask.fBounds.set(left, top, right, bottom);
+        if (fUseRegionToDraw) {

[bungeman-skia, 2015/11/30 21:42:29]

So essentially, this is trading off a predictable call through a function
pointer with a predictable branch?

[herb_g, 2015/11/30 22:11:43]

That is one of two reasons. The second is that they share much of the setup
code.

+            SkRegion::Cliperator clipper(*fClip, mask.fBounds);
 
-    // this extra test is worth it, assuming that most of the time it succeeds
-    // since we can avoid writing to storage
-    if (!state.fClipBounds.containsNoEmptyCheck(left, top, right, bottom)) {
-        if (!storage.intersectNoEmptyCheck(mask.fBounds, state.fClipBounds))
-            return;
-        bounds = &storage;
-    }
+            if (!clipper.done() && this->getImageData(glyph, &mask)) {
+                const SkIRect& cr = clipper.rect();
+                do {
+                    this->blitMask(mask, cr);
+                    clipper.next();
+                } while (!clipper.done());
+            }
+        } else {
+            SkIRect  storage;
+            SkIRect* bounds = &mask.fBounds;
+
+            // this extra test is worth it, assuming that most of the time it succeeds
+            // since we can avoid writing to storage
+            if (!fClipBounds.containsNoEmptyCheck(mask.fBounds)) {
+                if (!storage.intersectNoEmptyCheck(mask.fBounds, fClipBounds))
+                    return;
+                bounds = &storage;
+            }
 
-    uint8_t* aa = (uint8_t*)glyph.fImage;
-    if (nullptr == aa) {
-        aa = (uint8_t*)state.fCache->findImage(glyph);
-        if (nullptr == aa) {
-            return; // can't rasterize glyph
+            if (this->getImageData(glyph, &mask)) {
+                this->blitMask(mask, *bounds);
+            }
         }
     }
 
-    mask.fRowBytes = glyph.rowBytes();
-    mask.fFormat = static_cast<SkMask::Format>(glyph.fMaskFormat);
-    mask.fImage = aa;
-    state.blitMask(mask, *bounds);
-}
-
-static void D1G_RgnClip(const SkDraw1Glyph& state, Sk48Dot16 fx, Sk48Dot16 fy,
-                        const SkGlyph& glyph) {
-    int left = Sk48Dot16FloorToInt(fx);
-    int top = Sk48Dot16FloorToInt(fy);
-    SkASSERT(glyph.fWidth > 0 && glyph.fHeight > 0);
-    SkASSERT(!state.fClip->isRect());
-
-    SkMask  mask;
-
-    left += glyph.fLeft;
-    top  += glyph.fTop;
-
-    mask.fBounds.set(left, top, left + glyph.fWidth, top + glyph.fHeight);
-    SkRegion::Cliperator clipper(*state.fClip, mask.fBounds);
+private:
+    static bool UseRegionDrawOneGlyph(const SkRasterClip* rClip) {

[bungeman-skia, 2015/11/30 21:42:29]

Can this just be named 'UseRegionToDraw' so that it is named similar to what it
initializes?

Also, if we have this one, why not a 'SkIRect ClipBounds(const SkRasterClip*
rClip)' so that fClipBounds can also be marked const?

[herb_g, 2015/11/30 22:11:44]

Done.

+        return rClip->isBW() && !rClip->isRect();
+    }
 
-    if (!clipper.done()) {
-        const SkIRect&  cr = clipper.rect();
-        const uint8_t* aa = (uint8_t*)state.fCache->findImage(glyph);
-        if (nullptr == aa) {
-            return;
+    bool getImageData(const SkGlyph& glyph, SkMask* mask) {
+        uint8_t* bits = (uint8_t*)(fGlyphCache->findImage(glyph));
+        if (nullptr == bits) {
+            return false;  // can't rasterize glyph
         }
-
-        mask.fRowBytes = glyph.rowBytes();
-        mask.fFormat = static_cast<SkMask::Format>(glyph.fMaskFormat);
-        mask.fImage = (uint8_t*)aa;
-        do {
-            state.blitMask(mask, cr);
-            clipper.next();
-        } while (!clipper.done());
+        mask->fImage    = bits;
+        mask->fRowBytes = glyph.rowBytes();
+        mask->fFormat   = static_cast<SkMask::Format>(glyph.fMaskFormat);
+        return true;
     }
-}
-
-SkDraw1Glyph::Proc SkDraw1Glyph::init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache,
-                                      const SkPaint& pnt) {
-    fDraw = draw;
-    fBlitter = blitter;
-    fCache = cache;
-    fPaint = &pnt;
 
-    if (cache->isSubpixel()) {
-        fHalfSampleX = fHalfSampleY = SkFixedToScalar(SkGlyph::kSubpixelRound);
-    } else {
-        fHalfSampleX = fHalfSampleY = SK_ScalarHalf;
-    }
+    void blitMask(const SkMask& mask, const SkIRect& clip) const {
+        if (SkMask::kARGB32_Format == mask.fFormat) {
+            SkBitmap bm;
+            bm.installPixels(
+                SkImageInfo::MakeN32Premul(mask.fBounds.width(), mask.fBounds.height()),
+                (SkPMColor*)mask.fImage, mask.fRowBytes);
 
-    if (draw->fRC->isBW()) {
-        fAAClip = nullptr;
-        fClip = &draw->fRC->bwRgn();
-        fClipBounds = fClip->getBounds();
-        if (fClip->isRect()) {
-            return D1G_RectClip;
+            fDraw.drawSprite(bm, mask.fBounds.x(), mask.fBounds.y(), fPaint);
         } else {
-            return D1G_RgnClip;
+            fBlitter->blitMask(mask, clip);
         }
-    } else {    // aaclip
-        fAAClip = &draw->fRC->aaRgn();
-        fClip = nullptr;
-        fClipBounds = fAAClip->getBounds();
-        return D1G_RectClip;
     }
-}
 
-void SkDraw1Glyph::blitMaskAsSprite(const SkMask& mask) const {
-    SkASSERT(SkMask::kARGB32_Format == mask.fFormat);
-
-    SkBitmap bm;
-    bm.installPixels(SkImageInfo::MakeN32Premul(mask.fBounds.width(), mask.fBounds.height()),
-                     (SkPMColor*)mask.fImage, mask.fRowBytes);
-
-    fDraw->drawSprite(bm, mask.fBounds.x(), mask.fBounds.y(), *fPaint);
-}
+    const bool            fUseRegionToDraw;
+    SkGlyphCache  * const fGlyphCache;
+    SkBlitter     * const fBlitter;
+    const SkRegion* const fClip;
+    const SkDraw&         fDraw;
+    const SkPaint&        fPaint;
+    SkIRect               fClipBounds;
+};
 
-///////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////////////////////////
 
 void SkDraw::drawText(const char text[], size_t byteLength,
                       SkScalar x, SkScalar y, const SkPaint& paint) const {
@@ -1609,20 +1556,18 @@ void SkDraw::drawText(const char text[], size_t byteLength,
     SkAutoGlyphCache    autoCache(paint, &fDevice->surfaceProps(), fMatrix);
     SkGlyphCache*       cache = autoCache.getCache();
 
-
     // The Blitter Choose needs to be live while using the blitter below.
     SkAutoBlitterChoose    blitterChooser(fDst, *fMatrix, paint);
     SkAAClipBlitterWrapper wrapper(*fRC, blitterChooser.get());
 
-    SkDraw1Glyph        d1g;
-    SkDraw1Glyph::Proc  proc = d1g.init(this, wrapper.getBlitter(), cache, paint);
+    DrawOneGlyph       drawOneGlyph(*this, paint, cache, wrapper.getBlitter());

[bungeman-skia, 2015/11/30 21:42:29]

nit: Any reason for the spacing here? It seems to come from the spacing where
the same line is used below, but here it doesn't line up with anything, so it
looks a little odd.

[herb_g, 2015/11/30 22:11:43]

Done.

 
     SkFindAndPlaceGlyph::ProcessText(
         paint.getTextEncoding(), text, byteLength,
         {x, y}, *fMatrix, paint.getTextAlign(), cache,
         [&](const SkGlyph& glyph, SkPoint position, SkPoint rounding) {
             position += rounding;
-            proc(d1g, SkScalarTo48Dot16(position.fX), SkScalarTo48Dot16(position.fY), glyph);
+            drawOneGlyph.drawOneGlyph(glyph, position);
         }
     );
 }
@@ -1696,14 +1641,15 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
         return;
     }
 
+    SkAutoGlyphCache   autoCache(paint, &fDevice->surfaceProps(), fMatrix);
+    SkGlyphCache*      cache = autoCache.getCache();
+
     // The Blitter Choose needs to be live while using the blitter below.
     SkAutoBlitterChoose    blitterChooser(fDst, *fMatrix, paint);
     SkAAClipBlitterWrapper wrapper(*fRC, blitterChooser.get());
 
-    SkAutoGlyphCache   autoCache(paint, &fDevice->surfaceProps(), fMatrix);
-    SkGlyphCache*      cache = autoCache.getCache();
-    SkDraw1Glyph       d1g;
-    SkDraw1Glyph::Proc proc = d1g.init(this, wrapper.getBlitter(), cache, paint);
+    DrawOneGlyph       drawOneGlyph(*this, paint, cache, wrapper.getBlitter());
+
     SkPaint::Align     textAlignment = paint.getTextAlign();
 
     SkFindAndPlaceGlyph::ProcessPosText(
@@ -1711,7 +1657,7 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
         offset, *fMatrix, pos, scalarsPerPosition, textAlignment, cache,
         [&](const SkGlyph& glyph, SkPoint position, SkPoint rounding) {
             position += rounding;
-            proc(d1g, SkScalarTo48Dot16(position.fX), SkScalarTo48Dot16(position.fY), glyph);
+            drawOneGlyph.drawOneGlyph(glyph, position);
         }
     );
 }