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>
diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h
index 06c0e42..3ea795b 100644
--- a/src/core/SkGlyph.h
+++ b/src/core/SkGlyph.h
@@ -85,6 +85,12 @@
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 @@
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 6b8bd70..e5c871f 100644
--- a/src/core/SkGlyphCache.cpp
+++ b/src/core/SkGlyphCache.cpp
@@ -201,8 +201,10 @@
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 @@
fMemoryUsed += size;
}
}
-
- return true;
}
const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) {
@@ -237,7 +237,9 @@
}
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 47a1d3d..612ddb4 100644
--- a/src/core/SkGlyphCache.h
+++ b/src/core/SkGlyphCache.h
@@ -91,10 +91,9 @@
*/
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 @@
*/
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 0ccbf2f..fa47670 100644
--- a/src/core/SkRemoteGlyphCache.cpp
+++ b/src/core/SkRemoteGlyphCache.cpp
@@ -668,6 +668,8 @@
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 @@
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 @@
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 @@
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 62036d2..8bdfb7a 100644
--- a/tests/SkRemoteGlyphCacheTest.cpp
+++ b/tests/SkRemoteGlyphCacheTest.cpp
@@ -509,3 +509,104 @@
// 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();
+}