Cleanup code in SkChromeRemoteGlyphCache
* Add this->'s
* Make the logic in checkForDeletedEntries more robust.
* Harden readStrikeData by checking the typefaceID before doing
the descriptor typefaceID translation.
* Rename fRemoteFontIdToTypeface to fRemoteTypefaceIdToTypeface
* Simplify some SK_DEBUG code.
Change-Id: I8498f9be7ef4874f28f595222e661b9f8330b94f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540317
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Herb Derby <herb@google.com>
diff --git a/src/core/SkChromeRemoteGlyphCache.cpp b/src/core/SkChromeRemoteGlyphCache.cpp
index 9be90778..574e09e 100644
--- a/src/core/SkChromeRemoteGlyphCache.cpp
+++ b/src/core/SkChromeRemoteGlyphCache.cpp
@@ -100,7 +100,7 @@
bool readDescriptor(SkAutoDescriptor* ad) {
uint32_t descLength = 0u;
- if (!read<uint32_t>(&descLength)) return false;
+ if (!this->read<uint32_t>(&descLength)) return false;
auto* underlyingBuffer = this->ensureAtLeast(descLength, alignof(SkDescriptor));
if (!underlyingBuffer) return false;
@@ -148,12 +148,10 @@
: fTypefaceID{typefaceID}, fDiscardableHandleId(discardableHandleId) {}
SkTypefaceID fTypefaceID = 0u;
SkDiscardableHandleId fDiscardableHandleId = 0u;
- /* desc */
- /* n X (glyphs ids) */
};
// Represent a set of x sub-pixel-position glyphs with a glyph id < kMaxGlyphID and
-// y sub-pxiel-position must be 0. Most sub-pixel-positioned glyphs have been x-axis aligned
+// y sub-pixel-position must be 0. Most sub-pixel-positioned glyphs have been x-axis aligned
// forcing the y sub-pixel position to be zero. We can organize the SkPackedGlyphID to check that
// the glyph id and the y position == 0 with a single compare in the following way:
// <y-sub-pixel-position>:2 | <glyphid:16> | <x-sub-pixel-position>:2
@@ -461,9 +459,9 @@
SkPackedGlyphID packedID = variant.packedID();
if (fSentLowGlyphIDs.test(packedID)) {
#ifdef SK_DEBUG
- SkGlyphDigest* digest = fSentGlyphs.find(packedID);
- SkASSERT(digest != nullptr);
- SkASSERT(digest->canDrawAsMask() && digest->canDrawAsSDFT());
+ SkGlyphDigest* digest = fSentGlyphs.find(packedID);
+ SkASSERT(digest != nullptr);
+ SkASSERT(digest->canDrawAsMask() && digest->canDrawAsSDFT());
#endif
continue;
}
@@ -676,31 +674,20 @@
serializer.emplace<uint64_t>(SkTo<uint64_t>(strikesToSend));
fRemoteStrikesToSend.foreach (
-#ifdef SK_DEBUG
- [&](RemoteStrike* strike) {
- if (strike->hasPendingGlyphs()) {
- strike->writePendingGlyphs(&serializer);
- strike->resetScalerContext();
- }
+ [&](RemoteStrike* strike) {
+ if (strike->hasPendingGlyphs()) {
+ strike->writePendingGlyphs(&serializer);
+ strike->resetScalerContext();
+ }
+ #ifdef SK_DEBUG
auto it = fDescToRemoteStrike.find(&strike->getDescriptor());
SkASSERT(it != fDescToRemoteStrike.end());
SkASSERT(it->second.get() == strike);
- #if defined(SK_TRACE_GLYPH_RUN_PROCESS)
- msg.append(strike->getDescriptor().dumpRec());
- #endif
- }
-
-#else
- [&serializer](RemoteStrike* strike) {
- if (strike->hasPendingGlyphs()) {
- strike->writePendingGlyphs(&serializer);
- strike->resetScalerContext();
- }
- #if defined(SK_TRACE_GLYPH_RUN_PROCESS)
- msg.append(strike->getDescriptor().dumpRec());
- #endif
- }
-#endif
+ #endif
+ #if defined(SK_TRACE_GLYPH_RUN_PROCESS)
+ msg.append(strike->getDescriptor().dumpRec());
+ #endif
+ }
);
fRemoteStrikesToSend.reset();
#if defined(SK_TRACE_GLYPH_RUN_PROCESS)
@@ -719,17 +706,18 @@
it != fDescToRemoteStrike.end()) {
RemoteStrike* strike = it->second.get();
if (fDiscardableHandleManager->isHandleDeleted(strike->discardableHandleId())) {
- // If we are removing the strike, we better not be trying to send it at the same time.
- SkASSERT(!fRemoteStrikesToSend.contains(strike));
- it = fDescToRemoteStrike.erase(it);
- } else {
- ++it;
+ // If we are trying to send the strike, then do not erase it.
+ if (!fRemoteStrikesToSend.contains(strike)) {
+ // Erase returns the iterator following the removed element.
+ it = fDescToRemoteStrike.erase(it);
+ continue;
+ }
}
+ ++it;
}
}
RemoteStrike* SkStrikeServerImpl::getOrCreateCache(const SkStrikeSpec& strikeSpec) {
-
// In cases where tracing is turned off, make sure not to get an unused function warning.
// Lambdaize the function.
TRACE_EVENT1("skia", "RecForDesc", "rec",
@@ -744,8 +732,9 @@
)
);
- auto it = fDescToRemoteStrike.find(&strikeSpec.descriptor());
- if (it != fDescToRemoteStrike.end()) {
+ if (auto it = fDescToRemoteStrike.find(&strikeSpec.descriptor());
+ it != fDescToRemoteStrike.end())
+ {
// We have processed the RemoteStrike before. Reuse it.
RemoteStrike* strike = it->second.get();
strike->setStrikeSpec(strikeSpec);
@@ -761,6 +750,7 @@
return strike;
}
+ // If it wasn't locked, then forget this strike, and build it anew below.
fDescToRemoteStrike.erase(it);
}
@@ -949,7 +939,7 @@
static bool ReadGlyph(SkTLazy<SkGlyph>& glyph, Deserializer* deserializer);
sk_sp<SkTypeface> addTypeface(const WireTypeface& wire);
- SkTHashMap<SkTypefaceID, sk_sp<SkTypeface>> fRemoteFontIdToTypeface;
+ SkTHashMap<SkTypefaceID, sk_sp<SkTypeface>> fRemoteTypefaceIdToTypeface;
sk_sp<SkStrikeClient::DiscardableHandleManager> fDiscardableHandleManager;
SkStrikeCache* const fStrikeCache;
const bool fIsLogging;
@@ -1010,10 +1000,7 @@
WireTypeface wire;
if (!deserializer.read<WireTypeface>(&wire)) READ_FAILURE
- // TODO(khushalsagar): The typeface no longer needs a reference to the
- // SkStrikeClient, since all needed glyphs must have been pushed before
- // raster.
- addTypeface(wire);
+ this->addTypeface(wire);
}
#if defined(SK_TRACE_GLYPH_RUN_PROCESS)
@@ -1041,6 +1028,11 @@
if (!deserializer.read<SkFontMetrics>(&fontMetrics)) READ_FAILURE
}
+ // Preflight the TypefaceID before doing the Descriptor translation.
+ auto* tfPtr = fRemoteTypefaceIdToTypeface.find(spec.fTypefaceID);
+ // Received a TypefaceID for a typeface we don't know about.
+ if (!tfPtr) READ_FAILURE
+
// Replace the ContextRec in the desc from the server to create the client
// side descriptor.
if (!this->translateTypefaceID(&ad)) READ_FAILURE
@@ -1060,10 +1052,6 @@
// be an existing strike.
if (fontMetricsInitialized && strike == nullptr) READ_FAILURE
if (strike == nullptr) {
- // Get the local typeface from remote fontID.
- auto* tfPtr = fRemoteFontIdToTypeface.find(spec.fTypefaceID);
- // Received strikes for a typeface which doesn't exist.
- if (!tfPtr) READ_FAILURE
// Note that we don't need to deserialize the effects since we won't be generating any
// glyphs here anyway, and the desc is still correct since it includes the serialized
// effects.
@@ -1157,7 +1145,7 @@
SkScalerContextRec rec;
std::memcpy((void*)&rec, ptr, size);
// Get the local typeface from remote typefaceID.
- auto* tfPtr = fRemoteFontIdToTypeface.find(rec.fTypefaceID);
+ auto* tfPtr = fRemoteTypefaceIdToTypeface.find(rec.fTypefaceID);
// Received a strike for a typeface which doesn't exist.
if (!tfPtr) { return false; }
// Update the typeface id to work with the client side.
@@ -1178,13 +1166,13 @@
}
sk_sp<SkTypeface> SkStrikeClientImpl::addTypeface(const WireTypeface& wire) {
- auto* typeface = fRemoteFontIdToTypeface.find(wire.fTypefaceID);
+ auto* typeface = fRemoteTypefaceIdToTypeface.find(wire.fTypefaceID);
if (typeface) return *typeface;
auto newTypeface = sk_make_sp<SkTypefaceProxy>(
wire.fTypefaceID, wire.fGlyphCount, wire.fStyle, wire.fIsFixed,
wire.fGlyphMaskNeedsCurrentColor, fDiscardableHandleManager, fIsLogging);
- fRemoteFontIdToTypeface.set(wire.fTypefaceID, newTypeface);
+ fRemoteTypefaceIdToTypeface.set(wire.fTypefaceID, newTypeface);
return std::move(newTypeface);
}