[core] use stale glyphs

Updating glyphs is still unsupported, and there's no good use case for doing so. When we're using a stale glyph PBF, and the fresh answer contains changed to that glyph, we will continue to use the old glyph.

9 files changed, 104 insertions, 36 deletions

diff --git a/include/mbgl/platform/event.hpp b/include/mbgl/platform/event.hpp
index 5c59dc69c7..8e36508388 100644
--- a/include/mbgl/platform/event.hpp
+++ b/include/mbgl/platform/event.hpp
@@ -37,7 +37,8 @@ enum class Event : uint8_t {
     OpenGL,
     JNI,
     Android,
-    Crash
+    Crash,
+    Glyph,
 };
 
 MBGL_DEFINE_ENUM_CLASS(EventClass, Event, {
@@ -56,6 +57,7 @@ MBGL_DEFINE_ENUM_CLASS(EventClass, Event, {
     { Event::JNI, "JNI" },
     { Event::Android, "Android" },
     { Event::Crash, "Crash" },
+    { Event::Glyph, "Glyph" },
     { Event(-1), "Unknown" },
 });
 
diff --git a/src/mbgl/text/font_stack.cpp b/src/mbgl/text/font_stack.cpp
index d31ff7bd59..60e90a77e8 100644
--- a/src/mbgl/text/font_stack.cpp
+++ b/src/mbgl/text/font_stack.cpp
@@ -1,17 +1,28 @@
 #include <mbgl/text/font_stack.hpp>
 #include <cassert>
+#include <mbgl/platform/log.hpp>
 #include <mbgl/util/math.hpp>
 
 namespace mbgl {
 
 void FontStack::insert(uint32_t id, const SDFGlyph &glyph) {
-    metrics.emplace(id, glyph.metrics);
-    bitmaps.emplace(id, glyph.bitmap);
-    sdfs.emplace(id, glyph);
-}
-
-const std::map<uint32_t, GlyphMetrics> &FontStack::getMetrics() const {
-    return metrics;
+    auto it = sdfs.find(id);
+    if (it == sdfs.end()) {
+        // Glyph doesn't exist yet.
+        sdfs.emplace(id, glyph);
+    } else if (it->second.metrics == glyph.metrics) {
+        if (it->second.bitmap != glyph.bitmap) {
+            // The actual bitmap was updated; this is unsupported.
+            Log::Warning(Event::Glyph, "Modified glyph changed bitmap represenation");
+        }
+        // At least try to update it in case it's currently unsused.
+        // If it is already used; we won't attempt to update the glyph atlas texture.
+        it->second.bitmap = glyph.bitmap;
+    } else {
+        // The metrics were updated; this is unsupported.
+        Log::Warning(Event::Glyph, "Modified glyph has different metrics");
+        return;
+    }
 }
 
 const std::map<uint32_t, SDFGlyph> &FontStack::getSDFs() const {
@@ -32,10 +43,10 @@ const Shaping FontStack::getShaping(const std::u32string &string, const float ma
 
     // Loop through all characters of this label and shape.
     for (uint32_t chr : string) {
-        auto metric = metrics.find(chr);
-        if (metric != metrics.end()) {
+        auto it = sdfs.find(chr);
+        if (it != sdfs.end()) {
             shaping.positionedGlyphs.emplace_back(chr, x, y);
-            x += metric->second.advance + spacing;
+            x += it->second.metrics.advance + spacing;
         }
     }
 
@@ -59,12 +70,12 @@ void align(Shaping &shaping, const float justify, const float horizontalAlign,
     }
 }
 
-void justifyLine(std::vector<PositionedGlyph> &positionedGlyphs, const std::map<uint32_t, GlyphMetrics> &metrics, uint32_t start,
+void justifyLine(std::vector<PositionedGlyph> &positionedGlyphs, const std::map<uint32_t, SDFGlyph> &sdfs, uint32_t start,
                  uint32_t end, float justify) {
     PositionedGlyph &glyph = positionedGlyphs[end];
-    auto metric = metrics.find(glyph.glyph);
-    if (metric != metrics.end()) {
-        const uint32_t lastAdvance = metric->second.advance;
+    auto it = sdfs.find(glyph.glyph);
+    if (it != sdfs.end()) {
+        const uint32_t lastAdvance = it->second.metrics.advance;
         const float lineIndent = float(glyph.x + lastAdvance) * justify;
 
         for (uint32_t j = start; j <= end; j++) {
@@ -112,7 +123,7 @@ void FontStack::lineWrap(Shaping &shaping, const float lineHeight, const float m
                         lineEnd--;
                     }
 
-                    justifyLine(positionedGlyphs, metrics, lineStartIndex, lineEnd, justify);
+                    justifyLine(positionedGlyphs, sdfs, lineStartIndex, lineEnd, justify);
                 }
 
                 lineStartIndex = lastSafeBreak + 1;
@@ -138,14 +149,14 @@ void FontStack::lineWrap(Shaping &shaping, const float lineHeight, const float m
     }
 
     const PositionedGlyph& lastPositionedGlyph = positionedGlyphs.back();
-    const auto lastGlyphMetric = metrics.find(lastPositionedGlyph.glyph);
-    assert(lastGlyphMetric != metrics.end());
-    const uint32_t lastLineLength = lastPositionedGlyph.x + lastGlyphMetric->second.advance;
+    const auto lastGlyphIt = sdfs.find(lastPositionedGlyph.glyph);
+    assert(lastGlyphIt != sdfs.end());
+    const uint32_t lastLineLength = lastPositionedGlyph.x + lastGlyphIt->second.metrics.advance;
     maxLineLength = std::max(maxLineLength, lastLineLength);
 
     const uint32_t height = (line + 1) * lineHeight;
 
-    justifyLine(positionedGlyphs, metrics, lineStartIndex, uint32_t(positionedGlyphs.size()) - 1, justify);
+    justifyLine(positionedGlyphs, sdfs, lineStartIndex, uint32_t(positionedGlyphs.size()) - 1, justify);
     align(shaping, justify, horizontalAlign, verticalAlign, maxLineLength, lineHeight, line, translate);
 
     // Calculate the bounding box
diff --git a/src/mbgl/text/font_stack.hpp b/src/mbgl/text/font_stack.hpp
index 4ca967d7a8..d81460a534 100644
--- a/src/mbgl/text/font_stack.hpp
+++ b/src/mbgl/text/font_stack.hpp
@@ -9,7 +9,6 @@ namespace mbgl {
 class FontStack {
 public:
     void insert(uint32_t id, const SDFGlyph &glyph);
-    const std::map<uint32_t, GlyphMetrics> &getMetrics() const;
     const std::map<uint32_t, SDFGlyph> &getSDFs() const;
     const Shaping getShaping(const std::u32string &string, float maxWidth, float lineHeight,
                              float horizontalAlign, float verticalAlign, float justify,
@@ -18,8 +17,6 @@ public:
                   float verticalAlign, float justify, const vec2<float> &translate) const;
 
 private:
-    std::map<uint32_t, std::string> bitmaps;
-    std::map<uint32_t, GlyphMetrics> metrics;
     std::map<uint32_t, SDFGlyph> sdfs;
 };
 
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 0c0626c0de..f6f0dca021 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -75,15 +75,9 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
     });
 
     auto requestCallback = [this, store, fontStack, glyphRange](Response res) {
-        if (res.stale) {
-            // Only handle fresh responses.
-            return;
-        }
-        req = nullptr;
-
         if (res.error) {
             observer->onGlyphsError(fontStack, glyphRange, std::make_exception_ptr(std::runtime_error(res.error->message)));
-        } else {
+        } else if (data != res.data || (*data != *res.data)) {
             data = res.data;
             parse(store, fontStack, glyphRange);
         }
diff --git a/test/fixtures/stale/glyph.pbf b/test/fixtures/stale/glyph.pbf
new file mode 100644
index 0000000000..0d160f7898
--- /dev/null
+++ b/test/fixtures/stale/glyph.pbf
diff --git a/test/fixtures/stale/stale_style_and_glyphs/expected.png b/test/fixtures/stale/stale_style_and_glyphs/expected.png
new file mode 100644
index 0000000000..d8dbda1092
--- /dev/null
+++ b/test/fixtures/stale/stale_style_and_glyphs/expected.png
diff --git a/test/fixtures/stale/style_and_glyphs.json b/test/fixtures/stale/style_and_glyphs.json
new file mode 100644
index 0000000000..adf91705fb
--- /dev/null
+++ b/test/fixtures/stale/style_and_glyphs.json
@@ -0,0 +1,39 @@
+{
+  "version": 8,
+  "name": "Water",
+  "sources": {
+    "mapbox": {
+      "type": "vector",
+      "url": "asset://test/fixtures/stale/streets.json"
+    }
+  },
+  "glyphs": "http://127.0.0.1:3000/stale/{fontstack}/{range}.pbf",
+  "layers": [{
+    "id": "background",
+    "type": "background",
+    "paint": {
+      "background-color": "red"
+    }
+  }, {
+    "id": "water",
+    "type": "fill",
+    "source": "mapbox",
+    "source-layer": "water",
+    "paint": {
+      "fill-color": "blue"
+    }
+  }, {
+    "id": "admin",
+    "type": "symbol",
+    "source": "mapbox",
+    "source-layer": "admin",
+    "layout": {
+        "text-field": "Text"
+    },
+    "paint": {
+      "text-color": "black",
+      "text-halo-color": "white",
+      "text-halo-width": 2
+    }
+  }]
+}
diff --git a/test/storage/cache_stale.cpp b/test/storage/cache_stale.cpp
index ab9fe7b202..96fd9bbbcd 100644
--- a/test/storage/cache_stale.cpp
+++ b/test/storage/cache_stale.cpp
@@ -15,8 +15,13 @@ using namespace std::literals::string_literals;
 
 namespace {
 
-void checkRendering(Map& map, const char *name, std::chrono::milliseconds timeout) {
-    test::checkImage("test/fixtures/stale/"s + name, test::render(map, timeout), 0.001, 0.1);
+void checkRendering(Map& map,
+                    const char* name,
+                    std::chrono::milliseconds timeout,
+                    double imageThreshold = 0.001,
+                    double pixelThreshold = 0.1) {
+    test::checkImage("test/fixtures/stale/"s + name, test::render(map, timeout), imageThreshold,
+                     pixelThreshold);
 }
 
 Response expiredItem(const std::string& path) {
@@ -100,3 +105,27 @@ TEST_F(Storage, CacheStaleStyleAndSprite) {
 
     checkRendering(map, "stale_style_and_sprite", 1000ms);
 }
+
+TEST_F(Storage, CacheStaleStyleAndGlyphs) {
+    HeadlessView view(display, 1);
+
+    auto cache = SQLiteCache::getShared(":memory:");
+
+    // Rig the cache with an expired stylesheet.
+    const std::string stylePath = "stale/style_and_glyphs.json";
+    const Resource styleResource{ Resource::Kind::Style, prefix + "/" + stylePath };
+    cache->put(styleResource, expiredItem(stylePath));
+
+    // Rig the cache with an expired glyph PBF.
+    const std::string glyphPath = "stale/glyph.pbf";
+    const Resource glyphResource{ Resource::Kind::Glyphs, prefix + "/stale/Open%20Sans%20Regular%2c%20Arial%20Unicode%20MS%20Regular/0-255.pbf" };
+    cache->put(glyphResource, expiredItem(glyphPath));
+
+    DefaultFileSource fileSource(":memory:", ".");
+
+    Map map(view, fileSource, MapMode::Still);
+    map.setStyleURL(styleResource.url);
+
+    // TODO: this shouldn't take > 1 second
+    checkRendering(map, "stale_style_and_glyphs", 2000ms, 0.0015);
+}
diff --git a/test/style/glyph_store.cpp b/test/style/glyph_store.cpp
index a53b20c6d2..0215dff3ff 100644
--- a/test/style/glyph_store.cpp
+++ b/test/style/glyph_store.cpp
@@ -52,7 +52,6 @@ TEST(GlyphStore, LoadingSuccess) {
             return;
 
         auto fontStack = test.glyphStore.getFontStack("Test Stack");
-        ASSERT_FALSE(fontStack->getMetrics().empty());
         ASSERT_FALSE(fontStack->getSDFs().empty());
 
         test.end();
@@ -73,7 +72,6 @@ TEST(GlyphStore, LoadingFail) {
         ASSERT_EQ(glyphRange, GlyphRange(0, 255));
 
         auto stack = test.glyphStore.getFontStack("Test Stack");
-        ASSERT_TRUE(stack->getMetrics().empty());
         ASSERT_TRUE(stack->getSDFs().empty());
         ASSERT_FALSE(test.glyphStore.hasGlyphRanges("Test Stack", {{0, 255}}));
 
@@ -95,7 +93,6 @@ TEST(GlyphStore, LoadingCorrupted) {
         ASSERT_EQ(glyphRange, GlyphRange(0, 255));
 
         auto stack = test.glyphStore.getFontStack("Test Stack");
-        ASSERT_TRUE(stack->getMetrics().empty());
         ASSERT_TRUE(stack->getSDFs().empty());
         ASSERT_FALSE(test.glyphStore.hasGlyphRanges("Test Stack", {{0, 255}}));
 
@@ -132,7 +129,6 @@ TEST(GlyphStore, InvalidURL) {
         ASSERT_TRUE(error != nullptr);
 
         auto stack = test.glyphStore.getFontStack("Test Stack");
-        ASSERT_TRUE(stack->getMetrics().empty());
         ASSERT_TRUE(stack->getSDFs().empty());
 
         test.end();