Simplify draw one glyph

src/core/SkDraw.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #define __STDC_LIMIT_MACROS
 
 #include "SkDraw.h"
 #include "SkBlitter.h"
@@ skipping 1405 matching lines @@
     }
 }
 
 // disable warning : local variable used without having been initialized
 #if defined _WIN32 && _MSC_VER >= 1300
 #pragma warning ( push )
 #pragma warning ( disable : 4701 )
 #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&);
+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;
 
-    Proc init(const SkDraw* draw, SkBlitter* blitter, SkGlyphCache* cache,
-              const SkPaint&);
+        if (rasterClip.isBW()) {
+            fClipBounds = rasterClip.bwRgn().getBounds();
+        } else {
+            fClipBounds = rasterClip.aaRgn().getBounds();
+        }
+    }
 
-    // call this instead of fBlitter->blitMask() since this wrapper will handle
-    // the case when the mask is ARGB32_Format
-    //
+    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);
+
+        left += glyph.fLeft;
+        top  += glyph.fTop;
+
+        int right   = left + glyph.fWidth;
+        int bottom  = top  + glyph.fHeight;
+
+        SkMask mask;
+        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);
+
+            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;
+            }
+
+            if (this->getImageData(glyph, &mask)) {
+                this->blitMask(mask, *bounds);
+            }
+        }
+    }
+
+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();
+    }
+
+    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->fImage    = bits;
+        mask->fRowBytes = glyph.rowBytes();
+        mask->fFormat   = static_cast<SkMask::Format>(glyph.fMaskFormat);
+        return true;
+    }
+
     void blitMask(const SkMask& mask, const SkIRect& clip) const {
         if (SkMask::kARGB32_Format == mask.fFormat) {
-            this->blitMaskAsSprite(mask);
+            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);
         } else {
             fBlitter->blitMask(mask, clip);
         }
     }
 
-    // mask must be kARGB32_Format
-    void blitMaskAsSprite(const SkMask& mask) const;
+    const bool            fUseRegionToDraw;
+    SkGlyphCache  * const fGlyphCache;
+    SkBlitter     * const fBlitter;
+    const SkRegion* const fClip;
+    const SkDraw&         fDraw;
+    const SkPaint&        fPaint;
+    SkIRect               fClipBounds;
 };
 
-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;
-    }
-
-    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()));
-
-    left += glyph.fLeft;
-    top  += glyph.fTop;
-
-    int right   = left + glyph.fWidth;
-    int bottom  = top + glyph.fHeight;
-
-    SkMask        mask;
-    SkIRect        storage;
-    SkIRect*    bounds = &mask.fBounds;
-
-    mask.fBounds.set(left, top, right, bottom);
-
-    // 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;
-    }
-
-    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
-        }
-    }
-
-    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);
-
-    if (!clipper.done()) {
-        const SkIRect&  cr = clipper.rect();
-        const uint8_t* aa = (uint8_t*)state.fCache->findImage(glyph);
-        if (nullptr == aa) {
-            return;
-        }
-
-        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());
-    }
-}
-
-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;
-    }
-
-    if (draw->fRC->isBW()) {
-        fAAClip = nullptr;
-        fClip = &draw->fRC->bwRgn();
-        fClipBounds = fClip->getBounds();
-        if (fClip->isRect()) {
-            return D1G_RectClip;
-        } else {
-            return D1G_RgnClip;
-        }
-    } 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);
-}
-
-///////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////////////////////////
 
 void SkDraw::drawText(const char text[], size_t byteLength,
                       SkScalar x, SkScalar y, const SkPaint& paint) const {
     SkASSERT(byteLength == 0 || text != nullptr);
 
     SkDEBUGCODE(this->validate();)
 
     // nothing to draw
     if (text == nullptr || byteLength == 0 || fRC->isEmpty()) {
         return;
     }
 
     // SkScalarRec doesn't currently have a way of representing hairline stroke and
     // will fill if its frame-width is 0.
     if (ShouldDrawTextAsPaths(paint, *fMatrix)) {
         this->drawText_asPaths(text, byteLength, x, y, paint);
         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());
 
-    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);
         }
     );
 }
 
 //////////////////////////////////////////////////////////////////////////////
 
 void SkDraw::drawPosText_asPaths(const char text[], size_t byteLength,
                                  const SkScalar pos[], int scalarsPerPosition,
                                  const SkPoint& offset, const SkPaint& origPaint) const {
     // setup our std paint, in hopes of getting hits in the cache
@@ skipping 53 matching lines @@
     // nothing to draw
     if (text == nullptr || byteLength == 0 || fRC->isEmpty()) {
         return;
     }
 
     if (ShouldDrawTextAsPaths(paint, *fMatrix)) {
         this->drawPosText_asPaths(text, byteLength, pos, scalarsPerPosition, offset, paint);
         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(
         paint.getTextEncoding(), text, 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);
         }
     );
 }
 
 #if defined _WIN32 && _MSC_VER >= 1300
 #pragma warning ( pop )
 #endif
 
 ///////////////////////////////////////////////////////////////////////////////
 
@@ skipping 384 matching lines @@
         mask->fImage = SkMask::AllocImage(size);
         memset(mask->fImage, 0, mask->computeImageSize());
     }
 
     if (SkMask::kJustComputeBounds_CreateMode != mode) {
         draw_into_mask(*mask, devPath, style);
     }
 
     return true;
 }