[core] Inline GlyphPBF into GlyphAtlas

6 files changed, 111 insertions, 173 deletions

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 6aa4e11b1c..10bdeeea4a 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -4,7 +4,6 @@
 #include <mbgl/util/font_stack.hpp>
 #include <mbgl/util/rect.hpp>
 #include <mbgl/util/traits.hpp>
-#include <mbgl/util/image.hpp>
 
 #include <cstdint>
 #include <vector>
@@ -84,21 +83,6 @@ class Shaping {
     explicit operator bool() const { return !positionedGlyphs.empty(); }
 };
 
-class SDFGlyph {
-public:
-    // We're using this value throughout the Mapbox GL ecosystem. If this is different, the glyphs
-    // also need to be reencoded.
-    static constexpr const uint8_t borderSize = 3;
-
-    GlyphID id = 0;
-
-    // A signed distance field of the glyph with a border (see above).
-    AlphaImage bitmap;
-
-    // Glyph metrics
-    GlyphMetrics metrics;
-};
-
 enum class WritingModeType : uint8_t {
     None = 0,
     Horizontal = 1 << 0,
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index ade2734ca9..3e8bb0fe9c 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -4,6 +4,9 @@
 #include <mbgl/gl/context.hpp>
 #include <mbgl/util/logging.hpp>
 #include <mbgl/util/platform.hpp>
+#include <mbgl/storage/file_source.hpp>
+#include <mbgl/storage/resource.hpp>
+#include <mbgl/storage/response.hpp>
 
 #include <cassert>
 #include <algorithm>
@@ -34,21 +37,15 @@ bool GlyphAtlas::hasGlyphRanges(const FontStack& fontStack, const GlyphRangeSet&
     }
     return true;
 }
-    
-bool rangeIsParsed(const std::map<GlyphRange, GlyphPBF>& ranges, const GlyphRange& range) {
-    auto rangeIt = ranges.find(range);
-    if (rangeIt == ranges.end())
-        return false;
-    
-    return rangeIt->second.isParsed();
+
+bool GlyphAtlas::rangeIsParsed(const std::map<GlyphRange, GlyphRequest>& ranges, const GlyphRange& range) const {
+    auto it = ranges.find(range);
+    return it != ranges.end() && it->second.parsed;
 }
-    
+
 bool GlyphAtlas::hasGlyphRange(const FontStack& fontStack, const GlyphRange& range) const {
-    auto entry = entries.find(fontStack);
-    if (entry == entries.end())
-        return false;
-    
-    return rangeIsParsed(entry->second.ranges, range);
+    auto it = entries.find(fontStack);
+    return it != entries.end() && rangeIsParsed(it->second.ranges, range);
 }
     
 void GlyphAtlas::getGlyphs(GlyphRequestor& requestor, GlyphDependencies glyphDependencies) {
@@ -60,23 +57,19 @@ void GlyphAtlas::getGlyphs(GlyphRequestor& requestor, GlyphDependencies glyphDep
     // for that requestor have been fetched, and can notify it of completion.
     for (const auto& dependency : *dependencies) {
         const FontStack& fontStack = dependency.first;
-        const GlyphIDs& glyphIDs = dependency.second;
-
         Entry& entry = entries[fontStack];
 
-        GlyphRangeSet processedRanges;
+        const GlyphIDs& glyphIDs = dependency.second;
+        GlyphRangeSet ranges;
         for (const auto& glyphID : glyphIDs) {
-            GlyphRange range = getGlyphRange(glyphID);
-            if (processedRanges.find(range) == processedRanges.end() && !rangeIsParsed(entry.ranges, range)) {
-                if (entry.ranges.find(range) == entry.ranges.end()) {
-                    entry.ranges.emplace(std::piecewise_construct,
-                                         std::forward_as_tuple(range),
-                                         std::forward_as_tuple(this, fontStack, range, this, fileSource));
-                }
+            ranges.insert(getGlyphRange(glyphID));
+        }
 
-                entry.ranges.find(range)->second.requestors[&requestor] = dependencies;
+        for (const auto& range : ranges) {
+            if (!rangeIsParsed(entry.ranges, range)) {
+                GlyphRequest& request = requestRange(entry, fontStack, range);
+                request.requestors[&requestor] = dependencies;
             }
-            processedRanges.insert(range);
         }
     }
 
@@ -87,34 +80,56 @@ void GlyphAtlas::getGlyphs(GlyphRequestor& requestor, GlyphDependencies glyphDep
     }
 }
 
-void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) {
-    observer = observer_;
-}
-    
-void GlyphAtlas::onGlyphsError(const FontStack& fontStack, const GlyphRange& range, std::exception_ptr err) {
-    if (observer) {
-        observer->onGlyphsError(fontStack, range, err);
+GlyphAtlas::GlyphRequest& GlyphAtlas::requestRange(Entry& entry, const FontStack& fontStack, const GlyphRange& range) {
+    GlyphRequest& request = entry.ranges[range];
+
+    if (request.req) {
+        return request;
     }
-}
 
-void GlyphAtlas::onGlyphsLoaded(const FontStack& fontStack, const GlyphRange& range) {
-    Entry& entry = entries[fontStack];
+    request.req = fileSource.request(Resource::glyphs(glyphURL, fontStack, range),
+        [this, &entry, &request, fontStack, range](Response res) {
+            if (res.error) {
+                observer->onGlyphsError(fontStack, range, std::make_exception_ptr(std::runtime_error(res.error->message)));
+            } else if (res.notModified) {
+                return;
+            } else {
+                if (!res.noContent) {
+                    std::vector<SDFGlyph> glyphs;
+
+                    try {
+                        glyphs = parseGlyphPBF(range, *res.data);
+                    } catch (...) {
+                        observer->onGlyphsError(fontStack, range, std::current_exception());
+                        return;
+                    }
+
+                    for (auto& glyph : glyphs) {
+                        entry.glyphSet.insert(std::move(glyph));
+                    }
+                }
+
+                request.parsed = true;
+
+                for (auto& pair : request.requestors) {
+                    GlyphRequestor& requestor = *pair.first;
+                    const std::shared_ptr<GlyphDependencies>& dependencies = pair.second;
+                    if (dependencies.unique()) {
+                        addGlyphs(requestor, *dependencies);
+                    }
+                }
+
+                request.requestors.clear();
 
-    auto it = entry.ranges.find(range);
-    if (it != entry.ranges.end()) {
-        for (auto& pair : it->second.requestors) {
-            GlyphRequestor& requestor = *pair.first;
-            const std::shared_ptr<GlyphDependencies>& dependencies = pair.second;
-            if (dependencies.unique()) {
-                addGlyphs(requestor, *dependencies);
+                observer->onGlyphsLoaded(fontStack, range);
             }
-        }
-        it->second.requestors.clear();
-    }
+        });
 
-    if (observer) {
-        observer->onGlyphsLoaded(fontStack, range);
-    }
+    return request;
+}
+
+void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) {
+    observer = observer_ ? observer_ : &nullObserver;
 }
 
 void GlyphAtlas::addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& glyphDependencies) {
@@ -228,7 +243,7 @@ void GlyphAtlas::removeGlyphValues(GlyphRequestor& requestor, std::map<uint32_t,
     }
 }
 
-void GlyphAtlas::removePendingRanges(mbgl::GlyphRequestor &requestor, std::map<GlyphRange, GlyphPBF> &ranges) {
+void GlyphAtlas::removePendingRanges(mbgl::GlyphRequestor& requestor, std::map<GlyphRange, GlyphRequest>& ranges) {
     for (auto it = ranges.begin(); it != ranges.end(); it++) {
         it->second.requestors.erase(&requestor);
     }
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index 120f19accc..f6491a6d81 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -22,7 +22,7 @@ class GlyphAtlasTest;
 namespace mbgl {
 
 class FileSource;
-class GlyphPBF;
+class AsyncRequest;
 
 namespace gl {
 class Context;
@@ -33,13 +33,13 @@ public:
     virtual void onGlyphsAvailable(GlyphPositionMap, GlyphRangeSet) = 0;
 };
     
-class GlyphAtlas : public util::noncopyable, public GlyphAtlasObserver {
+class GlyphAtlas : public util::noncopyable {
 public:
     GlyphAtlas(Size, FileSource&);
     ~GlyphAtlas();
 
     GlyphSet& getGlyphSet(const FontStack&);
-    
+
     // Workers send a `getGlyphs` message to the main thread once they have determined
     // which glyphs they will need. Invoking this method will increment reference
     // counts for all the glyphs in `GlyphDependencies`. If all glyphs are already
@@ -49,19 +49,14 @@ public:
     // Workers are given a copied 'GlyphPositions' map to use for placing their glyphs.
     // The positions specified in this object are guaranteed to be
     // valid for the lifetime of the tile.
-    void getGlyphs(GlyphRequestor& requestor, GlyphDependencies glyphs);
+    void getGlyphs(GlyphRequestor&, GlyphDependencies);
+    void removeGlyphs(GlyphRequestor&);
 
-    void setURL(const std::string &url) {
+    void setURL(const std::string& url) {
         glyphURL = url;
     }
 
-    std::string getURL() const {
-        return glyphURL;
-    }
-
-    void setObserver(GlyphAtlasObserver* observer);
-
-    void removeGlyphs(GlyphRequestor&);
+    void setObserver(GlyphAtlasObserver*);
 
     // Binds the atlas texture to the GPU, and uploads data if it is out of date.
     void bind(gl::Context&, gl::TextureUnit unit);
@@ -71,21 +66,8 @@ public:
     void upload(gl::Context&, gl::TextureUnit unit);
 
     Size getSize() const;
-    
-    virtual void onGlyphsLoaded(const FontStack&, const GlyphRange&);
-    virtual void onGlyphsError(const FontStack&, const GlyphRange&, std::exception_ptr);
-    
-    friend class ::GlyphAtlasTest;
 
 private:
-    void addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& glyphDependencies);
-
-    // Only used by GlyphAtlasTest
-    bool hasGlyphRanges(const FontStack&, const GlyphRangeSet& ranges) const;
-    bool hasGlyphRange(const FontStack&, const GlyphRange& range) const;
-
-    void addGlyph(GlyphRequestor& requestor, const FontStack&, const SDFGlyph&);
-    
     FileSource& fileSource;
     std::string glyphURL;
 
@@ -96,16 +78,33 @@ private:
         std::unordered_set<GlyphRequestor*> ids;
     };
 
+    struct GlyphRequest {
+        bool parsed = false;
+        std::unique_ptr<AsyncRequest> req;
+        std::unordered_map<GlyphRequestor*, std::shared_ptr<GlyphDependencies>> requestors;
+    };
+
     struct Entry {
-        std::map<GlyphRange, GlyphPBF> ranges;
+        std::map<GlyphRange, GlyphRequest> ranges;
         GlyphSet glyphSet;
         std::map<uint32_t, GlyphValue> glyphValues;
     };
 
     std::unordered_map<FontStack, Entry, FontStackHash> entries;
-    
-    void removeGlyphValues(GlyphRequestor& requestor, std::map<uint32_t, GlyphValue>& face);
-    void removePendingRanges(GlyphRequestor& requestor, std::map<GlyphRange, GlyphPBF>& ranges);
+
+    // Only used by GlyphAtlasTest
+    friend class ::GlyphAtlasTest;
+    bool hasGlyphRanges(const FontStack&, const GlyphRangeSet&) const;
+    bool hasGlyphRange(const FontStack&, const GlyphRange&) const;
+    bool rangeIsParsed(const std::map<GlyphRange, GlyphRequest>&, const GlyphRange&) const;
+
+    GlyphRequest& requestRange(Entry&, const FontStack&, const GlyphRange&);
+
+    void addGlyphs(GlyphRequestor&, const GlyphDependencies&);
+    void addGlyph(GlyphRequestor&, const FontStack&, const SDFGlyph&);
+
+    void removeGlyphValues(GlyphRequestor&, std::map<uint32_t, GlyphValue>&);
+    void removePendingRanges(GlyphRequestor&, std::map<GlyphRange, GlyphRequest>&);
 
     GlyphAtlasObserver* observer = nullptr;
 
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index c49f19c73a..033f50fe9c 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -1,14 +1,4 @@
 #include <mbgl/text/glyph_pbf.hpp>
-#include <mbgl/text/glyph_atlas.hpp>
-#include <mbgl/text/glyph_atlas_observer.hpp>
-#include <mbgl/text/glyph_set.hpp>
-#include <mbgl/storage/file_source.hpp>
-#include <mbgl/storage/resource.hpp>
-#include <mbgl/storage/response.hpp>
-#include <mbgl/util/exception.hpp>
-#include <mbgl/util/string.hpp>
-#include <mbgl/util/token.hpp>
-#include <mbgl/util/url.hpp>
 
 #include <protozero/pbf_reader.hpp>
 
@@ -101,42 +91,4 @@ std::vector<SDFGlyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::str
     return result;
 }
 
-GlyphPBF::GlyphPBF(GlyphAtlas* atlas,
-                   const FontStack& fontStack,
-                   const GlyphRange& glyphRange,
-                   GlyphAtlasObserver* observer_,
-                   FileSource& fileSource)
-    : parsed(false),
-      observer(observer_) {
-    req = fileSource.request(Resource::glyphs(atlas->getURL(), fontStack, glyphRange), [this, atlas, fontStack, glyphRange](Response res) {
-        if (res.error) {
-            observer->onGlyphsError(fontStack, glyphRange, std::make_exception_ptr(std::runtime_error(res.error->message)));
-        } else if (res.notModified) {
-            return;
-        } else if (res.noContent) {
-            parsed = true;
-            observer->onGlyphsLoaded(fontStack, glyphRange);
-        } else {
-            std::vector<SDFGlyph> glyphs;
-
-            try {
-                glyphs = parseGlyphPBF(glyphRange, *res.data);
-            } catch (...) {
-                observer->onGlyphsError(fontStack, glyphRange, std::current_exception());
-                return;
-            }
-
-            GlyphSet& glyphSet = atlas->getGlyphSet(fontStack);
-            for (auto& glyph : glyphs) {
-                glyphSet.insert(std::move(glyph));
-            }
-
-            parsed = true;
-            observer->onGlyphsLoaded(fontStack, glyphRange);
-        }
-    });
-}
-
-GlyphPBF::~GlyphPBF() = default;
-
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index 914338f1ec..162aeed93a 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -1,42 +1,29 @@
 #pragma once
 
 #include <mbgl/text/glyph.hpp>
-#include <mbgl/util/font_stack.hpp>
-#include <mbgl/util/noncopyable.hpp>
+#include <mbgl/text/glyph_range.hpp>
+#include <mbgl/util/image.hpp>
 
-#include <atomic>
-#include <functional>
 #include <string>
-#include <memory>
-#include <unordered_map>
+#include <vector>
 
 namespace mbgl {
 
-class GlyphAtlas;
-class GlyphRequestor;
-class GlyphAtlasObserver;
-class AsyncRequest;
-class FileSource;
+class SDFGlyph {
+public:
+    // We're using this value throughout the Mapbox GL ecosystem. If this is different, the glyphs
+    // also need to be reencoded.
+    static constexpr const uint8_t borderSize = 3;
 
-std::vector<SDFGlyph> parseGlyphPBF(const GlyphRange&, const std::string& data);
+    GlyphID id = 0;
 
-class GlyphPBF : private util::noncopyable {
-public:
-    GlyphPBF(GlyphAtlas*,
-             const FontStack&,
-             const GlyphRange&,
-             GlyphAtlasObserver*,
-             FileSource&);
-    ~GlyphPBF();
-
-    bool isParsed() const {
-        return parsed;
-    }
-
-    bool parsed;
-    std::unique_ptr<AsyncRequest> req;
-    GlyphAtlasObserver* observer = nullptr;
-    std::unordered_map<GlyphRequestor*, std::shared_ptr<GlyphDependencies>> requestors;
+    // A signed distance field of the glyph with a border (see above).
+    AlphaImage bitmap;
+
+    // Glyph metrics
+    GlyphMetrics metrics;
 };
 
+std::vector<SDFGlyph> parseGlyphPBF(const GlyphRange&, const std::string& data);
+
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_set.hpp b/src/mbgl/text/glyph_set.hpp
index 01d89c7f79..8b56ccd7a3 100644
--- a/src/mbgl/text/glyph_set.hpp
+++ b/src/mbgl/text/glyph_set.hpp
@@ -1,7 +1,8 @@
 #pragma once
 
-#include <mbgl/text/glyph.hpp>
-#include <mbgl/util/geometry.hpp>
+#include <mbgl/text/glyph_pbf.hpp>
+
+#include <map>
 
 namespace mbgl {