From 653d2a4eee8b76655dd1180261cacdc1d88fe0ee Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Wed, 5 Feb 2020 13:47:43 -0500 Subject: [PATCH] Remove search of desperation Change-Id: Id862057ad5b853e979a9e93fc43a559360b400d9 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/268842 Commit-Queue: Herb Derby Reviewed-by: Mike Klein Reviewed-by: Khushal Sagar --- src/core/SkRemoteGlyphCache.h | 4 +- src/core/SkStrike.cpp | 14 ---- src/core/SkStrike.h | 5 -- src/core/SkStrikeCache.cpp | 80 --------------------- src/core/SkStrikeCache.h | 7 -- src/core/SkTypeface_remote.cpp | 31 +-------- tests/SkRemoteGlyphCacheTest.cpp | 116 ------------------------------- 7 files changed, 4 insertions(+), 253 deletions(-) diff --git a/src/core/SkRemoteGlyphCache.h b/src/core/SkRemoteGlyphCache.h index d78b431cc3..4a3bd36f18 100644 --- a/src/core/SkRemoteGlyphCache.h +++ b/src/core/SkRemoteGlyphCache.h @@ -176,11 +176,11 @@ public: kGlyphImage = 2, kGlyphPath = 3, - // The original glyph could not be found and a fallback was used. + // (DEPRECATED) The original glyph could not be found and a fallback was used. kGlyphMetricsFallback = 4, kGlyphPathFallback = 5, - kLast = kGlyphPathFallback + kLast = kGlyphPath }; // An interface to delete handles that may be pinned by the remote server. diff --git a/src/core/SkStrike.cpp b/src/core/SkStrike.cpp index cf49926aac..7f828ef41a 100644 --- a/src/core/SkStrike.cpp +++ b/src/core/SkStrike.cpp @@ -124,20 +124,6 @@ SkGlyph* SkStrike::mergeGlyphAndImage(SkPackedGlyphID toID, const SkGlyph& from) return glyph; } -const SkGlyph* SkStrike::getCachedGlyphAnySubPix(SkGlyphID glyphID, SkPackedGlyphID vetoID) const { - for (SkFixed subY = 0; subY < SK_Fixed1; subY += SK_FixedQuarter) { - for (SkFixed subX = 0; subX < SK_Fixed1; subX += SK_FixedQuarter) { - SkPackedGlyphID packedGlyphID{glyphID, subX, subY}; - if (packedGlyphID == vetoID) continue; - if (SkGlyph* glyphPtr = fGlyphMap.findOrNull(packedGlyphID)) { - return glyphPtr; - } - } - } - - return nullptr; -} - SkSpan SkStrike::metrics(SkSpan glyphIDs, const SkGlyph* results[]) { return this->internalPrepare(glyphIDs, kMetricsOnly, results); diff --git a/src/core/SkStrike.h b/src/core/SkStrike.h index 0ae57ac9c6..20d0d3fa5c 100644 --- a/src/core/SkStrike.h +++ b/src/core/SkStrike.h @@ -63,11 +63,6 @@ public: void findIntercepts(const SkScalar bounds[2], SkScalar scale, SkScalar xPos, SkGlyph* , SkScalar* array, int* count); - /** Find any glyph in this cache with the given ID, regardless of subpixel positioning. - * If set and present, skip over the glyph with vetoID. - */ - const SkGlyph* getCachedGlyphAnySubPix(SkGlyphID, - SkPackedGlyphID vetoID = SkPackedGlyphID()) const; /** Return the vertical metrics for this strike. */ diff --git a/src/core/SkStrikeCache.cpp b/src/core/SkStrikeCache.cpp index 482776d5cb..205ae28e7a 100644 --- a/src/core/SkStrikeCache.cpp +++ b/src/core/SkStrikeCache.cpp @@ -279,84 +279,6 @@ auto SkStrikeCache::findAndDetachStrike(const SkDescriptor& desc) -> Node* { return nullptr; } - -static bool loose_compare(const SkDescriptor& lhs, const SkDescriptor& rhs) { - uint32_t size; - auto ptr = lhs.findEntry(kRec_SkDescriptorTag, &size); - SkScalerContextRec lhsRec; - std::memcpy((void*)&lhsRec, ptr, size); - - ptr = rhs.findEntry(kRec_SkDescriptorTag, &size); - SkScalerContextRec rhsRec; - std::memcpy((void*)&rhsRec, ptr, size); - - // If these don't match, there's no way we can use these strikes interchangeably. - // Note that a typeface from each renderer maps to a unique proxy typeface on the GPU, - // keyed in the glyph cache using fontID in the SkDescriptor. By limiting this search - // to descriptors with the same fontID, we ensure that a renderer never uses glyphs - // generated by a different renderer. - return - lhsRec.fFontID == rhsRec.fFontID && - lhsRec.fTextSize == rhsRec.fTextSize && - lhsRec.fPreScaleX == rhsRec.fPreScaleX && - lhsRec.fPreSkewX == rhsRec.fPreSkewX && - lhsRec.fPost2x2[0][0] == rhsRec.fPost2x2[0][0] && - lhsRec.fPost2x2[0][1] == rhsRec.fPost2x2[0][1] && - lhsRec.fPost2x2[1][0] == rhsRec.fPost2x2[1][0] && - lhsRec.fPost2x2[1][1] == rhsRec.fPost2x2[1][1]; -} - -bool SkStrikeCache::desperationSearchForImage(const SkDescriptor& desc, SkGlyph* glyph, - SkStrike* targetCache) { - SkAutoSpinlock ac(fLock); - - SkGlyphID glyphID = glyph->getGlyphID(); - for (Node* node = fHead; node != nullptr; node = node->fNext) { - if (loose_compare(node->fStrike.getDescriptor(), desc)) { - if (SkGlyph *fallback = node->fStrike.glyphOrNull(glyph->getPackedID())) { - // This desperate-match node may disappear as soon as we drop fLock, so we - // need to copy the glyph from node into this strike, including a - // deep copy of the mask. - targetCache->mergeGlyphAndImage(glyph->getPackedID(), *fallback); - return true; - } - - // Look for any sub-pixel pos for this glyph, in case there is a pos mismatch. - if (const auto* fallback = node->fStrike.getCachedGlyphAnySubPix(glyphID)) { - targetCache->mergeGlyphAndImage(glyph->getPackedID(), *fallback); - return true; - } - } - } - - return false; -} - -bool SkStrikeCache::desperationSearchForPath( - const SkDescriptor& desc, SkGlyphID glyphID, SkPath* path) { - SkAutoSpinlock ac(fLock); - - // The following is wrong there is subpixel positioning with paths... - // Paths are only ever at sub-pixel position (0,0), so we can just try that directly rather - // than try our packed position first then search all others on failure like for masks. - // - // This will have to search the sub-pixel positions too. - // There is also a problem with accounting for cache size with shared path data. - for (Node* node = fHead; node != nullptr; node = node->fNext) { - if (loose_compare(node->fStrike.getDescriptor(), desc)) { - if (SkGlyph *from = node->fStrike.glyphOrNull(SkPackedGlyphID{glyphID})) { - if (from->setPathHasBeenCalled() && from->path() != nullptr) { - // We can just copy the path out by value here, so no need to worry - // about the lifetime of this desperate-match node. - *path = *from->path(); - return true; - } - } - } - } - return false; -} - SkExclusiveStrikePtr SkStrikeCache::createStrikeExclusive( const SkDescriptor& desc, std::unique_ptr scaler, @@ -588,5 +510,3 @@ void SkStrikeCache::validate() const { } } #endif - -//////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/core/SkStrikeCache.h b/src/core/SkStrikeCache.h index 47b5f2629a..db5ee6be04 100644 --- a/src/core/SkStrikeCache.h +++ b/src/core/SkStrikeCache.h @@ -82,13 +82,6 @@ public: const SkScalerContextEffects& effects, const SkTypeface& typeface); - // Routines to find suitable data when working in a remote cache situation. These are - // suitable as substitutes for similar calls in SkScalerContext. - bool desperationSearchForImage(const SkDescriptor& desc, - SkGlyph* glyph, - SkStrike* targetCache); - bool desperationSearchForPath(const SkDescriptor& desc, SkGlyphID glyphID, SkPath* path); - SkScopedStrikeForGPU findOrCreateScopedStrike(const SkDescriptor& desc, const SkScalerContextEffects& effects, const SkTypeface& typeface) override; diff --git a/src/core/SkTypeface_remote.cpp b/src/core/SkTypeface_remote.cpp index dd6c2e2e78..c08c2e9e0f 100644 --- a/src/core/SkTypeface_remote.cpp +++ b/src/core/SkTypeface_remote.cpp @@ -42,27 +42,6 @@ void SkScalerContextProxy::generateMetrics(SkGlyph* glyph) { } glyph->fMaskFormat = fRec.fMaskFormat; - - // Since the scaler context is being called, we don't have the needed data. Try to find a - // fallback before failing. - if (fCache && fCache->glyphOrNull(glyph->getPackedID()) != nullptr) { - // First check the original cache, in case there is a sub-pixel pos mismatch. - if (const SkGlyph* from = - fCache->getCachedGlyphAnySubPix(glyph->getGlyphID(), glyph->getPackedID())) { - fCache->mergeGlyphAndImage(glyph->getPackedID(), *from); - fDiscardableManager->notifyCacheMiss( - SkStrikeClient::CacheMissType::kGlyphMetricsFallback); - return; - } - - // Now check other caches for a desc mismatch. - if (fStrikeCache->desperationSearchForImage(fCache->getDescriptor(), glyph, fCache)) { - fDiscardableManager->notifyCacheMiss( - SkStrikeClient::CacheMissType::kGlyphMetricsFallback); - return; - } - } - glyph->zeroMetrics(); fDiscardableManager->notifyCacheMiss(SkStrikeClient::CacheMissType::kGlyphMetrics); } @@ -84,14 +63,8 @@ bool SkScalerContextProxy::generatePath(SkGlyphID glyphID, SkPath* path) { SkDebugf("GlyphCacheMiss generatePath: %s\n", this->getRec().dump().c_str()); } - // Since the scaler context is being called, we don't have the needed data. Try to find a - // fallback before failing. - auto desc = SkScalerContext::DescriptorGivenRecAndEffects(this->getRec(), this->getEffects()); - bool foundPath = fStrikeCache && fStrikeCache->desperationSearchForPath(*desc, glyphID, path); - fDiscardableManager->notifyCacheMiss(foundPath - ? SkStrikeClient::CacheMissType::kGlyphPathFallback - : SkStrikeClient::CacheMissType::kGlyphPath); - return foundPath; + fDiscardableManager->notifyCacheMiss(SkStrikeClient::CacheMissType::kGlyphPath); + return false; } void SkScalerContextProxy::generateFontMetrics(SkFontMetrics* metrics) { diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp index fd327e8624..c3b21c2258 100644 --- a/tests/SkRemoteGlyphCacheTest.cpp +++ b/tests/SkRemoteGlyphCacheTest.cpp @@ -821,122 +821,6 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkRemoteGlyphCache_TypefaceWithNoPaths, repor discardableManager->unlockAndDeleteAll(); } -DEF_TEST(SkRemoteGlyphCache_SearchOfDesperation, reporter) { - // Build proxy typeface on the client for initializing the cache. - sk_sp discardableManager = sk_make_sp(); - SkStrikeServer server(discardableManager.get()); - SkStrikeClient client(discardableManager, false); - - auto serverTf = SkTypeface::MakeFromName("monospace", SkFontStyle()); - auto tfData = server.serializeTypeface(serverTf.get()); - auto clientTf = client.deserializeTypeface(tfData->data(), tfData->size()); - REPORTER_ASSERT(reporter, clientTf); - - SkFont font; - font.setTypeface(clientTf); - font.setSubpixel(true); - SkPaint paint; - paint.setAntiAlias(true); - paint.setColor(SK_ColorRED); - - auto lostGlyphID = SkPackedGlyphID(1, SK_FixedHalf, SK_FixedHalf); - const uint8_t glyphImage[] = {0xFF, 0xFF}; - - SkStrikeCache strikeCache; - - // Build a fallback cache. - { - SkAutoDescriptor ad; - SkScalerContextRec rec; - SkScalerContextEffects effects; - SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; - SkScalerContext::MakeRecAndEffects( - font, paint, SkSurfacePropsCopyOrDefault(nullptr), flags, - SkMatrix::I(), &rec, &effects); - auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); - - auto fallbackCache = strikeCache.findOrCreateStrikeExclusive(*desc, effects, *clientTf); - SkGlyphPrototype proto = {lostGlyphID, 0.f, 0.f, 2, 1, 0, 0, SkMask::kA8_Format, false}; - fallbackCache->glyphFromPrototype(proto, (void*)glyphImage); - } - - // Make sure we can find the fall back cache. - { - SkAutoDescriptor ad; - SkScalerContextRec rec; - SkScalerContextEffects effects; - SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; - SkScalerContext::MakeRecAndEffects( - font, paint, SkSurfacePropsCopyOrDefault(nullptr), flags, - SkMatrix::I(), &rec, &effects); - auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); - auto testCache = strikeCache.findStrikeExclusive(*desc); - REPORTER_ASSERT(reporter, !(testCache == nullptr)); - } - - // Create the target cache. - SkExclusiveStrikePtr testCache; - SkAutoDescriptor ad; - SkScalerContextRec rec; - SkScalerContextEffects effects; - SkScalerContextFlags flags = SkScalerContextFlags::kNone; - SkScalerContext::MakeRecAndEffects( - font, paint, SkSurfacePropsCopyOrDefault(nullptr), flags, - SkMatrix::I(), &rec, &effects); - auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); - testCache = strikeCache.findStrikeExclusive(*desc); - REPORTER_ASSERT(reporter, testCache == nullptr); - testCache = strikeCache.createStrikeExclusive(*desc, - clientTf->createScalerContext(effects, desc)); - auto scalerProxy = static_cast(testCache->getScalerContext()); - scalerProxy->initCache(testCache.get(), &strikeCache); - - auto rounding = testCache->roundingSpec(); - SkBulkGlyphMetricsAndImages metricsAndImages{std::move(testCache)}; - - // Look for the lost glyph. - { - SkPoint pt{SkFixedToScalar(lostGlyphID.getSubXFixed()), - SkFixedToScalar(lostGlyphID.getSubYFixed())}; - SkPackedGlyphID packedID{ - lostGlyphID.glyphID(), pt, rounding.ignorePositionFieldMask}; - - const SkGlyph* lostGlyph = metricsAndImages.glyph(packedID); - - REPORTER_ASSERT(reporter, lostGlyph->height() == 1); - REPORTER_ASSERT(reporter, lostGlyph->width() == 2); - REPORTER_ASSERT(reporter, lostGlyph->maskFormat() == SkMask::kA8_Format); - REPORTER_ASSERT(reporter, memcmp(lostGlyph->image(), glyphImage, sizeof(glyphImage)) == 0); - } - - // Look for the lost glyph with a different sub-pix position. - { - SkPoint pt{SkFixedToScalar(SK_FixedQuarter), - SkFixedToScalar(SK_FixedQuarter)}; - SkPackedGlyphID packedID{ - lostGlyphID.glyphID(), pt, rounding.ignorePositionFieldMask}; - const SkGlyph* lostGlyph = metricsAndImages.glyph(packedID); - - REPORTER_ASSERT(reporter, lostGlyph->height() == 1); - REPORTER_ASSERT(reporter, lostGlyph->width() == 2); - REPORTER_ASSERT(reporter, lostGlyph->maskFormat() == SkMask::kA8_Format); - REPORTER_ASSERT(reporter, memcmp(lostGlyph->image(), glyphImage, sizeof(glyphImage)) == 0); - } - - for (uint32_t i = 0; i <= SkStrikeClient::CacheMissType::kLast; ++i) { - if (i == SkStrikeClient::CacheMissType::kGlyphMetricsFallback || - i == SkStrikeClient::CacheMissType::kFontMetrics) { - REPORTER_ASSERT(reporter, discardableManager->cacheMissCount(i) == 2); - } else { - REPORTER_ASSERT(reporter, discardableManager->cacheMissCount(i) == 0); - } - } - strikeCache.validateGlyphCacheDataSize(); - - // Must unlock everything on termination, otherwise valgrind complains about memory leaks. - discardableManager->unlockAndDeleteAll(); -} - DEF_TEST(SkRemoteGlyphCache_ReWriteGlyph, reporter) { // Build proxy typeface on the client for initializing the cache. sk_sp discardableManager = sk_make_sp();