Do not process buckets depending on Glyphs that were not loaded

For now, the buckets depending on Glyphs that are not yet available
are simply not created (or created and discarded). In the future
we could refactor the code to not create the bucket at all or put
the bucket in a suspended state until the glyph range is available.

src/mbgl/map/tile_parser.cpp

@@ -119,11 +119,13 @@ std::unique_ptr<Bucket> TileParser::createBucket(const StyleBucket &bucketDesc)
         } else if (bucketDesc.type == StyleLayerType::Line) {
             return createLineBucket(*layer, bucketDesc);
         } else if (bucketDesc.type == StyleLayerType::Symbol) {
-            if (sprite->isLoaded()) {
-                return createSymbolBucket(*layer, bucketDesc);
-            } else {
+            bool needsResources = false;
+            auto symbolBucket = createSymbolBucket(*layer, bucketDesc, needsResources);
+            if (needsResources) {
                 partialParse = true;
                 return nullptr;
+            } else {
+                return std::move(symbolBucket);
             }
         } else if (bucketDesc.type == StyleLayerType::Raster) {
             return nullptr;
@@ -185,7 +187,13 @@ std::unique_ptr<Bucket> TileParser::createLineBucket(const GeometryTileLayer& la
 }
 
 std::unique_ptr<Bucket> TileParser::createSymbolBucket(const GeometryTileLayer& layer,
-                                                       const StyleBucket& bucket_desc) {
+                                                       const StyleBucket& bucket_desc,
+                                                       bool& needsResources) {
+    if (!sprite->isLoaded()) {
+        needsResources = true;
+        return nullptr;
+    }
+
     auto bucket = util::make_unique<SymbolBucket>(*collision);
 
     const float z = tile.id.z;
@@ -231,6 +239,13 @@ std::unique_ptr<Bucket> TileParser::createSymbolBucket(const GeometryTileLayer&
 
     bucket->addFeatures(
         layer, bucket_desc.filter, reinterpret_cast<uintptr_t>(&tile), spriteAtlas, *sprite, glyphAtlas, glyphStore);
+
+    if (bucket->needsGlyphs()) {
+        needsResources = true;
+        return nullptr;
+    }
+
     return bucket->hasData() ? std::move(bucket) : nullptr;
 }
+
 }

src/mbgl/map/tile_parser.hpp

@@ -54,7 +54,7 @@ private:
     std::unique_ptr<Bucket> createBucket(const StyleBucket&);
     std::unique_ptr<Bucket> createFillBucket(const GeometryTileLayer&, const StyleBucket&);
     std::unique_ptr<Bucket> createLineBucket(const GeometryTileLayer&, const StyleBucket&);
-    std::unique_ptr<Bucket> createSymbolBucket(const GeometryTileLayer&, const StyleBucket&);
+    std::unique_ptr<Bucket> createSymbolBucket(const GeometryTileLayer&, const StyleBucket&, bool& needsResources);
 
     template <class Bucket>
     void addBucketGeometries(Bucket&, const GeometryTileLayer&, const FilterExpression&);

src/mbgl/renderer/symbol_bucket.cpp

@@ -29,7 +29,7 @@
 namespace mbgl {
 
 SymbolBucket::SymbolBucket(Collision &collision_)
-    : collision(collision_) {
+    : collision(collision_), needsGlyphs_(false) {
 }
 
 SymbolBucket::~SymbolBucket() {
@@ -134,7 +134,10 @@ std::vector<SymbolFeature> SymbolBucket::processFeatures(const GeometryTileLayer
         util::mergeLines(features);
     }
 
-    glyphStore.waitForGlyphRanges(layout.text.font, ranges);
+    if (glyphStore.requestGlyphRangesIfNeeded(layout.text.font, ranges)) {
+        needsGlyphs_ = true;
+        return {};
+    }
 
     return features;
 }
@@ -148,6 +151,12 @@ void SymbolBucket::addFeatures(const GeometryTileLayer& layer,
                                GlyphStore& glyphStore) {
     const std::vector<SymbolFeature> features = processFeatures(layer, filter, glyphStore);
 
+    // Stop if we still need glyphs because the
+    // bucket will be discarded.
+    if (needsGlyphs()) {
+        return;
+    }
+
     float horizontalAlign = 0.5;
     float verticalAlign = 0.5;
 
@@ -189,7 +198,7 @@ void SymbolBucket::addFeatures(const GeometryTileLayer& layer,
     if (layout.text.justify == TextJustifyType::Right) justify = 1;
     else if (layout.text.justify == TextJustifyType::Left) justify = 0;
 
-    const auto &fontStack = glyphStore.getFontStack(layout.text.font);
+    auto* fontStack = glyphStore.getFontStack(layout.text.font);
 
     for (const auto& feature : features) {
         if (!feature.geometry.size()) continue;
@@ -213,7 +222,7 @@ void SymbolBucket::addFeatures(const GeometryTileLayer& layer,
 
             // Add the glyphs we need for this label to the glyph atlas.
             if (shaping.size()) {
-                glyphAtlas.addGlyphs(tileUID, feature.label, layout.text.font, fontStack, face);
+                glyphAtlas.addGlyphs(tileUID, feature.label, layout.text.font, *fontStack, face);
             }
         }
 

src/mbgl/renderer/symbol_bucket.hpp

@@ -69,6 +69,8 @@ public:
                      GlyphAtlas&,
                      GlyphStore&);
 
+    inline bool needsGlyphs() const { return needsGlyphs_; }
+
     void drawGlyphs(SDFShader& shader);
     void drawIcons(SDFShader& shader);
     void drawIcons(IconShader& shader);
@@ -103,6 +105,7 @@ private:
         std::vector<std::unique_ptr<IconElementGroup>> groups;
     } icon;
 
+    bool needsGlyphs_;
 };
 }
 

src/mbgl/text/glyph_store.cpp

@@ -10,6 +10,7 @@
 #include <mbgl/util/token.hpp>
 #include <mbgl/util/math.hpp>
 #include <mbgl/storage/file_source.hpp>
+#include <mbgl/platform/log.hpp>
 #include <mbgl/platform/platform.hpp>
 #include <mbgl/util/uv_detail.hpp>
 #include <algorithm>
@@ -138,11 +139,12 @@ void FontStack::lineWrap(Shaping &shaping, const float lineHeight, const float m
     align(shaping, justify, horizontalAlign, verticalAlign, maxLineLength, lineHeight, line);
 }
 
-GlyphPBF::GlyphPBF(const std::string &glyphURL,
-                   const std::string &fontStack,
+GlyphPBF::GlyphPBF(const std::string& glyphURL,
+                   const std::string& fontStack,
                    GlyphRange glyphRange,
-                   Environment &env)
-    : future(promise.get_future().share()) {
+                   Environment& env,
+                   const GlyphLoadedCallback& callback)
+    : parsed(false) {
     // Load the glyph set URL
     std::string url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string {
         if (name == "fontstack") return util::percentEncode(fontStack);
@@ -151,27 +153,23 @@ GlyphPBF::GlyphPBF(const std::string &glyphURL,
     });
 
     // The prepare call jumps back to the main thread.
-    env.requestAsync({ Resource::Kind::Glyphs, url }, [&, url](const Response &res) {
+    env.requestAsync({ Resource::Kind::Glyphs, url }, [&, url, callback](const Response &res) {
         if (res.status != Response::Successful) {
-            // Something went wrong with loading the glyph pbf. Pass on the error to the future listeners.
+            // Something went wrong with loading the glyph pbf.
             const std::string msg = std::string { "[ERROR] failed to load glyphs: " } + url + " message: " + res.message;
-            promise.set_exception(std::make_exception_ptr(std::runtime_error(msg)));
+            Log::Error(Event::HttpRequest, msg);
         } else {
             // Transfer the data to the GlyphSet and signal its availability.
             // Once it is available, the caller will need to call parse() to actually
             // parse the data we received. We are not doing this here since this callback is being
             // called from another (unknown) thread.
             data = res.data;
-            promise.set_value(*this);
+            parsed = true;
+            callback(this);
         }
     });
 }
 
-
-std::shared_future<GlyphPBF &> GlyphPBF::getFuture() {
-    return future;
-}
-
 void GlyphPBF::parse(FontStack &stack) {
     std::lock_guard<std::mutex> lock(mtx);
 
@@ -226,8 +224,12 @@ void GlyphPBF::parse(FontStack &stack) {
     data.clear();
 }
 
+bool GlyphPBF::isParsed() const {
+    return parsed;
+}
+
 GlyphStore::GlyphStore(Environment& env_)
-    : env(env_), mtx(util::make_unique<uv::mutex>()), observer(nullptr) {
+    : env(env_), observer(nullptr) {
 }
 
 GlyphStore::~GlyphStore() {
@@ -238,61 +240,59 @@ void GlyphStore::setURL(const std::string &url) {
     glyphURL = url;
 }
 
+bool GlyphStore::requestGlyphRangesIfNeeded(const std::string& fontStack,
+                                            const std::set<GlyphRange>& glyphRanges) {
+    bool requestIsNeeded = false;
 
-void GlyphStore::waitForGlyphRanges(const std::string &fontStack, const std::set<GlyphRange> &glyphRanges) {
-    // We are implementing a blocking wait with futures: Every GlyphSet has a future that we are
-    // waiting for until it is loaded.
     if (glyphRanges.empty()) {
-        return;
+        return requestIsNeeded;
     }
 
-    uv::exclusive<FontStack> stack(mtx);
+    auto callback = [this, fontStack](GlyphPBF* glyph) {
+        glyph->parse(*createFontStack(fontStack));
+        emitGlyphRangeLoaded();
+    };
 
-    std::vector<std::shared_future<GlyphPBF &>> futures;
-    futures.reserve(glyphRanges.size());
-    {
-        auto &rangeSets = ranges[fontStack];
+    std::lock_guard<std::mutex> lock(rangesMutex);
+    auto& rangeSets = ranges[fontStack];
 
-        stack << createFontStack(fontStack);
+    for (const auto& range : glyphRanges) {
+        const auto& rangeSets_it = rangeSets.find(range);
+        if (rangeSets_it == rangeSets.end()) {
+            auto glyph = util::make_unique<GlyphPBF>(glyphURL, fontStack, range, env, callback);
+            rangeSets.emplace(range, std::move(glyph));
+            requestIsNeeded = true;
+            continue;
+        }
 
-        // Attempt to load the glyph range. If the GlyphSet already exists, we are getting back
-        // the same shared_future.
-        for (const auto range : glyphRanges) {
-            futures.emplace_back(loadGlyphRange(fontStack, rangeSets, range));
+        if (!rangeSets_it->second->isParsed()) {
+            requestIsNeeded = true;
         }
     }
 
-    // Now that we potentially created all GlyphSets, we are waiting for the results, one by one.
-    // When we get a result (or the GlyphSet is aready loaded), we are attempting to parse the
-    // GlyphSet.
-    for (const auto& future : futures) {
-        future.get().parse(stack);
-    }
+    return requestIsNeeded;
 }
 
-std::shared_future<GlyphPBF &> GlyphStore::loadGlyphRange(const std::string &fontStack, std::map<GlyphRange, std::unique_ptr<GlyphPBF>> &rangeSets, const GlyphRange range) {
-    auto range_it = rangeSets.find(range);
-    if (range_it == rangeSets.end()) {
-        // We don't have this glyph set yet for this font stack.
-        range_it = rangeSets.emplace(range, util::make_unique<GlyphPBF>(glyphURL, fontStack, range, env)).first;
-    }
+FontStack* GlyphStore::createFontStack(const std::string &fontStack) {
+    std::lock_guard<std::mutex> lock(stacksMutex);
 
-    return range_it->second->getFuture();
-}
-
-FontStack &GlyphStore::createFontStack(const std::string &fontStack) {
     auto stack_it = stacks.find(fontStack);
     if (stack_it == stacks.end()) {
         stack_it = stacks.emplace(fontStack, util::make_unique<FontStack>()).first;
     }
 
-    return *stack_it->second.get();
+    return stack_it->second.get();
 }
 
-uv::exclusive<FontStack> GlyphStore::getFontStack(const std::string &fontStack) {
-    uv::exclusive<FontStack> stack(mtx);
-    stack << createFontStack(fontStack);
-    return stack;
+FontStack* GlyphStore::getFontStack(const std::string &fontStack) {
+    std::lock_guard<std::mutex> lock(stacksMutex);
+
+    const auto& stack_it = stacks.find(fontStack);
+    if (stack_it == stacks.end()) {
+        return nullptr;
+    }
+
+    return stack_it->second.get();
 }
 
 void GlyphStore::setObserver(Observer* observer_) {

src/mbgl/text/glyph_store.hpp

@@ -4,13 +4,13 @@
 #include <mbgl/text/glyph.hpp>
 #include <mbgl/util/vec.hpp>
 #include <mbgl/util/ptr.hpp>
-#include <mbgl/util/uv.hpp>
 
 #include <cstdint>
 #include <vector>
 #include <future>
 #include <map>
 #include <set>
+#include <string>
 #include <unordered_map>
 
 namespace mbgl {
@@ -49,10 +49,16 @@ private:
 
 class GlyphPBF {
 public:
+    using GlyphLoadedCallback = std::function<void(GlyphPBF*)>;
+
     GlyphPBF(const std::string &glyphURL,
              const std::string &fontStack,
              GlyphRange glyphRange,
-             Environment &env);
+             Environment &env,
+             const GlyphLoadedCallback& callback);
+
+    void parse(FontStack &stack);
+    bool isParsed() const;
 
 private:
     GlyphPBF(const GlyphPBF &) = delete;
@@ -60,16 +66,9 @@ private:
     GlyphPBF &operator=(const GlyphPBF &) = delete;
     GlyphPBF &operator=(GlyphPBF &&) = delete;
 
-public:
-    void parse(FontStack &stack);
-
-    std::shared_future<GlyphPBF &> getFuture();
-
-private:
     std::string data;
-    std::promise<GlyphPBF &> promise;
-    std::shared_future<GlyphPBF &> future;
-    std::mutex mtx;
+    std::atomic_bool parsed;
+    mutable std::mutex mtx;
 };
 
 // Manages Glyphrange PBF loading.
@@ -85,10 +84,13 @@ public:
     GlyphStore(Environment &);
     ~GlyphStore();
 
-    // Block until all specified GlyphRanges of the specified font stack are loaded.
-    void waitForGlyphRanges(const std::string &fontStack, const std::set<GlyphRange> &glyphRanges);
+    // Asynchronously request for GlyphRanges and when it gets loaded, notifies the
+    // observer subscribed to this object. Successive requests for the same range are
+    // going to be discarded. Returns true if a request was made or false if all the
+    // GlyphRanges are already available, and thus, no request is performed.
+    bool requestGlyphRangesIfNeeded(const std::string &fontStack, const std::set<GlyphRange> &glyphRanges);
 
-    uv::exclusive<FontStack> getFontStack(const std::string &fontStack);
+    FontStack* getFontStack(const std::string &fontStack);
 
     void setURL(const std::string &url);
 
@@ -97,21 +99,20 @@ public:
 private:
     void emitGlyphRangeLoaded();
 
-    // Loads an individual glyph range from the font stack and adds it to rangeSets
-    std::shared_future<GlyphPBF &> loadGlyphRange(const std::string &fontStack, std::map<GlyphRange, std::unique_ptr<GlyphPBF>> &rangeSets, GlyphRange range);
-
-    FontStack &createFontStack(const std::string &fontStack);
+    FontStack* createFontStack(const std::string &fontStack);
 
     std::string glyphURL;
     Environment &env;
+
     std::unordered_map<std::string, std::map<GlyphRange, std::unique_ptr<GlyphPBF>>> ranges;
+    std::mutex rangesMutex;
+
     std::unordered_map<std::string, std::unique_ptr<FontStack>> stacks;
-    std::unique_ptr<uv::mutex> mtx;
+    std::mutex stacksMutex;
 
     Observer* observer;
 };
 
-
 }
 
 #endif