diff options
author | Khushal <khushalsagar@chromium.org> | 2018-06-13 09:44:23 -0700 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2018-06-13 17:10:56 +0000 |
commit | 4010c792c6bd0378087225ae16b0131bd2c303f2 (patch) | |
tree | 0f09301a1539664603f285ad991e677abda04ebc | |
parent | c6530d1e5ecce640e0255d66a01f9db73add7df9 (diff) |
fonts: Ignore re-initialization of fallback glyphs from the server.
If the client finds that the server re-initializes a cached path/image,
we consider this an error and invalid data. But since we might
initialize a glyph using a fallback on the client, and receive the
correct version from the server later, this is not longer true.
Bug: 829622
Change-Id: I34ab17b54139d89a15179265d4aed4a1fe36fd47
Reviewed-on: https://skia-review.googlesource.com/133566
Commit-Queue: Khusal Sagar <khushalsagar@chromium.org>
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
-rw-r--r-- | src/core/SkGlyph.h | 10 | ||||
-rw-r--r-- | src/core/SkGlyphCache.cpp | 12 | ||||
-rw-r--r-- | src/core/SkGlyphCache.h | 9 | ||||
-rw-r--r-- | src/core/SkRemoteGlyphCache.cpp | 31 | ||||
-rw-r--r-- | tests/SkRemoteGlyphCacheTest.cpp | 101 |
5 files changed, 140 insertions, 23 deletions
diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h index 06c0e422ce..3ea795b928 100644 --- a/src/core/SkGlyph.h +++ b/src/core/SkGlyph.h @@ -85,6 +85,12 @@ struct SkPackedID { return SkChecksum::CheapMix(fID); } + SkString dump() const { + SkString str; + str.appendf("code: %d, x: %d, y:%d", code(), getSubXFixed(), getSubYFixed()); + return str; + } + private: static unsigned ID2SubX(uint32_t id) { return id >> (kSubShift + kSubShiftX); @@ -143,8 +149,8 @@ class SkGlyph { public: static const SkFixed kSubpixelRound = SK_FixedHalf >> SkPackedID::kSubBits; - void* fImage; - PathData* fPathData; + void* fImage; + PathData* fPathData; float fAdvanceX, fAdvanceY; uint16_t fWidth, fHeight; diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp index 6b8bd7098a..e5c871fd7d 100644 --- a/src/core/SkGlyphCache.cpp +++ b/src/core/SkGlyphCache.cpp @@ -201,8 +201,10 @@ const void* SkGlyphCache::findImage(const SkGlyph& glyph) { return glyph.fImage; } -bool SkGlyphCache::initializeImage(const volatile void* data, size_t size, SkGlyph* glyph) { - if (glyph->fImage) return false; +void SkGlyphCache::initializeImage(const volatile void* data, size_t size, SkGlyph* glyph) { + // Don't overwrite the image if we already have one. We could have used a fallback if the + // glyph was missing earlier. + if (glyph->fImage) return; if (glyph->fWidth > 0 && glyph->fWidth < kMaxGlyphWidth) { size_t allocSize = glyph->allocImage(&fAlloc); @@ -213,8 +215,6 @@ bool SkGlyphCache::initializeImage(const volatile void* data, size_t size, SkGly fMemoryUsed += size; } } - - return true; } const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) { @@ -237,7 +237,9 @@ const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) { } bool SkGlyphCache::initializePath(SkGlyph* glyph, const volatile void* data, size_t size) { - if (glyph->fPathData) return false; + // Don't overwrite the path if we already have one. We could have used a fallback if the + // glyph was missing earlier. + if (glyph->fPathData) return true; if (glyph->fWidth) { SkGlyph::PathData* pathData = fAlloc.make<SkGlyph::PathData>(); diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h index 47a1d3d4d2..612ddb48c6 100644 --- a/src/core/SkGlyphCache.h +++ b/src/core/SkGlyphCache.h @@ -91,10 +91,9 @@ public: */ const void* findImage(const SkGlyph&); - /** Initializes the image associated with the glyph with |data|. Returns false if an image - * already exists. + /** Initializes the image associated with the glyph with |data|. */ - bool initializeImage(const volatile void* data, size_t size, SkGlyph*); + void initializeImage(const volatile void* data, size_t size, SkGlyph*); /** If the advance axis intersects the glyph's path, append the positions scaled and offset to the array (if non-null), and set the count to the updated array length. @@ -107,8 +106,8 @@ public: */ const SkPath* findPath(const SkGlyph&); - /** Initializes the path associated with the glyph with |data|. Returns false if a path - * already exits or data is invalid. + /** Initializes the path associated with the glyph with |data|. Returns false if + * data is invalid. */ bool initializePath(SkGlyph*, const volatile void* data, size_t size); diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index 0ccbf2f15e..fa4767069f 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -668,6 +668,8 @@ void SkStrikeServer::SkGlyphCacheState::writePendingGlyphs(Serializer* serialize SkGlyph stationaryGlyph = *glyph; stationaryGlyph.fImage = serializer->allocate(imageSize, stationaryGlyph.formatAlignment()); fContext->getImage(stationaryGlyph); + // TODO: Generating the image can change the mask format, do we need to update it in the + // serialized glyph? } fPendingGlyphImages.clear(); @@ -811,10 +813,14 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi if (!deserializer.read<SkGlyph>(&glyph)) READ_FAILURE SkGlyph* allocatedGlyph = strike->getRawGlyphByID(glyph.getPackedID()); - // Don't override the path, if the glyph has one. - auto* glyphPath = allocatedGlyph->fPathData; - *allocatedGlyph = glyph; - allocatedGlyph->fPathData = glyphPath; + + // Update the glyph unless it's already got an image (from fallback), + // preserving any path that might be present. + if (allocatedGlyph->fImage == nullptr) { + auto* glyphPath = allocatedGlyph->fPathData; + *allocatedGlyph = glyph; + allocatedGlyph->fPathData = glyphPath; + } bool tooLargeForAtlas = false; #if SK_SUPPORT_GPU @@ -828,9 +834,8 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi if (imageSize == 0u) continue; auto* image = deserializer.read(imageSize, allocatedGlyph->formatAlignment()); - if (!image || - !strike->initializeImage(image, imageSize, allocatedGlyph)) - READ_FAILURE + if (!image) READ_FAILURE + strike->initializeImage(image, imageSize, allocatedGlyph); } size_t glyphPathsCount = 0u; @@ -840,10 +845,14 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi if (!deserializer.read<SkGlyph>(&glyph)) READ_FAILURE SkGlyph* allocatedGlyph = strike->getRawGlyphByID(glyph.getPackedID()); - // Don't override the image, if the glyph has one. - auto* glyphImage = allocatedGlyph->fImage; - *allocatedGlyph = glyph; - allocatedGlyph->fImage = glyphImage; + + // Update the glyph unless it's already got a path (from fallback), + // preserving any image that might be present. + if (allocatedGlyph->fPathData == nullptr) { + auto* glyphImage = allocatedGlyph->fImage; + *allocatedGlyph = glyph; + allocatedGlyph->fImage = glyphImage; + } if (!read_path(&deserializer, allocatedGlyph, strike.get())) READ_FAILURE } diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp index 62036d2285..8bdfb7af69 100644 --- a/tests/SkRemoteGlyphCacheTest.cpp +++ b/tests/SkRemoteGlyphCacheTest.cpp @@ -509,3 +509,104 @@ DEF_TEST(SkRemoteGlyphCache_SearchOfDesperation, reporter) { // 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> discardableManager = sk_make_sp<DiscardableManager>(); + 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); + + SkPaint paint; + paint.setAntiAlias(true); + paint.setColor(SK_ColorRED); + + auto lostGlyphID = SkPackedGlyphID(1, SK_FixedHalf, SK_FixedHalf); + const uint8_t glyphImage[] = {0xFF, 0xFF}; + uint32_t realMask; + uint32_t fakeMask; + + { + SkAutoDescriptor ad; + SkScalerContextRec rec; + SkScalerContextEffects effects; + SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; + paint.setTypeface(serverTf); + SkScalerContext::MakeRecAndEffects(paint, nullptr, nullptr, flags, &rec, &effects, false); + auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); + + auto context = serverTf->createScalerContext(effects, desc, false); + SkGlyph glyph; + glyph.initWithGlyphID(lostGlyphID); + context->getMetrics(&glyph); + realMask = glyph.fMaskFormat; + REPORTER_ASSERT(reporter, realMask != MASK_FORMAT_UNKNOWN); + } + + // Build a fallback cache. + { + SkAutoDescriptor ad; + SkScalerContextRec rec; + SkScalerContextEffects effects; + SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; + paint.setTypeface(clientTf); + SkScalerContext::MakeRecAndEffects(paint, nullptr, nullptr, flags, &rec, &effects, false); + auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); + + auto fallbackCache = SkStrikeCache::FindOrCreateStrikeExclusive(*desc, effects, *clientTf); + auto glyph = fallbackCache->getRawGlyphByID(lostGlyphID); + fakeMask = (realMask == SkMask::kA8_Format) ? SkMask::kBW_Format : SkMask::kA8_Format; + glyph->fMaskFormat = fakeMask; + glyph->fHeight = 1; + glyph->fWidth = 2; + fallbackCache->initializeImage(glyphImage, glyph->computeImageSize(), glyph); + } + + // Send over the real glyph and make sure the client cache stays intact. + { + SkAutoDescriptor ad; + SkScalerContextRec rec; + SkScalerContextEffects effects; + SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; + paint.setTypeface(serverTf); + auto* cacheState = server.getOrCreateCache(paint, nullptr, nullptr, flags, &rec, &effects); + cacheState->addGlyph(serverTf.get(), effects, lostGlyphID, false); + + std::vector<uint8_t> serverStrikeData; + server.writeStrikeData(&serverStrikeData); + REPORTER_ASSERT(reporter, + client.readStrikeData(serverStrikeData.data(), serverStrikeData.size())); + } + + { + SkAutoDescriptor ad; + SkScalerContextRec rec; + SkScalerContextEffects effects; + SkScalerContextFlags flags = SkScalerContextFlags::kFakeGammaAndBoostContrast; + paint.setTypeface(clientTf); + SkScalerContext::MakeRecAndEffects(paint, nullptr, nullptr, flags, &rec, &effects, false); + auto desc = SkScalerContext::AutoDescriptorGivenRecAndEffects(rec, effects, &ad); + + auto fallbackCache = SkStrikeCache::FindStrikeExclusive(*desc); + REPORTER_ASSERT(reporter, fallbackCache.get() != nullptr); + auto glyph = fallbackCache->getRawGlyphByID(lostGlyphID); + REPORTER_ASSERT(reporter, glyph->fMaskFormat == fakeMask); + + // Try overriding the image, it should stay the same. + REPORTER_ASSERT(reporter, + memcmp(glyph->fImage, glyphImage, glyph->computeImageSize()) == 0); + const uint8_t newGlyphImage[] = {0, 0}; + fallbackCache->initializeImage(newGlyphImage, glyph->computeImageSize(), glyph); + REPORTER_ASSERT(reporter, + memcmp(glyph->fImage, glyphImage, glyph->computeImageSize()) == 0); + } + + SkStrikeCache::Validate(); + + // Must unlock everything on termination, otherwise valgrind complains about memory leaks. + discardableManager->unlockAndDeleteAll(); +} |