From 3a9a7a310c5cff72bc1c2388a496af1b82326355 Mon Sep 17 00:00:00 2001 From: Florin Malita Date: Mon, 13 Mar 2017 09:03:24 -0400 Subject: [PATCH] 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 Reviewed-by: Mike Reed --- include/core/SkPicture.h | 3 +- include/core/SkTextBlob.h | 11 +-- src/core/SkReadBuffer.h | 1 + src/core/SkTextBlob.cpp | 125 ++++++++++++++++++------------- src/core/SkTextBlobRunIterator.h | 1 - tests/TextBlobTest.cpp | 9 ++- 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; 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 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(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(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(this->clusterBuffer() + fCount); + return isExtended() + ? reinterpret_cast(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( - reinterpret_cast(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( + reinterpret_cast(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::MakeFromBuffer(SkReadBuffer& reader) { - int runCount = reader.read32(); + const int runCount = reader.isVersionLT(SkReadBuffer::kTextBlobImplicitRunCount_Version) + ? reader.read32() : std::numeric_limits::max(); if (runCount < 0) { return nullptr; } @@ -348,6 +363,11 @@ sk_sp 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 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(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(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(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 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 blob(builder.make()); - REPORTER_ASSERT(reporter, blob->bounds().isEmpty()); + REPORTER_ASSERT(reporter, !blob); } // Implicit bounds @@ -273,6 +272,10 @@ private: } sk_sp blob(builder.make()); + REPORTER_ASSERT(reporter, (inCount > 0) == SkToBool(blob)); + if (!blob) { + return; + } SkTextBlobRunIterator it(blob.get()); for (unsigned i = 0; i < outCount; ++i) {