Remove run count field from SkTextBlob.

We can flag the last run record instead.  Run iteration is always
sequential, so no penalty.

As a side effect, we can no longer allow instantiation of zero-run text
blobs - but that seems like a good idea anyway.

Change-Id: I7ca80c4780623d5a188f92dfe6d6fe152f20f666
Reviewed-on: https://skia-review.googlesource.com/9149
Commit-Queue: Florin Malita <fmalita@chromium.org>
Reviewed-by: Mike Reed <reed@google.com>

6 files changed, 90 insertions, 60 deletions

diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index 8047858b16..133d3c92c0 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -199,10 +199,11 @@ private:
     // V49: Gradients serialized as SkColor4f + SkColorSpace
     // V50: SkXfermode -> SkBlendMode
     // V51: more SkXfermode -> SkBlendMode
+    // V52: Remove SkTextBlob::fRunCount
 
     // Only SKPs within the min/current picture version range (inclusive) can be read.
     static const uint32_t     MIN_PICTURE_VERSION = 35;     // Produced by Chrome M39.
-    static const uint32_t CURRENT_PICTURE_VERSION = 51;
+    static const uint32_t CURRENT_PICTURE_VERSION = 52;
 
     static_assert(MIN_PICTURE_VERSION <= 41,
                   "Remove kFontFileName and related code from SkFontDescriptor.cpp.");
diff --git a/include/core/SkTextBlob.h b/include/core/SkTextBlob.h
index 8198f04a80..1d17f4dafe 100644
--- a/include/core/SkTextBlob.h
+++ b/include/core/SkTextBlob.h
@@ -60,7 +60,7 @@ private:
     friend class SkNVRefCnt<SkTextBlob>;
     class RunRecord;
 
-    SkTextBlob(int runCount, const SkRect& bounds);
+    explicit SkTextBlob(const SkRect& bounds);
 
     ~SkTextBlob();
 
@@ -78,8 +78,7 @@ private:
     friend class SkTextBlobBuilder;
     friend class SkTextBlobRunIterator;
 
-    const int        fRunCount;
-    const SkRect     fBounds;
+    const SkRect   fBounds;
     const uint32_t fUniqueID;
 
     SkDEBUGCODE(size_t fStorageSize;)
@@ -101,8 +100,10 @@ public:
     ~SkTextBlobBuilder();
 
     /**
-     *  Returns an immutable SkTextBlob for the current runs/glyphs. The builder is reset and
-     *  can be reused.
+     *  Returns an immutable SkTextBlob for the current runs/glyphs,
+     *  or nullptr if no runs were allocated.
+     *
+     *  The builder is reset and can be reused.
      */
     sk_sp<SkTextBlob> make();
 
diff --git a/src/core/SkReadBuffer.h b/src/core/SkReadBuffer.h
index 12fc5ca88e..014e034020 100644
--- a/src/core/SkReadBuffer.h
+++ b/src/core/SkReadBuffer.h
@@ -71,6 +71,7 @@ public:
         kGradientShaderFloatColor_Version  = 49,
         kXfermodeToBlendMode_Version       = 50,
         kXfermodeToBlendMode2_Version      = 51,
+        kTextBlobImplicitRunCount_Version  = 52,
     };
 
     /**
diff --git a/src/core/SkTextBlob.cpp b/src/core/SkTextBlob.cpp
index 817fc62a4a..b1f151f1df 100644
--- a/src/core/SkTextBlob.cpp
+++ b/src/core/SkTextBlob.cpp
@@ -132,10 +132,12 @@ public:
         : fFont(font)
         , fCount(count)
         , fOffset(offset)
-        , fPositioning(pos)
-        , fExtended(textSize > 0) {
+        , fFlags(pos) {
+        SkASSERT(static_cast<unsigned>(pos) <= Flags::kPositioning_Mask);
+
         SkDEBUGCODE(fMagic = kRunRecordMagic);
         if (textSize > 0) {
+            fFlags |= kExtended_Flag;
             *this->textSizePtr() = textSize;
         }
     }
@@ -153,7 +155,7 @@ public:
     }
 
     GlyphPositioning positioning() const {
-        return fPositioning;
+        return static_cast<GlyphPositioning>(fFlags & kPositioning_Mask);
     }
 
     uint16_t* glyphBuffer() const {
@@ -168,16 +170,17 @@ public:
                                            SkAlign4(fCount * sizeof(uint16_t)));
     }
 
-    uint32_t textSize() const { return fExtended ? *this->textSizePtr() : 0; }
+    uint32_t textSize() const { return isExtended() ? *this->textSizePtr() : 0; }
 
     uint32_t* clusterBuffer() const {
         // clusters follow the textSize.
-        return fExtended ? 1 + this->textSizePtr() : nullptr;
+        return isExtended() ? 1 + this->textSizePtr() : nullptr;
     }
 
     char* textBuffer() const {
-        if (!fExtended) { return nullptr; }
-        return reinterpret_cast<char*>(this->clusterBuffer() + fCount);
+        return isExtended()
+            ? reinterpret_cast<char*>(this->clusterBuffer() + fCount)
+            : nullptr;
     }
 
     static size_t StorageSize(int glyphCount, int textSize,
@@ -201,22 +204,21 @@ public:
     }
 
     static const RunRecord* Next(const RunRecord* run) {
-        return reinterpret_cast<const RunRecord*>(
-                reinterpret_cast<const uint8_t*>(run)
-                + StorageSize(run->glyphCount(), run->textSize(), run->positioning()));
+        return SkToBool(run->fFlags & kLast_Flag) ? nullptr : NextUnchecked(run);
     }
 
     void validate(const uint8_t* storageTop) const {
         SkASSERT(kRunRecordMagic == fMagic);
-        SkASSERT((uint8_t*)Next(this) <= storageTop);
+        SkASSERT((uint8_t*)NextUnchecked(this) <= storageTop);
 
         SkASSERT(glyphBuffer() + fCount <= (uint16_t*)posBuffer());
-        SkASSERT(posBuffer() + fCount * ScalarsPerGlyph(fPositioning) <= (SkScalar*)Next(this));
-        if (fExtended) {
+        SkASSERT(posBuffer() + fCount * ScalarsPerGlyph(positioning())
+                 <= (SkScalar*)NextUnchecked(this));
+        if (isExtended()) {
             SkASSERT(textSize() > 0);
-            SkASSERT(textSizePtr() < (uint32_t*)Next(this));
-            SkASSERT(clusterBuffer() < (uint32_t*)Next(this));
-            SkASSERT(textBuffer() + textSize() <= (char*)Next(this));
+            SkASSERT(textSizePtr() < (uint32_t*)NextUnchecked(this));
+            SkASSERT(clusterBuffer() < (uint32_t*)NextUnchecked(this));
+            SkASSERT(textBuffer() + textSize() <= (char*)NextUnchecked(this));
         }
         static_assert(sizeof(SkTextBlob::RunRecord) == sizeof(RunRecordStorageEquivalent),
                       "runrecord_should_stay_packed");
@@ -225,6 +227,18 @@ public:
 private:
     friend class SkTextBlobBuilder;
 
+    enum Flags {
+        kPositioning_Mask = 0x03, // bits 0-1 reserved for positioning
+        kLast_Flag        = 0x04, // set for the last blob run
+        kExtended_Flag    = 0x08, // set for runs with text/cluster info
+    };
+
+    static const RunRecord* NextUnchecked(const RunRecord* run) {
+        return reinterpret_cast<const RunRecord*>(
+                reinterpret_cast<const uint8_t*>(run)
+                + StorageSize(run->glyphCount(), run->textSize(), run->positioning()));
+    }
+
     static size_t PosCount(int glyphCount,
                            SkTextBlob::GlyphPositioning positioning) {
         return glyphCount * ScalarsPerGlyph(positioning);
@@ -232,8 +246,8 @@ private:
     
     uint32_t* textSizePtr() const {
         // textSize follows the position buffer.
-        SkASSERT(fExtended);
-        return (uint32_t*)(&this->posBuffer()[PosCount(fCount, fPositioning)]);
+        SkASSERT(isExtended());
+        return (uint32_t*)(&this->posBuffer()[PosCount(fCount, positioning())]);
     }
 
     void grow(uint32_t count) {
@@ -242,18 +256,21 @@ private:
         fCount += count;
 
         // Move the initial pos scalars to their new location.
-        size_t copySize = initialCount * sizeof(SkScalar) * ScalarsPerGlyph(fPositioning);
-        SkASSERT((uint8_t*)posBuffer() + copySize <= (uint8_t*)Next(this));
+        size_t copySize = initialCount * sizeof(SkScalar) * ScalarsPerGlyph(positioning());
+        SkASSERT((uint8_t*)posBuffer() + copySize <= (uint8_t*)NextUnchecked(this));
 
         // memmove, as the buffers may overlap
         memmove(posBuffer(), initialPosBuffer, copySize);
     }
 
+    bool isExtended() const {
+        return fFlags & kExtended_Flag;
+    }
+
     RunFont          fFont;
     uint32_t         fCount;
     SkPoint          fOffset;
-    GlyphPositioning fPositioning;
-    bool             fExtended;
+    uint32_t         fFlags;
 
     SkDEBUGCODE(unsigned fMagic;)
 };
@@ -267,20 +284,19 @@ static int32_t next_id() {
     return id;
 }
 
-SkTextBlob::SkTextBlob(int runCount, const SkRect& bounds)
-    : fRunCount(runCount)
-    , fBounds(bounds)
+SkTextBlob::SkTextBlob(const SkRect& bounds)
+    : fBounds(bounds)
     , fUniqueID(next_id()) {
 }
 
 SkTextBlob::~SkTextBlob() {
-    const RunRecord* run = RunRecord::First(this);
-    for (int i = 0; i < fRunCount; ++i) {
-        const RunRecord* nextRun = RunRecord::Next(run);
+    const auto* run = RunRecord::First(this);
+    do {
+        const auto* nextRun = RunRecord::Next(run);
         SkDEBUGCODE(run->validate((uint8_t*)this + fStorageSize);)
         run->~RunRecord();
         run = nextRun;
-    }
+    } while (run);
 }
 
 namespace {
@@ -295,9 +311,6 @@ union PositioningAndExtended {
 } // namespace
 
 void SkTextBlob::flatten(SkWriteBuffer& buffer) const {
-    int runCount = fRunCount;
-
-    buffer.write32(runCount);
     buffer.writeRect(fBounds);
 
     SkPaint runPaint;
@@ -331,13 +344,15 @@ void SkTextBlob::flatten(SkWriteBuffer& buffer) const {
         }
 
         it.next();
-        SkDEBUGCODE(runCount--);
     }
-    SkASSERT(0 == runCount);
+
+    // Marker for the last run (0 is not a valid glyph count).
+    buffer.write32(0);
 }
 
 sk_sp<SkTextBlob> SkTextBlob::MakeFromBuffer(SkReadBuffer& reader) {
-    int runCount = reader.read32();
+    const int runCount = reader.isVersionLT(SkReadBuffer::kTextBlobImplicitRunCount_Version)
+        ? reader.read32() : std::numeric_limits<int>::max();
     if (runCount < 0) {
         return nullptr;
     }
@@ -348,6 +363,11 @@ sk_sp<SkTextBlob> SkTextBlob::MakeFromBuffer(SkReadBuffer& reader) {
     SkTextBlobBuilder blobBuilder;
     for (int i = 0; i < runCount; ++i) {
         int glyphCount = reader.read32();
+        if (glyphCount == 0 &&
+            !reader.isVersionLT(SkReadBuffer::kTextBlobImplicitRunCount_Version)) {
+            // End-of-runs marker.
+            break;
+        }
 
         PositioningAndExtended pe;
         pe.intValue = reader.read32();
@@ -403,13 +423,12 @@ unsigned SkTextBlob::ScalarsPerGlyph(GlyphPositioning pos) {
 }
 
 SkTextBlobRunIterator::SkTextBlobRunIterator(const SkTextBlob* blob)
-    : fCurrentRun(SkTextBlob::RunRecord::First(blob))
-    , fRemainingRuns(blob->fRunCount) {
+    : fCurrentRun(SkTextBlob::RunRecord::First(blob)) {
     SkDEBUGCODE(fStorageTop = (uint8_t*)blob + blob->fStorageSize;)
 }
 
 bool SkTextBlobRunIterator::done() const {
-    return fRemainingRuns <= 0;
+    return !fCurrentRun;
 }
 
 void SkTextBlobRunIterator::next() {
@@ -418,7 +437,6 @@ void SkTextBlobRunIterator::next() {
     if (!this->done()) {
         SkDEBUGCODE(fCurrentRun->validate(fStorageTop);)
         fCurrentRun = SkTextBlob::RunRecord::Next(fCurrentRun);
-        fRemainingRuns--;
     }
 }
 
@@ -742,29 +760,36 @@ const SkTextBlobBuilder::RunBuffer& SkTextBlobBuilder::allocRunTextPos(const SkP
 }
 
 sk_sp<SkTextBlob> SkTextBlobBuilder::make() {
-    SkASSERT((fRunCount > 0) == (nullptr != fStorage.get()));
+    if (!fRunCount) {
+        // We don't instantiate empty blobs.
+        SkASSERT(!fStorage.get());
+        SkASSERT(fStorageUsed == 0);
+        SkASSERT(fStorageSize == 0);
+        SkASSERT(fLastRun == 0);
+        SkASSERT(fBounds.isEmpty());
+        return nullptr;
+    }
 
     this->updateDeferredBounds();
 
-    if (0 == fRunCount) {
-        SkASSERT(nullptr == fStorage.get());
-        fStorageUsed = sizeof(SkTextBlob);
-        fStorage.realloc(fStorageUsed);
-    }
+    // Tag the last run as such.
+    auto* lastRun = reinterpret_cast<SkTextBlob::RunRecord*>(fStorage.get() + fLastRun);
+    lastRun->fFlags |= SkTextBlob::RunRecord::kLast_Flag;
 
-    SkTextBlob* blob = new (fStorage.release()) SkTextBlob(fRunCount, fBounds);
+    SkTextBlob* blob = new (fStorage.release()) SkTextBlob(fBounds);
     SkDEBUGCODE(const_cast<SkTextBlob*>(blob)->fStorageSize = fStorageSize;)
 
     SkDEBUGCODE(
         size_t validateSize = sizeof(SkTextBlob);
-        const SkTextBlob::RunRecord* run = SkTextBlob::RunRecord::First(blob);
-        for (int i = 0; i < fRunCount; ++i) {
+        for (const auto* run = SkTextBlob::RunRecord::First(blob); run;
+             run = SkTextBlob::RunRecord::Next(run)) {
             validateSize += SkTextBlob::RunRecord::StorageSize(
-                    run->fCount, run->textSize(), run->fPositioning);
+                    run->fCount, run->textSize(), run->positioning());
             run->validate(reinterpret_cast<const uint8_t*>(blob) + fStorageUsed);
-            run = SkTextBlob::RunRecord::Next(run);
+            fRunCount--;
         }
         SkASSERT(validateSize == fStorageUsed);
+        SkASSERT(fRunCount == 0);
     )
 
     fStorageUsed = 0;
diff --git a/src/core/SkTextBlobRunIterator.h b/src/core/SkTextBlobRunIterator.h
index 2f1477bf06..18f41d7dcb 100644
--- a/src/core/SkTextBlobRunIterator.h
+++ b/src/core/SkTextBlobRunIterator.h
@@ -36,7 +36,6 @@ public:
 
 private:
     const SkTextBlob::RunRecord* fCurrentRun;
-    int fRemainingRuns;
 
     SkDEBUGCODE(uint8_t* fStorageTop;)
 };
diff --git a/tests/TextBlobTest.cpp b/tests/TextBlobTest.cpp
index 09389a4b52..38341163ac 100644
--- a/tests/TextBlobTest.cpp
+++ b/tests/TextBlobTest.cpp
@@ -105,7 +105,7 @@ public:
         // Explicit bounds.
         {
             sk_sp<SkTextBlob> blob(builder.make());
-            REPORTER_ASSERT(reporter, blob->bounds().isEmpty());
+            REPORTER_ASSERT(reporter, !blob);
         }
 
         {
@@ -143,9 +143,8 @@ public:
         }
 
         {
-            // Verify empty blob bounds after building some non-empty blobs.
             sk_sp<SkTextBlob> blob(builder.make());
-            REPORTER_ASSERT(reporter, blob->bounds().isEmpty());
+            REPORTER_ASSERT(reporter, !blob);
         }
 
         // Implicit bounds
@@ -273,6 +272,10 @@ private:
         }
 
         sk_sp<SkTextBlob> blob(builder.make());
+        REPORTER_ASSERT(reporter, (inCount > 0) == SkToBool(blob));
+        if (!blob) {
+            return;
+        }
 
         SkTextBlobRunIterator it(blob.get());
         for (unsigned i = 0; i < outCount; ++i) {