From fc6cf92e4b21c92ead769fae557534056eac6d83 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Wed, 23 May 2018 00:16:47 +0000 Subject: [PATCH] Revert "fonts: Cleanup cache miss logging for font remoting." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 2b8a0d1844ed236259284fbc2efe08dab0ed539f. Reason for revert: reverting a CL this builds on Original change's description: > fonts: Cleanup cache miss logging for font remoting. > > Add hooks to notify the embedder if there is a cache miss during draw. > Also remove the reference to SkStrikeClient from SkTypefaceProxy and > SkScalerContextProxy, since the proxies can outlive the client. > > R=​herb@google.com > > Bug: 829622 > Change-Id: Ib2fd1b91ebd057856c1d4e717cf50b49f08c903b > Reviewed-on: https://skia-review.googlesource.com/129402 > Commit-Queue: Khusal Sagar > Reviewed-by: Herb Derby TBR=herb@google.com,khushalsagar@chromium.org Change-Id: I8a331545988885c620685008f4b60240d80f3712 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 829622 Reviewed-on: https://skia-review.googlesource.com/129682 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- src/core/SkRemoteGlyphCache.cpp | 35 ++++++++++++++++++++++++++++++- src/core/SkRemoteGlyphCache.h | 23 +++++++++++--------- src/core/SkTypeface_remote.cpp | 36 ++++++++++---------------------- src/core/SkTypeface_remote.h | 35 ++++++++++++++++++++----------- tests/SkRemoteGlyphCacheTest.cpp | 34 ++---------------------------- 5 files changed, 83 insertions(+), 80 deletions(-) diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index 216a2a9dab..d5e96fce88 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -801,7 +801,40 @@ sk_sp SkStrikeClient::addTypeface(const WireTypeface& wire) { if (typeface) return *typeface; auto newTypeface = sk_make_sp(wire.typefaceID, wire.glyphCount, wire.style, - wire.isFixed, fDiscardableHandleManager); + wire.isFixed, this); fRemoteFontIdToTypeface.set(wire.typefaceID, newTypeface); return std::move(newTypeface); } + +void SkStrikeClient::generateFontMetrics(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkPaint::FontMetrics* metrics) { + TRACE_EVENT1("skia", "generateFontMetrics", "rec", TRACE_STR_COPY(rec.dump().c_str())); + SkDebugf("generateFontMetrics: %s\n", rec.dump().c_str()); + SkStrikeCache::Dump(); + SkDEBUGFAIL("GlyphCacheMiss"); + + sk_bzero(metrics, sizeof(*metrics)); +} + +void SkStrikeClient::generateMetricsAndImage(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkArenaAlloc* alloc, + SkGlyph* glyph) { + TRACE_EVENT1("skia", "generateMetricsAndImage", "rec", TRACE_STR_COPY(rec.dump().c_str())); + SkDebugf("generateMetricsAndImage: %s\n", rec.dump().c_str()); + SkStrikeCache::Dump(); + SkDEBUGFAIL("GlyphCacheMiss"); + + glyph->zeroMetrics(); +} + +void SkStrikeClient::generatePath(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkGlyphID glyphID, + SkPath* path) { + TRACE_EVENT1("skia", "generatePath", "rec", TRACE_STR_COPY(rec.dump().c_str())); + SkDebugf("generatePath: %s\n", rec.dump().c_str()); + SkStrikeCache::Dump(); + SkDEBUGFAIL("GlyphCacheMiss"); +} diff --git a/src/core/SkRemoteGlyphCache.h b/src/core/SkRemoteGlyphCache.h index 32d0b47c18..d1119cf70a 100644 --- a/src/core/SkRemoteGlyphCache.h +++ b/src/core/SkRemoteGlyphCache.h @@ -189,14 +189,6 @@ private: class SK_API SkStrikeClient { public: - enum CacheMissType : uint32_t { - kFontMetrics, - kGlyphMetrics, - kGlyphImage, - kGlyphPath, - kLast = kGlyphPath - }; - // An interface to delete handles that may be pinned by the remote server. class DiscardableHandleManager : public SkRefCnt { public: @@ -205,8 +197,6 @@ public: // Returns true if the handle was unlocked and can be safely deleted. Once // successful, subsequent attempts to delete the same handle are invalid. virtual bool deleteHandle(SkDiscardableHandleId) = 0; - - virtual void NotifyCacheMiss(CacheMissType) {} }; SkStrikeClient(sk_sp); @@ -222,6 +212,19 @@ public: // Returns false if the data is invalid. bool readStrikeData(const volatile void* memory, size_t memorySize); + // TODO: Remove these since we don't support pulling this data on-demand. + void generateFontMetrics(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkPaint::FontMetrics* metrics); + void generateMetricsAndImage(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkArenaAlloc* alloc, + SkGlyph* glyph); + void generatePath(const SkTypefaceProxy& typefaceProxy, + const SkScalerContextRec& rec, + SkGlyphID glyphID, + SkPath* path); + private: class DiscardableStrikePinner; diff --git a/src/core/SkTypeface_remote.cpp b/src/core/SkTypeface_remote.cpp index cbddb1fcaa..fe3fde83f5 100644 --- a/src/core/SkTypeface_remote.cpp +++ b/src/core/SkTypeface_remote.cpp @@ -6,16 +6,15 @@ */ #include "SkTypeface_remote.h" -#include "SkPaint.h" #include "SkRemoteGlyphCache.h" -#include "SkStrikeCache.h" -#include "SkTraceEvent.h" + +#include "SkPaint.h" SkScalerContextProxy::SkScalerContextProxy(sk_sp tf, const SkScalerContextEffects& effects, const SkDescriptor* desc, - sk_sp manager) - : SkScalerContext{std::move(tf), effects, desc}, fDiscardableManager{std::move(manager)} {} + SkStrikeClient* rsc) + : SkScalerContext{std::move(tf), effects, desc}, fClient{rsc} {} unsigned SkScalerContextProxy::generateGlyphCount() { SK_ABORT("Should never be called."); @@ -32,34 +31,21 @@ void SkScalerContextProxy::generateAdvance(SkGlyph* glyph) { } void SkScalerContextProxy::generateMetrics(SkGlyph* glyph) { - TRACE_EVENT1("skia", "generateMetrics", "rec", TRACE_STR_COPY(this->getRec().dump().c_str())); - SkDebugf("GlyphCacheMiss generateMetrics: %s\n", this->getRec().dump().c_str()); - - fDiscardableManager->NotifyCacheMiss(SkStrikeClient::CacheMissType::kGlyphMetrics); - glyph->zeroMetrics(); + fClient->generateMetricsAndImage(*this->typefaceProxy(), this->getRec(), &fAlloc, glyph); } void SkScalerContextProxy::generateImage(const SkGlyph& glyph) { - TRACE_EVENT1("skia", "generateImage", "rec", TRACE_STR_COPY(this->getRec().dump().c_str())); - SkDebugf("GlyphCacheMiss generateImage: %s\n", this->getRec().dump().c_str()); - - fDiscardableManager->NotifyCacheMiss(SkStrikeClient::CacheMissType::kGlyphImage); } bool SkScalerContextProxy::generatePath(SkGlyphID glyphID, SkPath* path) { - TRACE_EVENT1("skia", "generatePath", "rec", TRACE_STR_COPY(this->getRec().dump().c_str())); - SkDebugf("GlyphCacheMiss generatePath: %s\n", this->getRec().dump().c_str()); - - fDiscardableManager->NotifyCacheMiss(SkStrikeClient::CacheMissType::kGlyphPath); + fClient->generatePath(*this->typefaceProxy(), this->getRec(), glyphID, path); return false; } void SkScalerContextProxy::generateFontMetrics(SkPaint::FontMetrics* metrics) { - TRACE_EVENT1( - "skia", "generateFontMetrics", "rec", TRACE_STR_COPY(this->getRec().dump().c_str())); - SkDebugf("GlyphCacheMiss generateFontMetrics: %s\n", this->getRec().dump().c_str()); - SkDEBUGCODE(SkStrikeCache::Dump()); - - fDiscardableManager->NotifyCacheMiss(SkStrikeClient::CacheMissType::kFontMetrics); - sk_bzero(metrics, sizeof(*metrics)); + fClient->generateFontMetrics(*this->typefaceProxy(), this->getRec(), metrics); +} + +SkTypefaceProxy* SkScalerContextProxy::typefaceProxy() { + return SkTypefaceProxy::DownCast(this->getTypeface()); } diff --git a/src/core/SkTypeface_remote.h b/src/core/SkTypeface_remote.h index ff5bc66775..ad729aded3 100644 --- a/src/core/SkTypeface_remote.h +++ b/src/core/SkTypeface_remote.h @@ -13,16 +13,18 @@ #include "SkFontDescriptor.h" #include "SkFontStyle.h" #include "SkPaint.h" -#include "SkRemoteGlyphCache.h" #include "SkScalerContext.h" #include "SkTypeface.h" +class SkStrikeClient; +class SkTypefaceProxy; + class SkScalerContextProxy : public SkScalerContext { public: SkScalerContextProxy(sk_sp tf, const SkScalerContextEffects& effects, const SkDescriptor* desc, - sk_sp manager); + SkStrikeClient* rsc); protected: unsigned generateGlyphCount() override; @@ -40,8 +42,10 @@ private: static constexpr size_t kMinGlyphImageSize = 16 /* height */ * 8 /* width */; static constexpr size_t kMinAllocAmount = kMinGlyphImageSize * kMinGlyphCount; + SkTypefaceProxy* typefaceProxy(); + SkArenaAlloc fAlloc{kMinAllocAmount}; - sk_sp fDiscardableManager; + SkStrikeClient* const fClient; typedef SkScalerContext INHERITED; }; @@ -51,13 +55,14 @@ public: int glyphCount, const SkFontStyle& style, bool isFixed, - sk_sp manager) - : INHERITED{style, false} - , fFontId{fontId} - , fGlyphCount{glyphCount} - , fDiscardableManager{std::move(manager)} {} + SkStrikeClient* rsc) + : INHERITED{style, false}, fFontId{fontId}, fGlyphCount{glyphCount}, fRsc{rsc} {} SkFontID remoteTypefaceID() const {return fFontId;} int glyphCount() const {return fGlyphCount;} + static SkTypefaceProxy* DownCast(SkTypeface* typeface) { + // TODO: how to check the safety of the down cast? + return (SkTypefaceProxy*)typeface; + } protected: int onGetUPEM() const override { SK_ABORT("Should never be called."); return 0; } @@ -92,11 +97,16 @@ protected: SkScalerContext* onCreateScalerContext(const SkScalerContextEffects& effects, const SkDescriptor* desc) const override { return new SkScalerContextProxy(sk_ref_sp(const_cast(this)), effects, - desc, fDiscardableManager); + desc, fRsc); + } void onFilterRec(SkScalerContextRec* rec) const override { - // The rec filtering is already applied by the server when generating - // the glyphs. + // Add all the device information here. + //rec->fPost2x2[0][0] = 0.5f; + + // This would be the best place to run the host SkTypeface_* onFilterRec. + // Can we move onFilterRec to the FongMgr, that way we don't need to cross the boundary to + // filter. } void onGetFontDescriptor(SkFontDescriptor*, bool*) const override { SK_ABORT("Should never be called."); @@ -128,7 +138,8 @@ private: const SkFontID fFontId; const int fGlyphCount; - sk_sp fDiscardableManager; + // TODO: Does this need a ref to the strike client? If yes, make it a weak ref. + SkStrikeClient* const fRsc; typedef SkTypeface INHERITED; }; diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp index 6a4a4d37cf..5d4b9ff6f3 100644 --- a/tests/SkRemoteGlyphCacheTest.cpp +++ b/tests/SkRemoteGlyphCacheTest.cpp @@ -22,7 +22,7 @@ class DiscardableManager : public SkStrikeServer::DiscardableHandleManager, public SkStrikeClient::DiscardableHandleManager { public: - DiscardableManager() { sk_bzero(&fCacheMissCount, sizeof(fCacheMissCount)); } + DiscardableManager() = default; ~DiscardableManager() override = default; // Server implementation. @@ -39,7 +39,6 @@ public: // Client implementation. bool deleteHandle(SkDiscardableHandleId id) override { return id <= fLastDeletedHandleId; } - void NotifyCacheMiss(SkStrikeClient::CacheMissType type) override { fCacheMissCount[type]++; } void unlockAll() { fLockedHandles.reset(); } void unlockAndDeleteAll() { @@ -48,13 +47,11 @@ public: } const SkTHashSet& lockedHandles() const { return fLockedHandles; } SkDiscardableHandleId handleCount() { return fNextHandleId; } - int cacheMissCount(SkStrikeClient::CacheMissType type) { return fCacheMissCount[type]; } private: SkDiscardableHandleId fNextHandleId = 0u; SkDiscardableHandleId fLastDeletedHandleId = 0u; SkTHashSet fLockedHandles; - int fCacheMissCount[SkStrikeClient::CacheMissType::kLast + 1u]; }; sk_sp buildTextBlob(sk_sp tf, int glyphCount) { @@ -117,7 +114,7 @@ DEF_TEST(SkRemoteGlyphCache_TypefaceSerialization, reporter) { auto client_tf = client.deserializeTypeface(tf_data->data(), tf_data->size()); REPORTER_ASSERT(reporter, client_tf); - REPORTER_ASSERT(reporter, static_cast(client_tf.get())->remoteTypefaceID() == + REPORTER_ASSERT(reporter, SkTypefaceProxy::DownCast(client_tf.get())->remoteTypefaceID() == server_tf->uniqueID()); } @@ -352,31 +349,4 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkRemoteGlyphCache_DrawTextAsDFT, reporter, c COMPARE_BLOBS(expected, actual, reporter); SkStrikeCache::Validate(); } - -DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkRemoteGlyphCache_CacheMissReporting, reporter, ctxInfo) { - sk_sp discardableManager = sk_make_sp(); - SkStrikeServer server(discardableManager.get()); - SkStrikeClient client(discardableManager); - - auto serverTf = SkTypeface::MakeFromName("monospace", SkFontStyle()); - auto tfData = server.serializeTypeface(serverTf.get()); - auto clientTf = client.deserializeTypeface(tfData->data(), tfData->size()); - REPORTER_ASSERT(reporter, clientTf); - int glyphCount = 10; - auto clientBlob = buildTextBlob(clientTf, glyphCount); - - // Raster the client-side blob without the glyph data, we should get cache miss notifications. - SkPaint paint; - SkMatrix matrix = SkMatrix::I(); - RasterBlob(clientBlob, 10, 10, paint, ctxInfo.grContext(), &matrix); - REPORTER_ASSERT(reporter, - discardableManager->cacheMissCount(SkStrikeClient::kFontMetrics) == 1); - REPORTER_ASSERT(reporter, - discardableManager->cacheMissCount(SkStrikeClient::kGlyphMetrics) == 10); - - // There shouldn't be any image or path requests, since we mark the glyph as empty on a cache - // miss. - REPORTER_ASSERT(reporter, discardableManager->cacheMissCount(SkStrikeClient::kGlyphImage) == 0); - REPORTER_ASSERT(reporter, discardableManager->cacheMissCount(SkStrikeClient::kGlyphPath) == 0); -} #endif