Revert of Stop using SkScalerContext::getAdvance() in SkGlyphCache. (patchset #2 id:20001 of https://codereview.chromium.org/1321243004/ )
Reason for revert:
Suspect this is the cause of regressions in crbug.com/527445. It's triggering on Windows and Linux, where getting advances does less than getting full metrics. It's not triggering on Mac, where this CL was a no-op.
Original issue's description:
> Stop using SkScalerContext::getAdvance() in SkGlyphCache.
>
> We think it'll simplify things to just always get the full metrics.
> On most platforms, it's no different, and we think the platforms that
> do differ (FreeType) will be nearly just as cheap.
>
> Removing this distinction helps us make SkGlyphCaches concurrent by
> removing a state (we-have-only-advances) from its logical state machine.
>
> We see no significant changes running SKPs before and after this CL.
> That makes sense, of course, because the SKPs bake some of this into drawPosText.
>
> BUG=skia:
>
> Committed: https://skia.googlesource.com/skia/+/518a2923f11b819fa44ed5cff54155326959540f
TBR=reed@google.com,bungeman@google.com,herb@google.com,mtklein@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review URL: https://codereview.chromium.org/1383403003
diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp
index ed158d3..4ecbe2e 100644
--- a/src/core/SkGlyphCache.cpp
+++ b/src/core/SkGlyphCache.cpp
@@ -118,40 +118,40 @@
const SkGlyph& SkGlyphCache::getUnicharAdvance(SkUnichar charCode) {
VALIDATE();
- return *this->lookupByChar(charCode);
+ return *this->lookupByChar(charCode, kJustAdvance_MetricsType);
}
const SkGlyph& SkGlyphCache::getGlyphIDAdvance(uint16_t glyphID) {
VALIDATE();
PackedGlyphID packedGlyphID = SkGlyph::MakeID(glyphID);
- return *this->lookupByPackedGlyphID(packedGlyphID);
+ return *this->lookupByPackedGlyphID(packedGlyphID, kJustAdvance_MetricsType);
}
///////////////////////////////////////////////////////////////////////////////
const SkGlyph& SkGlyphCache::getUnicharMetrics(SkUnichar charCode) {
VALIDATE();
- return *this->lookupByChar(charCode);
+ return *this->lookupByChar(charCode, kFull_MetricsType);
}
const SkGlyph& SkGlyphCache::getUnicharMetrics(SkUnichar charCode, SkFixed x, SkFixed y) {
VALIDATE();
- return *this->lookupByChar(charCode, x, y);
+ return *this->lookupByChar(charCode, kFull_MetricsType, x, y);
}
const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID) {
VALIDATE();
PackedGlyphID packedGlyphID = SkGlyph::MakeID(glyphID);
- return *this->lookupByPackedGlyphID(packedGlyphID);
+ return *this->lookupByPackedGlyphID(packedGlyphID, kFull_MetricsType);
}
const SkGlyph& SkGlyphCache::getGlyphIDMetrics(uint16_t glyphID, SkFixed x, SkFixed y) {
VALIDATE();
PackedGlyphID packedGlyphID = SkGlyph::MakeID(glyphID, x, y);
- return *this->lookupByPackedGlyphID(packedGlyphID);
+ return *this->lookupByPackedGlyphID(packedGlyphID, kFull_MetricsType);
}
-SkGlyph* SkGlyphCache::lookupByChar(SkUnichar charCode, SkFixed x, SkFixed y) {
+SkGlyph* SkGlyphCache::lookupByChar(SkUnichar charCode, MetricsType type, SkFixed x, SkFixed y) {
PackedUnicharID id = SkGlyph::MakeID(charCode, x, y);
CharGlyphRec* rec = this->getCharGlyphRec(id);
if (rec->fPackedUnicharID != id) {
@@ -160,21 +160,26 @@
// this ID is based on the glyph index
PackedGlyphID combinedID = SkGlyph::MakeID(fScalerContext->charToGlyphID(charCode), x, y);
rec->fPackedGlyphID = combinedID;
- return this->lookupByPackedGlyphID(combinedID);
+ return this->lookupByPackedGlyphID(combinedID, type);
} else {
- return this->lookupByPackedGlyphID(rec->fPackedGlyphID);
+ return this->lookupByPackedGlyphID(rec->fPackedGlyphID, type);
}
}
-SkGlyph* SkGlyphCache::lookupByPackedGlyphID(PackedGlyphID packedGlyphID) {
+SkGlyph* SkGlyphCache::lookupByPackedGlyphID(PackedGlyphID packedGlyphID, MetricsType type) {
SkGlyph* glyph = fGlyphMap.find(packedGlyphID);
+
if (nullptr == glyph) {
- glyph = this->allocateNewGlyph(packedGlyphID);
+ glyph = this->allocateNewGlyph(packedGlyphID, type);
+ } else {
+ if (type == kFull_MetricsType && glyph->isJustAdvance()) {
+ fScalerContext->getMetrics(glyph);
+ }
}
return glyph;
}
-SkGlyph* SkGlyphCache::allocateNewGlyph(PackedGlyphID packedGlyphID) {
+SkGlyph* SkGlyphCache::allocateNewGlyph(PackedGlyphID packedGlyphID, MetricsType mtype) {
fMemoryUsed += sizeof(SkGlyph);
SkGlyph* glyphPtr;
@@ -183,7 +188,13 @@
glyph.initGlyphFromCombinedID(packedGlyphID);
glyphPtr = fGlyphMap.set(glyph);
}
- fScalerContext->getMetrics(glyphPtr);
+
+ if (kJustAdvance_MetricsType == mtype) {
+ fScalerContext->getAdvance(glyphPtr);
+ } else {
+ SkASSERT(kFull_MetricsType == mtype);
+ fScalerContext->getMetrics(glyphPtr);
+ }
SkASSERT(glyphPtr->fID != SkGlyph::kImpossibleID);
return glyphPtr;
diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h
index 4eb6b5b..2f1f417 100644
--- a/src/core/SkGlyphCache.h
+++ b/src/core/SkGlyphCache.h
@@ -181,6 +181,11 @@
private:
friend class SkGlyphCache_Globals;
+ enum MetricsType {
+ kJustAdvance_MetricsType,
+ kFull_MetricsType
+ };
+
enum {
kHashBits = 8,
kHashCount = 1 << kHashBits,
@@ -208,13 +213,15 @@
// 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 assumed to be zero.
- SkGlyph* lookupByPackedGlyphID(PackedGlyphID packedGlyphID);
+ SkGlyph* lookupByPackedGlyphID(PackedGlyphID packedGlyphID, MetricsType type);
// Return a SkGlyph* associated with unicode id and position x and y.
- SkGlyph* lookupByChar(SkUnichar id, SkFixed x = 0, SkFixed y = 0);
+ SkGlyph* lookupByChar(SkUnichar id, MetricsType type, SkFixed x = 0, SkFixed y = 0);
- // Return a new SkGlyph for the glyph ID and subpixel position id.
- SkGlyph* allocateNewGlyph(PackedGlyphID packedGlyphID);
+ // Return a new SkGlyph for the glyph ID and subpixel position id. Limit the amount
+ // of work
+ // using type.
+ SkGlyph* allocateNewGlyph(PackedGlyphID packedGlyphID, MetricsType type);
static bool DetachProc(const SkGlyphCache*, void*) { return true; }