Refactor glyph selection and positioning

src/core/SkDraw.cpp

Index: src/core/SkDraw.cpp
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index d8d6fcf57fe123b7926eae12b668114c600e94dc..5da393f6b3ecbe1581cb8c9be240306ea51aad1e 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -14,6 +14,7 @@
 #include "SkDeviceLooper.h"
 #include "SkFixed.h"
 #include "SkMaskFilter.h"
+#include "SkMatrix.h"
 #include "SkPaint.h"
 #include "SkPathEffect.h"
 #include "SkRasterClip.h"
@@ -25,8 +26,10 @@
 #include "SkString.h"
 #include "SkStroke.h"
 #include "SkStrokeRec.h"
+#include "SkTemplates.h"
 #include "SkTextMapStateProc.h"
 #include "SkTLazy.h"
+#include "SkUtility.h"
 #include "SkUtils.h"
 #include "SkVertState.h"
 
@@ -1439,7 +1442,7 @@ void SkDraw::drawText_asPaths(const char text[], size_t byteLength,
 #pragma warning ( disable : 4701 )
 #endif
 
-//////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////////////////////////
 
 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.
@@ -1726,6 +1729,474 @@ void SkDraw::drawPosText_asPaths(const char text[], size_t byteLength,
     }
 }
 
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// Calculate a type with the same size and alignment as a list of types Ts.

[mtklein, 2015/11/04 17:54:11]

You might just drop the AndAlignment on these names.  It's aligned enough, but
doesn't have the same alignment as Ts.

[herb_g, 2015/11/04 21:58:42]

Done.

+template <typename... Ts> struct MaxSizeOf;
+
+template <> struct MaxSizeOf<> { static const size_t value = 0; };
+
+template <typename H, typename... Ts> struct MaxSizeOf<H, Ts...> {
+    static const size_t value =
+        sizeof(H) >= MaxSizeOf<Ts...>::value ? sizeof(H) : MaxSizeOf<Ts...>::value;
+};
+
+template <typename... Ts> struct SameSizeAndAlignmentAs {
+private:
+    SkAlignedSStorage<MaxSizeOf<Ts...>::value> unused;

[mtklein, 2015/11/04 17:54:11]

Even

template <typename... Ts>
using SameSizeAndAlignmentAs = SkAlignedSStorage<MaxSizeOf<Ts...>::value>;

[herb_g, 2015/11/04 21:58:41]

I just embeded SkAlignedSStorage<MaxSizeOf<Ts...>::value> into UntaggedVariant.

+};
+
+///////////////////////////////////////////////////////////////////////////////////////////////////
+// UntaggedVariant is a pile of memory that can hold one of the Ts. It provides a way
+// to initialize that memory in a typesafe way.
+template <typename... Ts>
+class UntaggedVariant {

[mtklein, 2015/11/04 17:54:12]

I'm not sure I see why SameSizeAndAlignmentAs is separate from UntaggedVariant,
or UntaggedVariant is separate from PolymorphicVariant.  Seems like things would
be clearer if we just merged them all together into PolymorphicVariant.

[herb_g, 2015/11/04 21:58:42]

I agree with the SameSizeAndAlignmentAs. This is from code I previously wrote,
and it was used in a couple of places. UntaggedVariant is passed to the
PolymorphicVariant Initialize function to allow manual initialization of raw
memory.

+public:
+    UntaggedVariant() {}
+    ~UntaggedVariant() {}
+    UntaggedVariant(const UntaggedVariant&) = delete;
+    UntaggedVariant& operator=(const UntaggedVariant&) = delete;
+    UntaggedVariant(UntaggedVariant&&) = delete;
+    UntaggedVariant& operator=(UntaggedVariant&&) = delete;
+    template <typename Variant, typename... Args>

[mtklein, 2015/11/04 17:54:11]

It'd be nice to add a newline before template.

[herb_g, 2015/11/04 21:58:42]

Done.

+    void Init(Args&&... args) {

[mtklein, 2015/11/04 17:54:11]

-> init

+        SkASSERT(sizeof(Variant) <= sizeof(fSpace));
+        #ifndef SK_BUILD_FOR_WIN32

[mtklein, 2015/11/04 17:54:11]

Alternatively, I think we could do

#if defined(_MSC_VER) && _MSC_VER < 1900
  #define alignof __alignof
#endif

... use alignof() normally ...

Might be nice to back off the #ifndef and #endif one tab stop left?

[herb_g, 2015/11/04 21:58:42]

Done.

+        SkASSERT(alignof(Variant) <= alignof(SameSizeAndAlignmentAs<Ts...>));
+        #endif
+        new (&fSpace) Variant(skstd::forward<Args>(args)...);
+    }
+
+private:
+    SameSizeAndAlignmentAs<Ts...> fSpace;
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// PolymorphicVariant holds supclasses of Base without slicing. Ts must be subclasses of Base.

[mtklein, 2015/11/04 17:54:12]

sup -> sub

[herb_g, 2015/11/04 21:58:42]

Done.

+template <typename Base, typename... Ts>
+class PolymorphicVariant {
+public:
+    typedef UntaggedVariant<Ts...> Variants;
+    template <typename Initializer>
+    PolymorphicVariant(Initializer&& initializer) {
+        skstd::forward<Initializer>(initializer)(&fVariants);

[mtklein, 2015/11/04 17:54:12]

What's the deal with forward here?  We don't seem to be forwarding it anywhere. 
Can we not just 

PolymorphicVariant(Initializer&& initializer) {
   initializer(&fVariants);
}

?

[herb_g, 2015/11/04 21:58:42]

Forgotten experiment.

+    }
+    ~PolymorphicVariant() { get()->~Base(); }
+    Base* get() const { return (Base*) &fVariants; }

[mtklein, 2015/11/04 17:54:11]

This seems like a spot where reinterpret_cast<> would make things a bit clearer.

[herb_g, 2015/11/04 21:58:41]

Done.

+    Base* operator->() const { return get(); }
+    Base& operator*() const { return *get(); }
+
+private:
+    Variants fVariants;
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// PositionReaderInterface read a point from the pos vector.

[mtklein, 2015/11/04 17:54:11]

reads

[herb_g, 2015/11/04 21:58:42]

Done.

+// * LinearPositions - assumes a common Y for many X values.
+// * ArbitraryPositions - a list of (X,Y) pairs.
+class PositionReaderInterface : SkNoncopyable {
+public:
+    virtual ~PositionReaderInterface() { }
+    virtual const SkPoint NextPoint() = 0;

[mtklein, 2015/11/04 17:54:11]

nextPoint()

[herb_g, 2015/11/04 21:58:42]

Done.

+};
+
+class LinearPositions final : public PositionReaderInterface {

[mtklein, 2015/11/04 17:54:12]

Even, HorizontalPositions.

[herb_g, 2015/11/04 21:58:43]

Done.

+public:
+    LinearPositions(const SkScalar* positions)

[mtklein, 2015/11/04 17:54:11]

It seems surprising we don't pass the shared y here and have NextPoint feed it
back.

[herb_g, 2015/11/04 21:58:42]

Right. This is a confusion with the existing code. In the existing code, the
offset (origin) is backed into the transformation, the general case the offset
is added to each point as it comes in (the more intuitive approach). I
maintained this behavior. This code can be simplified if one strategy or the
other is adopted. Next CL.

+        : fPositions(positions) { }
+
+    const SkPoint NextPoint() override {
+        SkScalar x = *fPositions++;
+        return {x, 0};
+    }
+private:
+    const SkScalar* fPositions;
+};
+
+class ArbitraryPositions final : public PositionReaderInterface {
+public:
+    ArbitraryPositions(const SkScalar* positions)
+        : fPositions(positions) { }
+        : fPositions(positions) { }

[mtklein, 2015/11/04 17:54:12]

Typo?

[herb_g, 2015/11/04 21:58:43]

Done.

+    const SkPoint NextPoint() override {
+        SkPoint to_return {fPositions[0], fPositions[1]};
+        fPositions += 2;
+        return to_return;
+    }
+
+private:
+    const SkScalar* fPositions;
+};
+
+typedef PolymorphicVariant<PositionReaderInterface, LinearPositions, ArbitraryPositions>
+    PositionReader;
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// MapperInterface given a point map it through the matrix. There are several shortcut variants.
+// * TranslationMapper - assumes a translation only matrix.
+// * XScaleMapper - assumes an X scaling and a translation.
+// * GeneralMapper - Does all other matricies.
+class MapperInterface : SkNoncopyable {
+public:
+    virtual ~MapperInterface() {}
+    virtual SkPoint Map(SkPoint position) const = 0;

[mtklein, 2015/11/04 17:54:11]

map

[herb_g, 2015/11/04 21:58:42]

Done.

+};
+

[mtklein, 2015/11/04 17:54:11]

Let's write down a formula for Map explicitly in terms of point, matrix, and
origin?  I'm finding myself confused about whether offsetting by origin happens
first, or second, or if it matters.

[herb_g, 2015/11/04 21:58:42]

This code faithfully reproduces the existing code. It handles the code in to
ways:
1) For the general case M(p+o).
2) For Xscale and Translate it does q = Mo then it calculates Mp+q for all
points.

I think this can be simplified to
(M * translate(o))p

But, I want to keep the code as is until it is used in a few places.

+class TranslationMapper final : public MapperInterface {
+public:
+    TranslationMapper(const SkMatrix& matrix, const SkPoint origin)
+        : fTranslate(matrix.mapXY(origin.fX, origin.fY)) { }
+    SkPoint Map(SkPoint position) const override {
+        return position + fTranslate;
+    }
+
+private:
+    const SkPoint fTranslate;
+};
+
+class XScaleMapper final : public MapperInterface {
+public:
+    XScaleMapper(const SkMatrix& matrix, const SkPoint origin)
+        : fTranslate(matrix.mapXY(origin.fX, origin.fY))
+        , fXScale(matrix.getScaleX()) { }
+    SkPoint Map(SkPoint position) const override {
+        return {SkScalarMul(fXScale, position.fX) + fTranslate.fX, fTranslate.fY};
+    }
+
+private:
+    const SkPoint fTranslate;
+    const SkScalar fXScale;
+};
+
+class GeneralMapper final : public MapperInterface {
+public:
+    GeneralMapper(const SkMatrix& matrix, const SkPoint origin)

[mtklein, 2015/11/04 17:54:11]

// Caller must keep matrix alive.

[herb_g, 2015/11/04 21:58:42]

Done.

+        : fOrigin(origin)
+        , fMatrix(matrix)
+        , fMapProc(matrix.getMapXYProc()) { }
+    SkPoint Map(SkPoint position) const override {
+        SkPoint result;
+        fMapProc(fMatrix, position.fX + fOrigin.fX, position.fY + fOrigin.fY, &result);
+        return result;
+    }
+
+private:
+    const SkPoint fOrigin;
+    const SkMatrix& fMatrix;
+    const SkMatrix::MapXYProc fMapProc;
+};
+
+typedef PolymorphicVariant<MapperInterface, TranslationMapper, XScaleMapper, GeneralMapper> Mapper;

[mtklein, 2015/11/04 17:54:12]

So, under this scheme, we're always going to occupy enough space for a matrix,
an origin point, and a cached MapXYProc.  That's enough to implement all three
of these strategies efficiently, simply by switching out different cached
MapXYProcs.  Seems like this doesn't need to have any inheritance at all.  E.g.

class Mapper {
  Mapper(const SkMatrix& m, SkPoint origin) : fMatrix(m), fOrigin(origin),
fMapProc(m.getMapXYProc()) {}

  void map(SkPoint p) const {
    SkPoint result;
    fMapProc(fMatrix, p.fX + fOrigin.fX, p.fY + fOrigin.fY, &result);
    return result;
  }
  ...
};

This would have a uniform single indirect function call, where the
implementations we've got here require 1 or 2.

[herb_g, 2015/11/04 21:58:42]

The maximum this stores is an SkPoint and two pointers. So, 24 bytes. An
SkMatrix is 40 bytes.

As I said above, this is a faithful rendering of the existing code. The existing
code does not call the matrix proc for the common cases. It is structured as a
switch statement to the common case or a case statement to a proc for the
general case. The existing code inlines the operation for the common cases, and
only does a single call for the general case.

+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// Text alignment handles shifting the glyph based on its width.
+template <SkPaint::Align textAlignment>

[mtklein, 2015/11/04 17:54:12]

Reading this file is a serious cognitive load.  Given the complexity of all this
code we're adding, let's see if there are places we can simplify.

I think this is a good place to start: I don't really think there's any benefit
to having this be a template method.  It's static and called with constants, so
I've got to figure that if we write this as a normal function of (glyph, Align)
-> SkPoint, good code will be generated.

[herb_g, 2015/11/04 21:58:41]

Done.

+static SkPoint text_alignment_adjustment(const SkGlyph& glyph) {
+    switch (textAlignment) {
+        case SkPaint::kLeft_Align:
+            return {0.0f, 0.0f};
+            break;
+        case SkPaint::kCenter_Align:
+            return {SkFixedToScalar(glyph.fAdvanceX >> 1),
+                    SkFixedToScalar(glyph.fAdvanceY >> 1)};
+            break;
+        case SkPaint::kRight_Align:
+            return {SkFixedToScalar(glyph.fAdvanceX),
+                    SkFixedToScalar(glyph.fAdvanceY)};
+            break;
+    }
+    // Even though the entire enum is covered above, MVSC doesn't think so. Make it happy.

[mtklein, 2015/11/04 17:54:12]

Might be nice to SkASSERT(false); too.

[herb_g, 2015/11/04 21:58:42]

Done.

+    return {0.0f, 0.0f};
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// Axis alignment are handled by NoAlignment, XAxisAlignment, and YAxisAlignment. They
+// handle aligning rounding to whole pixels of either the X axis or the Y axis. The XAlignment
+// and YAlignment functions produce a suitable position for the glyph cache to produce the
+// correct alignment. If a position is aligned with an axis a shortcut of 0 is used for the
+// sub-pixel position.
+
+static const SkScalar kSubpixelRounding = SkFixedToScalar(SkGlyph::kSubpixelRound);

[mtklein, 2015/11/04 17:54:12]

Doesn't this require a pre-main call?

[herb_g, 2015/11/04 21:58:42]

No. SkFixedToScalar is actually a macro. There are no function calls, it's
implemented using more macros.

+
+struct NoAlignment {
+    static SkPoint Rounding() { return {kSubpixelRounding, kSubpixelRounding}; }

[mtklein, 2015/11/04 17:54:12]

rounding, xAlignment, yAlignment

[herb_g, 2015/11/04 21:58:42]

Re-did all of this.

+    static SkFixed XAlignment(SkScalar x) { return SkScalarToFixed(x + kSubpixelRounding); }
+    static SkFixed YAlignment(SkScalar y) { return SkScalarToFixed(y + kSubpixelRounding); }
+};

[mtklein, 2015/11/04 17:54:11]

Again, simplifying, think there's any harm in expressing these all as a single
function of (enum, x,y) -> (fx, fy, rounding)?
The three methods all seem to be called right at the same time, and it'd be
static so it could be optimized the crap out of.
Given that these guys have no state, it'd be nice to just work with a plain old
function instead of structs, etc.

[herb_g, 2015/11/04 21:58:43]

Done.

+
+struct XAxisAlignment {
+    static SkPoint Rounding() { return {SkFixedToScalar(SkGlyph::kSubpixelRound), SK_ScalarHalf}; }
+    static SkFixed XAlignment(SkScalar x) { return SkScalarToFixed(x + kSubpixelRounding); }
+    static SkFixed YAlignment(SkScalar y) { return 0; }
+};
+
+struct YAxisAlignment {
+    static SkPoint Rounding() { return {SK_ScalarHalf, kSubpixelRounding}; }
+    static SkFixed XAlignment(SkScalar x) { return 0; }
+    static SkFixed YAlignment(SkScalar y) { return SkScalarToFixed(y + kSubpixelRounding); }
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// GlyphFindAndPlaceInterface given the text and position finds the correct glyph and does glyph
+// specific position adjustment. The FindAndPositionGlyph method takes text and position and calls
+// processOneGlyph with the correct glyph, final position and rounding terms. The final position
+// is not rounded yet and is the responsibility of processOneGlyph.
+template <typename GlyphProcessor>
+class GlyphFindAndPlaceInterface : SkNoncopyable {
+public:
+    struct Result {
+        const SkGlyph* fGlyph;
+        Sk48Dot16 fX;
+        Sk48Dot16 fY;
+    };
+    virtual ~GlyphFindAndPlaceInterface() { };

[mtklein, 2015/11/04 17:54:11]

Mixing templates and virtuals is getting me kind of seasick.

After reading this file for a while, I've realized I've got no idea what a
GlyphProcessor is.
I am worried that we're making up noun-entities and inheritance relationships so
that we can
use virtual dispatch to emulate what can be expressed as simple conditional
branches.

[herb_g, 2015/11/04 21:58:42]

Acknowledged.

+    // This should be a pure virtual, but older versions of clang have a bug that causes a
+    // compile error.

[mtklein, 2015/11/04 17:54:11]

Can you tack on Clang <= vX.Y so we know when to try removing this?

[herb_g, 2015/11/04 21:58:42]

Done.

+    virtual void FindAndPositionGlyph(const char **text, SkPoint position,

[mtklein, 2015/11/04 17:54:12]

findAndPositionGlyph

[herb_g, 2015/11/04 21:58:42]

Done.

+                                      GlyphProcessor&& processOneGlyph) {};
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// GlyphFindAndPlaceForSubpixel handles finding and placing glyphs when sub-pixel positioning is
+// requested. After it has found a placed the glyph it calls the templated function
+// GlyphProcessor in order to actually perform an action.
+template <typename GlyphProcessor, SkPaint::Align textAlignment, typename axisAlignment>
+class GlyphFindAndPlaceForSubpixel final : public GlyphFindAndPlaceInterface<GlyphProcessor> {
+public:
+    GlyphFindAndPlaceForSubpixel(SkGlyphCache* cache, SkDrawCacheProc glyphCacheProc)
+        : fCache(cache)
+        , fGlyphCacheProc(glyphCacheProc) {
+    }
+    void FindAndPositionGlyph(const char** text, SkPoint position,
+                              GlyphProcessor&& processOneGlyph) override {
+        SkPoint finalPosition = position;
+        if (textAlignment != SkPaint::kLeft_Align) {
+            // Get the width of an un-sub-pixel positioned glyph for calculating the alignment.
+            // This is not needed for kLeftAlign because its adjustment is always {0, 0}.
+            const char* tempText = *text;
+            const SkGlyph& metricGlyph = fGlyphCacheProc(fCache, &tempText, 0, 0);
+
+            if (metricGlyph.fWidth <= 0) {
+                return;
+            }
+
+            // Adjust the final position by the alignment adjustment.
+            finalPosition -= text_alignment_adjustment<textAlignment>(metricGlyph);
+        }
+
+        // Find the glyph.
+        const SkGlyph& renderGlyph = fGlyphCacheProc(
+            fCache, text,
+            axisAlignment::XAlignment(finalPosition.fX),
+            axisAlignment::YAlignment(finalPosition.fY));
+
+        // If the glyph has no width (no pixels) then don't bother processing it.
+        if (renderGlyph.fWidth > 0) {
+            processOneGlyph(renderGlyph, finalPosition, axisAlignment::Rounding());
+        }
+    }
+
+private:
+    SkGlyphCache* const fCache;
+    SkDrawCacheProc fGlyphCacheProc;
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// GlyphFindAndPlaceForFullPixel handles finding and placing glyphs when no sub-pixel positioning
+// is requested.
+template <typename GlyphProcessor, SkPaint::Align textAlignment>
+class GlyphFindAndPlaceForFullPixel final : public GlyphFindAndPlaceInterface<GlyphProcessor> {
+public:
+    GlyphFindAndPlaceForFullPixel(SkGlyphCache* cache, SkDrawCacheProc glyphCacheProc)
+        : fCache(cache)
+        , fGlyphCacheProc(glyphCacheProc) { }
+    void FindAndPositionGlyph(const char** text, SkPoint position,
+                              GlyphProcessor&& processOneGlyph) override {
+        SkPoint finalPosition = position;
+        const SkGlyph& glyph = fGlyphCacheProc(fCache, text, 0, 0);
+        if (glyph.fWidth <= 0) {
+            return;
+        }
+        finalPosition -= text_alignment_adjustment<textAlignment>(glyph);
+        processOneGlyph(glyph, finalPosition, {SK_ScalarHalf, SK_ScalarHalf});
+    }
+
+private:
+    SkGlyphCache* const fCache;
+    SkDrawCacheProc fGlyphCacheProc;
+};
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// GlyphFindAndPlace is a large variant that encapsulates the multiple types of finding and
+// placing a glyph. There are three factors that go into the different factors.
+// * Is sub-pixel positioned - a boolean that says whether to use sub-pixel positioning.
+// * Text alignment - indicates if the glyph should be placed to the right, centered or left of a
+//   given position.
+// * Axis alignment - indicates if the glyphs final sub-pixel position should be rounded to a
+//   whole pixel if the glyph is aligned with an axis. This is only used for sub-pixel positioning
+//   and allows the baseline to look crisp.

[mtklein, 2015/11/04 17:54:12]

What's the code-size impact of all these new classes?

[herb_g, 2015/11/04 21:58:42]

According too objdump

The sub-pixel variants take 1806 bytes.
The full pixel variants take 555 bytes. 

Total = 2361 bytes

+template <typename GlyphProcessor>
+using GlyphFindAndPlace =
+    PolymorphicVariant<
+        GlyphFindAndPlaceInterface<GlyphProcessor>,
+        // Subpixel
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kLeft_Align,   NoAlignment   >,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kLeft_Align,   XAxisAlignment>,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kLeft_Align,   YAxisAlignment>,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kCenter_Align, NoAlignment   >,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kCenter_Align, XAxisAlignment>,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kCenter_Align, YAxisAlignment>,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kRight_Align,  NoAlignment   >,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kRight_Align,  XAxisAlignment>,
+        GlyphFindAndPlaceForSubpixel< GlyphProcessor, SkPaint::kRight_Align,  YAxisAlignment>,
+        // Full pixel
+        GlyphFindAndPlaceForFullPixel<GlyphProcessor, SkPaint::kLeft_Align  >,
+        GlyphFindAndPlaceForFullPixel<GlyphProcessor, SkPaint::kCenter_Align>,
+        GlyphFindAndPlaceForFullPixel<GlyphProcessor, SkPaint::kRight_Align >
+    >;
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// init_subpixel is a helper function for initializing all the variants of
+// GlyphFindAndPlaceForSubpixel.
+template <typename GlyphProcessor, SkPaint::Align textAlignment>
+static void init_subpixel(
+    typename GlyphFindAndPlace<GlyphProcessor>::Variants* to_init,
+    SkAxisAlignment axisAlignment,
+    SkGlyphCache* cache,
+    SkDrawCacheProc glyphCacheProc) {
+    switch (axisAlignment) {
+        case kX_SkAxisAlignment:
+            to_init->template Init<GlyphFindAndPlaceForSubpixel<
+                GlyphProcessor, textAlignment, XAxisAlignment>>(
+                cache, glyphCacheProc);
+            break;
+        case kNone_SkAxisAlignment:
+            to_init->template Init<GlyphFindAndPlaceForSubpixel<
+                GlyphProcessor, textAlignment, NoAlignment>>(
+                cache, glyphCacheProc);
+            break;
+        case kY_SkAxisAlignment:
+            to_init->template Init<GlyphFindAndPlaceForSubpixel<
+                GlyphProcessor, textAlignment, YAxisAlignment>>(
+                cache, glyphCacheProc);
+            break;
+    }
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// SpecializedProcessPosText is a version of ProcessPosText that de-virtualizes the different
+// components used. This allows greater inlining freedom to the compiler. Currently, there is
+// only one variant that is used: sub-pixel position, left-aligned, x-axis-aligned, translation,
+// and one scalar per position entry.
+template <typename GlyphProcessor>
+static void SpecializedProcessPosText(const char *const text, size_t byteLength,

[mtklein, 2015/11/04 17:54:11]

specialized_process_pos_text

[herb_g, 2015/11/04 21:58:43]

Done.

+                                      const SkPoint &offset, const SkMatrix &matrix,
+                                      const SkScalar *const pos, SkDrawCacheProc &glyphCacheProc,
+                                      SkGlyphCache *cache, GlyphProcessor &&processOneGlyph) {
+    typedef GlyphFindAndPlaceForSubpixel<
+        GlyphProcessor, SkPaint::kLeft_Align, XAxisAlignment> Positioner;
+    LinearPositions positions{pos};
+    TranslationMapper mapper{matrix, offset};
+    Positioner positioner{cache, glyphCacheProc};
+    const char* cursor = text;
+    const char* stop = text + byteLength;
+    while (cursor < stop) {
+        SkPoint mappedPoint = mapper.TranslationMapper::Map(positions.LinearPositions::NextPoint());
+        positioner.Positioner::FindAndPositionGlyph(
+            &cursor, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph));
+    }
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// ProcessPosText handles all cases for finding and positioning glyphs. It has a very large
+// multiplicity.
+// * Sub-pixel positioning (2) - use sub-pixel positioning.
+// * Text alignment (3) - text alignment with respect to the glyph's width.
+// * Matrix type (3) - special cases for translation and X-coordinate scaling.
+// * Components per position (2) - the positions vector can have a common Y with different Xs, or
+//   XY-pairs.
+// * Axis Alignment (for sub-pixel positioning) (3) - when using sub-pixel positioning, round to
+// a whole coordinate instead of using sub-pixel positioning.
+// The number of variations is 108 for sub-pixel and 36 for full-pixel.
+// This routine handles all of them using inline polymorphic variable (no heap allocation).
+template <typename GlyphProcessor>
+static void ProcessPosText(const char text[], size_t byteLength,
+                            const SkPoint& offset, const SkMatrix& matrix,
+                            const SkScalar pos[], int scalarsPerPosition,
+                            SkPaint::Align textAlignment, SkDrawCacheProc& glyphCacheProc,
+                            SkGlyphCache* cache, GlyphProcessor&& processOneGlyph) {
+
+    PositionReader positionReader {

[mtklein, 2015/11/04 17:54:12]

I think I'd still prefer these to be written with () on the outside.
It just still reads to me like we're creating a struct with a single lambda
field, not calling a constructor.

I don't see anything wrong with using {} for struct-y things, like SkPoints.

[herb_g, 2015/11/04 21:58:42]

Done.

+        [&](PositionReader::Variants* to_init) {
+            if (2 == scalarsPerPosition) {
+                to_init->Init<ArbitraryPositions>(pos);
+            } else {
+                to_init->Init<LinearPositions>(pos);
+            }
+        }
+    };
+
+    Mapper mapper {
+        [&] (Mapper::Variants* to_init) {
+            uint32_t mtype = matrix.getType();
+            if (mtype & (SkMatrix::kAffine_Mask | SkMatrix::kPerspective_Mask)
+                || scalarsPerPosition == 2) {

[mtklein, 2015/11/04 17:54:12]

What's scalarsPerPosition have to do with what mapper we use?

[herb_g, 2015/11/04 21:58:43]

Unfortunately, the original code uses that to determine special had written
cases. I'm using the same cases for now.

+                to_init->Init<GeneralMapper>(matrix, offset);
+            } else if (mtype & SkMatrix::kScale_Mask) {

[mtklein, 2015/11/04 17:54:12]

Is y-scale impossible?

[herb_g, 2015/11/04 21:58:41]

The Skia API does not have a drawVPosText, so it is impossible. I was surprised
also.

+                to_init->Init<XScaleMapper>(matrix, offset);
+            } else {
+                to_init->Init<TranslationMapper>(matrix, offset);
+            }
+        }
+    };
+
+    GlyphFindAndPlace<GlyphProcessor> findAndPosition{
+        [&](typename GlyphFindAndPlace<GlyphProcessor>::Variants* to_init) {
+            if (cache->isSubpixel()) {
+                SkAxisAlignment axisAlignment = SkComputeAxisAlignmentForHText(matrix);
+                switch (textAlignment) {
+                    case SkPaint::kLeft_Align:
+                        init_subpixel<GlyphProcessor, SkPaint::kLeft_Align>(
+                            to_init, axisAlignment, cache, glyphCacheProc);
+                        break;
+                    case SkPaint::kCenter_Align:
+                        init_subpixel<GlyphProcessor, SkPaint::kCenter_Align>(
+                            to_init, axisAlignment, cache, glyphCacheProc);
+                        break;
+                    case SkPaint::kRight_Align:
+                        init_subpixel<GlyphProcessor, SkPaint::kRight_Align>(
+                            to_init, axisAlignment, cache, glyphCacheProc);
+                        break;
+                }
+            } else {
+                switch (textAlignment) {
+                    case SkPaint::kLeft_Align:
+                        to_init->template Init<GlyphFindAndPlaceForFullPixel<GlyphProcessor,
+                            SkPaint::kLeft_Align>>(
+                            cache, glyphCacheProc);
+                        break;
+                    case SkPaint::kCenter_Align:
+                        to_init->template Init<GlyphFindAndPlaceForFullPixel<GlyphProcessor,
+                                SkPaint::kCenter_Align>>(
+                                cache, glyphCacheProc);
+                        break;
+                    case SkPaint::kRight_Align:
+                        to_init->template Init<GlyphFindAndPlaceForFullPixel<GlyphProcessor,
+                            SkPaint::kRight_Align>>(
+                            cache, glyphCacheProc);
+                        break;
+                }
+            }
+        }
+    };
+
+    const char* stop = text + byteLength;
+    while (text < stop) {
+        SkPoint mappedPoint = mapper->Map(positionReader->NextPoint());
+        findAndPosition->FindAndPositionGlyph(
+            &text, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph));
+    }
+}
+
 void SkDraw::drawPosText(const char text[], size_t byteLength,
                          const SkScalar pos[], int scalarsPerPosition,
                          const SkPoint& offset, const SkPaint& paint) const {
@@ -1760,108 +2231,36 @@ void SkDraw::drawPosText(const char text[], size_t byteLength,
         }
     }
 
-    const char*        stop = text + byteLength;
-    SkTextAlignProc    alignProc(paint.getTextAlign());
     SkDraw1Glyph       d1g;
     SkDraw1Glyph::Proc proc = d1g.init(this, blitter, cache, paint);
-    SkTextMapStateProc tmsProc(*fMatrix, offset, scalarsPerPosition);
 

[mtklein, 2015/11/04 17:54:12]

There's something to be said for this existing code.  You can see the control
flow explicitly.

[herb_g, 2015/11/04 21:58:43]

Acknowledged.

-    if (cache->isSubpixel()) {
-        // maybe we should skip the rounding if linearText is set
-        SkAxisAlignment baseline = SkComputeAxisAlignmentForHText(*fMatrix);
-
-        SkFixed fxMask = ~0;
-        SkFixed fyMask = ~0;
-        if (kX_SkAxisAlignment == baseline) {
-            fyMask = 0;
-            d1g.fHalfSampleY = SK_ScalarHalf;
-        } else if (kY_SkAxisAlignment == baseline) {
-            fxMask = 0;
-            d1g.fHalfSampleX = SK_ScalarHalf;
-        }
-
-        if (SkPaint::kLeft_Align == paint.getTextAlign()) {
-            while (text < stop) {
-                SkPoint tmsLoc;
-                tmsProc(pos, &tmsLoc);
-
-                Sk48Dot16 fx = SkScalarTo48Dot16(tmsLoc.fX + d1g.fHalfSampleX);
-                Sk48Dot16 fy = SkScalarTo48Dot16(tmsLoc.fY + d1g.fHalfSampleY);
-
-                const SkGlyph& glyph = glyphCacheProc(cache, &text, fx & fxMask, fy & fyMask);
-
-                if (glyph.fWidth) {
-                    proc(d1g, fx, fy, glyph);
-                }
-                pos += scalarsPerPosition;
-            }
-        } else {
-            while (text < stop) {
-                const char* currentText = text;
-                const SkGlyph& metricGlyph = glyphCacheProc(cache, &text, 0, 0);
-
-                if (metricGlyph.fWidth) {
-                    SkDEBUGCODE(SkFixed prevAdvX = metricGlyph.fAdvanceX;)
-                    SkDEBUGCODE(SkFixed prevAdvY = metricGlyph.fAdvanceY;)
-                    SkPoint tmsLoc;
-                    tmsProc(pos, &tmsLoc);
-
-                    SkPoint alignLoc;
-                    alignProc(tmsLoc, metricGlyph, &alignLoc);
-
-                    Sk48Dot16 fx = SkScalarTo48Dot16(alignLoc.fX + d1g.fHalfSampleX);
-                    Sk48Dot16 fy = SkScalarTo48Dot16(alignLoc.fY + d1g.fHalfSampleY);
-
-                    // have to call again, now that we've been "aligned"
-                    const SkGlyph& glyph = glyphCacheProc(cache, &currentText,
-                                                          fx & fxMask, fy & fyMask);
-                    // the assumption is that the metrics haven't changed
-                    SkASSERT(prevAdvX == glyph.fAdvanceX);
-                    SkASSERT(prevAdvY == glyph.fAdvanceY);
-                    SkASSERT(glyph.fWidth);
-
-                    proc(d1g, fx, fy, glyph);
-                }
-                pos += scalarsPerPosition;
+    SkAxisAlignment axisAlignment = SkComputeAxisAlignmentForHText(*fMatrix);
+    uint32_t mtype = fMatrix->getType();
+    SkPaint::Align textAlignment = paint.getTextAlign();
+    if (scalarsPerPosition == 1
+        && textAlignment == SkPaint::kLeft_Align
+        && axisAlignment == kX_SkAxisAlignment
+        && cache->isSubpixel()
+        && (mtype
+           & (SkMatrix::kTranslate_Mask | SkMatrix::kScale_Mask
+              | SkMatrix::kAffine_Mask | SkMatrix::kPerspective_Mask)) <=
+                                                        SkMatrix::kTranslate_Mask) {
+        SpecializedProcessPosText(
+            text, byteLength, offset, *fMatrix, pos, glyphCacheProc, cache,
+            [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) {
+                position += rounding;
+                proc(d1g, SkScalarToFixed(position.fX), SkScalarToFixed(position.fY), glyph);
             }
-        }
-    } else {    // not subpixel
-        if (SkPaint::kLeft_Align == paint.getTextAlign()) {
-            while (text < stop) {
-                // the last 2 parameters are ignored
-                const SkGlyph& glyph = glyphCacheProc(cache, &text, 0, 0);
-
-                if (glyph.fWidth) {
-                    SkPoint tmsLoc;
-                    tmsProc(pos, &tmsLoc);
-
-                    proc(d1g,
-                         SkScalarTo48Dot16(tmsLoc.fX + SK_ScalarHalf), //d1g.fHalfSampleX,
-                         SkScalarTo48Dot16(tmsLoc.fY + SK_ScalarHalf), //d1g.fHalfSampleY,
-                         glyph);
-                }
-                pos += scalarsPerPosition;
-            }
-        } else {
-            while (text < stop) {
-                // the last 2 parameters are ignored
-                const SkGlyph& glyph = glyphCacheProc(cache, &text, 0, 0);
-
-                if (glyph.fWidth) {
-                    SkPoint tmsLoc;
-                    tmsProc(pos, &tmsLoc);
-
-                    SkPoint alignLoc;
-                    alignProc(tmsLoc, glyph, &alignLoc);
-
-                    proc(d1g,
-                         SkScalarTo48Dot16(alignLoc.fX + SK_ScalarHalf), //d1g.fHalfSampleX,
-                         SkScalarTo48Dot16(alignLoc.fY + SK_ScalarHalf), //d1g.fHalfSampleY,
-                         glyph);
-                }
-                pos += scalarsPerPosition;
+        );
+    } else {
+        ProcessPosText(
+            text, byteLength, offset, *fMatrix, pos, scalarsPerPosition,
+            textAlignment, glyphCacheProc, cache,
+            [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) {
+                position += rounding;
+                proc(d1g, SkScalarToFixed(position.fX), SkScalarToFixed(position.fY), glyph);
             }
-        }
+        );
     }
 }