From: bsalomon Date: Tue, 3 Feb 2015 05:06:23 +0000 (-0800) Subject: Revert of Make the glyph array entries inline. (patchset #10 id:170001 of https:... X-Git-Tag: accepted/tizen/5.0/unified/20181102.025319~3761 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=9bf4e5bbf6ca96042c0e0f5dca9a52a943f25716;p=platform%2Fupstream%2FlibSkiaSharp.git Revert of Make the glyph array entries inline. (patchset #10 id:170001 of https://codereview.chromium.org/885903002/) Reason for revert: I suspect this is causing the off-by-one character issues that show up in gold.skia.org as of now. All the errors I've seen are on a Win7 bot. Example: good: https://gold.skia.org/img/images/35396bb0d299b81c0031dc0632a019d4.png bad: https://gold.skia.org/img/images/484e511f9e696d95031cd25aeae59da0.png Original issue's description: > Make the glyph array entries inline. > BUG=skia: > > Committed: https://skia.googlesource.com/skia/+/4c08f16b252a55e438a61f26e5581394ed177da1 TBR=mtklein@google.com,reed@google.com,herb@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia: Review URL: https://codereview.chromium.org/897463004 --- diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h index 4f9c5bf..9abefa8 100644 --- a/src/core/SkGlyph.h +++ b/src/core/SkGlyph.h @@ -104,10 +104,8 @@ struct SkGlyph { kSubShiftY = 0 }; - // The code is increased by one in MakeID. Adjust back. This allows the zero - // id to be the invalid id. static unsigned ID2Code(uint32_t id) { - return (id & kCodeMask) - 1; + return id & kCodeMask; } static unsigned ID2SubX(uint32_t id) { @@ -127,21 +125,17 @@ struct SkGlyph { return sub << (16 - kSubBits); } - // This and the MakeID below must not return an id of zero. Zero is used as - // the invalid id. static uint32_t MakeID(unsigned code) { - SkASSERT(code + 1 <= kCodeMask); - return code + 1; + return code; } - // See comment for MakeID above. static uint32_t MakeID(unsigned code, SkFixed x, SkFixed y) { - SkASSERT(code + 1 <= kCodeMask); + SkASSERT(code <= kCodeMask); x = FixedToSub(x); y = FixedToSub(y); return (x << (kSubShift + kSubShiftX)) | (y << (kSubShift + kSubShiftY)) | - (code + 1); + code; } void toMask(SkMask* mask) const; diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp index 4a82836..57c54bf 100755 --- a/src/core/SkGlyphCache.cpp +++ b/src/core/SkGlyphCache.cpp @@ -66,14 +66,10 @@ SkGlyphCache::SkGlyphCache(SkTypeface* typeface, const SkDescriptor* desc, SkSca fDesc = desc->copy(); fScalerContext->getFontMetrics(&fFontMetrics); - - // Create the sentinel SkGlyph. - SkGlyph* sentinel = fGlyphArray.insert(kSentinelGlyphID); - sentinel->init(0); - - // Initialize all index to zero which points to the sentinel SkGlyph. - memset(fGlyphHash, 0x00, sizeof(fGlyphHash)); + // init to 0 so that all of the pointers will be null + memset(fGlyphHash, 0, sizeof(fGlyphHash)); + fMemoryUsed = sizeof(*this); fGlyphArray.setReserve(kMinGlyphCount); @@ -110,10 +106,10 @@ SkGlyphCache::~SkGlyphCache() { } #endif - SkGlyph* gptr = fGlyphArray.begin(); - SkGlyph* stop = fGlyphArray.end(); + SkGlyph** gptr = fGlyphArray.begin(); + SkGlyph** stop = fGlyphArray.end(); while (gptr < stop) { - SkPath* path = gptr->fPath; + SkPath* path = (*gptr)->fPath; if (path) { SkDELETE(path); } @@ -126,31 +122,15 @@ SkGlyphCache::~SkGlyphCache() { SkGlyphCache::CharGlyphRec* SkGlyphCache::getCharGlyphRec(uint32_t id) { if (NULL == fCharToGlyphHash.get()) { - // Allocate the array. fCharToGlyphHash.reset(kHashCount); - // Initialize entries of fCharToGlyphHash to index the sentinel glyph. - memset(fCharToGlyphHash.get(), 0x00, + // init with 0xFF so that the charCode field will be -1, which is invalid + memset(fCharToGlyphHash.get(), 0xFF, sizeof(CharGlyphRec) * kHashCount); } return &fCharToGlyphHash[ID2HashIndex(id)]; } -void SkGlyphCache::adjustCaches(int insertion_index) { - for (int i = 0; i < kHashCount; ++i) { - if (fGlyphHash[i] >= SkToU16(insertion_index)) { - fGlyphHash[i] += 1; - } - } - if (fCharToGlyphHash.get() != NULL) { - for (int i = 0; i < kHashCount; ++i) { - if (fCharToGlyphHash[i].fGlyphIndex >= SkToU16(insertion_index)) { - fCharToGlyphHash[i].fGlyphIndex += 1; - } - } - } -} - /////////////////////////////////////////////////////////////////////////////// #ifdef SK_DEBUG @@ -165,7 +145,7 @@ uint16_t SkGlyphCache::unicharToGlyph(SkUnichar charCode) { const CharGlyphRec& rec = *this->getCharGlyphRec(id); if (rec.fID == id) { - return fGlyphArray[rec.fGlyphIndex].getGlyphID(); + return rec.fGlyph->getGlyphID(); } else { return fScalerContext->charToGlyphID(charCode); } @@ -183,118 +163,159 @@ unsigned SkGlyphCache::getGlyphCount() { const SkGlyph& SkGlyphCache::getUnicharAdvance(SkUnichar charCode) { VALIDATE(); - return *this->lookupByChar(charCode, kJustAdvance_MetricsType); + uint32_t id = SkGlyph::MakeID(charCode); + CharGlyphRec* rec = this->getCharGlyphRec(id); + + if (rec->fID != id) { + // this ID is based on the UniChar + rec->fID = id; + // this ID is based on the glyph index + id = SkGlyph::MakeID(fScalerContext->charToGlyphID(charCode)); + rec->fGlyph = this->lookupMetrics(id, kJustAdvance_MetricsType); + } + return *rec->fGlyph; } const SkGlyph& SkGlyphCache::getGlyphIDAdvance(uint16_t glyphID) { VALIDATE(); uint32_t id = SkGlyph::MakeID(glyphID); - return *this->lookupByChar(id, kJustAdvance_MetricsType); + unsigned index = ID2HashIndex(id); + SkGlyph* glyph = fGlyphHash[index]; + if (NULL == glyph || glyph->fID != id) { + glyph = this->lookupMetrics(glyphID, kJustAdvance_MetricsType); + fGlyphHash[index] = glyph; + } + return *glyph; } /////////////////////////////////////////////////////////////////////////////// const SkGlyph& SkGlyphCache::getUnicharMetrics(SkUnichar charCode) { VALIDATE(); - return *this->lookupByChar(charCode, kFull_MetricsType); + uint32_t id = SkGlyph::MakeID(charCode); + CharGlyphRec* rec = this->getCharGlyphRec(id); + + if (rec->fID != id) { + RecordHashCollisionIf(rec->fGlyph != NULL); + // this ID is based on the UniChar + rec->fID = id; + // this ID is based on the glyph index + id = SkGlyph::MakeID(fScalerContext->charToGlyphID(charCode)); + rec->fGlyph = this->lookupMetrics(id, kFull_MetricsType); + } else { + RecordHashSuccess(); + if (rec->fGlyph->isJustAdvance()) { + fScalerContext->getMetrics(rec->fGlyph); + } + } + SkASSERT(rec->fGlyph->isFullMetrics()); + return *rec->fGlyph; } const SkGlyph& SkGlyphCache::getUnicharMetrics(SkUnichar charCode, SkFixed x, SkFixed y) { VALIDATE(); - return *this->lookupByChar(charCode, kFull_MetricsType, x, y); -} - -const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID) { - VALIDATE(); - uint32_t id = SkGlyph::MakeID(glyphID); - return *this->lookupByCombinedID(id, kFull_MetricsType); -} - -const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID, SkFixed x, SkFixed y) { - VALIDATE(); - uint32_t id = SkGlyph::MakeID(glyphID, x, y); - return *this->lookupByCombinedID(id, kFull_MetricsType); -} - -SkGlyph* SkGlyphCache::lookupByChar(SkUnichar charCode, MetricsType type, SkFixed x, SkFixed y) { uint32_t id = SkGlyph::MakeID(charCode, x, y); CharGlyphRec* rec = this->getCharGlyphRec(id); - SkGlyph* glyph; + if (rec->fID != id) { - RecordHashCollisionIf(glyph_index != kSentinelGlyphIndex); + RecordHashCollisionIf(rec->fGlyph != NULL); // this ID is based on the UniChar rec->fID = id; // this ID is based on the glyph index id = SkGlyph::MakeID(fScalerContext->charToGlyphID(charCode), x, y); - rec->fGlyphIndex = this->lookupMetrics(id, type); - glyph = &fGlyphArray[rec->fGlyphIndex]; + rec->fGlyph = this->lookupMetrics(id, kFull_MetricsType); } else { RecordHashSuccess(); - glyph = &fGlyphArray[rec->fGlyphIndex]; - if (type == kFull_MetricsType && glyph->isJustAdvance()) { - fScalerContext->getMetrics(glyph); + if (rec->fGlyph->isJustAdvance()) { + fScalerContext->getMetrics(rec->fGlyph); } } - return glyph; + SkASSERT(rec->fGlyph->isFullMetrics()); + return *rec->fGlyph; } -SkGlyph* SkGlyphCache::lookupByCombinedID(uint32_t id, MetricsType type) { - uint32_t hash_index = ID2HashIndex(id); - uint16_t glyph_index = fGlyphHash[hash_index]; - SkGlyph* glyph = &fGlyphArray[glyph_index]; - - if (glyph->fID != id) { - RecordHashCollisionIf(glyph_index != kSentinelGlyphIndex); - glyph_index = this->lookupMetrics(id, type); - fGlyphHash[hash_index] = glyph_index; - glyph = &fGlyphArray[glyph_index]; +const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID) { + VALIDATE(); + uint32_t id = SkGlyph::MakeID(glyphID); + unsigned index = ID2HashIndex(id); + SkGlyph* glyph = fGlyphHash[index]; + + if (NULL == glyph || glyph->fID != id) { + RecordHashCollisionIf(glyph != NULL); + glyph = this->lookupMetrics(glyphID, kFull_MetricsType); + fGlyphHash[index] = glyph; } else { RecordHashSuccess(); - if (type == kFull_MetricsType && glyph->isJustAdvance()) { - fScalerContext->getMetrics(glyph); + if (glyph->isJustAdvance()) { + fScalerContext->getMetrics(glyph); } } - return glyph; + SkASSERT(glyph->isFullMetrics()); + return *glyph; } -uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { - // Count is always greater than 0 because of the sentinel. - SkGlyph* gptr = fGlyphArray.begin(); - int lo = 0; - int hi = fGlyphArray.count() - 1; - while (lo < hi) { - int mid = (hi + lo) >> 1; - if (gptr[mid].fID < id) { - lo = mid + 1; - } else { - hi = mid; - } - } +const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID, SkFixed x, SkFixed y) { + VALIDATE(); + uint32_t id = SkGlyph::MakeID(glyphID, x, y); + unsigned index = ID2HashIndex(id); + SkGlyph* glyph = fGlyphHash[index]; - uint16_t glyph_index = hi; - SkGlyph* glyph = &gptr[glyph_index]; - if (glyph->fID == id) { - if (kFull_MetricsType == mtype && glyph->isJustAdvance()) { + if (NULL == glyph || glyph->fID != id) { + RecordHashCollisionIf(glyph != NULL); + glyph = this->lookupMetrics(id, kFull_MetricsType); + fGlyphHash[index] = glyph; + } else { + RecordHashSuccess(); + if (glyph->isJustAdvance()) { fScalerContext->getMetrics(glyph); } - SkASSERT(glyph_index != kSentinelGlyphIndex); - return glyph_index; } + SkASSERT(glyph->isFullMetrics()); + return *glyph; +} + +SkGlyph* SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { + SkGlyph* glyph; + + int hi = 0; + int count = fGlyphArray.count(); - // check if we need to bump hi before falling though to the allocator - if (glyph->fID < id) { - glyph_index += 1; + if (count) { + SkGlyph** gptr = fGlyphArray.begin(); + int lo = 0; + + hi = count - 1; + while (lo < hi) { + int mid = (hi + lo) >> 1; + if (gptr[mid]->fID < id) { + lo = mid + 1; + } else { + hi = mid; + } + } + glyph = gptr[hi]; + if (glyph->fID == id) { + if (kFull_MetricsType == mtype && glyph->isJustAdvance()) { + fScalerContext->getMetrics(glyph); + } + return glyph; + } + + // check if we need to bump hi before falling though to the allocator + if (glyph->fID < id) { + hi += 1; + } } - + // not found, but hi tells us where to inser the new glyph fMemoryUsed += sizeof(SkGlyph); - - this->adjustCaches(glyph_index); - glyph = fGlyphArray.insert(glyph_index); + glyph = (SkGlyph*)fGlyphAlloc.alloc(sizeof(SkGlyph), + SkChunkAlloc::kThrow_AllocFailType); glyph->init(id); + *fGlyphArray.insert(hi) = glyph; if (kJustAdvance_MetricsType == mtype) { fScalerContext->getAdvance(glyph); @@ -303,8 +324,7 @@ uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { fScalerContext->getMetrics(glyph); } - SkASSERT(glyph_index != kSentinelGlyphIndex); - return glyph_index; + return glyph; } const void* SkGlyphCache::findImage(const SkGlyph& glyph) { diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h index 8805085..b335d29 100644 --- a/src/core/SkGlyphCache.h +++ b/src/core/SkGlyphCache.h @@ -187,48 +187,32 @@ private: kFull_MetricsType }; - // Return the SkGlyph* associated with MakeID. The id parameter is the combined glyph/x/y - // id generated by MakeID. If it is just a glyph id then x and y are assuemd to be zero. - SkGlyph* lookupByCombinedID(uint32_t id, MetricsType type); - - // Return a SkGlyph* associated with unicode id and position x and y. - SkGlyph* lookupByChar(SkUnichar id, MetricsType type, SkFixed x = 0, SkFixed y = 0); - - // Return the index of id in the fGlyphArray. If it does - // not exist, create a new one using MaetricsType. - uint16_t lookupMetrics(uint32_t id, MetricsType type); + SkGlyph* lookupMetrics(uint32_t id, MetricsType); static bool DetachProc(const SkGlyphCache*, void*) { return true; } - SkGlyphCache* fNext, *fPrev; - SkDescriptor* fDesc; - SkScalerContext* fScalerContext; + SkGlyphCache* fNext, *fPrev; + SkDescriptor* fDesc; + SkScalerContext* fScalerContext; SkPaint::FontMetrics fFontMetrics; enum { - kHashBits = 8, - kHashCount = 1 << kHashBits, - kHashMask = kHashCount - 1, - kSentinelGlyphIndex = 0, - kSentinelGlyphID = 0 + kHashBits = 8, + kHashCount = 1 << kHashBits, + kHashMask = kHashCount - 1 }; - - // A quick lookup to avoid the binary search looking for glyphs in fGlyphArray. - uint16_t fGlyphHash[kHashCount]; - SkTDArray fGlyphArray; - SkChunkAlloc fGlyphAlloc; + SkGlyph* fGlyphHash[kHashCount]; + SkTDArray fGlyphArray; + SkChunkAlloc fGlyphAlloc; struct CharGlyphRec { uint32_t fID; // unichar + subpixel - uint16_t fGlyphIndex; + SkGlyph* fGlyph; }; - // no reason to use the same kHashCount as fGlyphHash, but we do for now // Dynamically allocated when chars are encountered. SkAutoTArray fCharToGlyphHash; - - // The id arg is a combined id generated by MakeID. + CharGlyphRec* getCharGlyphRec(uint32_t id); - void adjustCaches(int insertion_index); static inline unsigned ID2HashIndex(uint32_t h) { return SkChecksum::CheapMix(h) & kHashMask;