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>

4 files changed, 39 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
         }