[core] Test glyph PBF parsing independently of GlyphAtlas

6 files changed, 37 insertions, 78 deletions

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 26eff812b7..c49f19c73a 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -14,10 +14,10 @@
 
 namespace mbgl {
 
-namespace {
+std::vector<SDFGlyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string& data) {
+    std::vector<SDFGlyph> result;
+    result.reserve(256);
 
-// Parses a Glyph Protobuf and inserts it into the GlyphAtlas. Must be called from main thread.
-void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::string& data) {
     protozero::pbf_reader glyphs_pbf(data);
 
     while (glyphs_pbf.next(1)) {
@@ -94,12 +94,12 @@ void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::
                 glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
             }
 
-            glyphSet.insert(glyph.id, std::move(glyph));
+            result.push_back(std::move(glyph));
         }
     }
-}
 
-} // namespace
+    return result;
+}
 
 GlyphPBF::GlyphPBF(GlyphAtlas* atlas,
                    const FontStack& fontStack,
@@ -117,13 +117,20 @@ GlyphPBF::GlyphPBF(GlyphAtlas* atlas,
             parsed = true;
             observer->onGlyphsLoaded(fontStack, glyphRange);
         } else {
+            std::vector<SDFGlyph> glyphs;
+
             try {
-                parseGlyphPBF(atlas->getGlyphSet(fontStack), glyphRange, *res.data);
+                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);
         }
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index d5b89cd107..914338f1ec 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -18,6 +18,8 @@ class GlyphAtlasObserver;
 class AsyncRequest;
 class FileSource;
 
+std::vector<SDFGlyph> parseGlyphPBF(const GlyphRange&, const std::string& data);
+
 class GlyphPBF : private util::noncopyable {
 public:
     GlyphPBF(GlyphAtlas*,
diff --git a/src/mbgl/text/glyph_set.cpp b/src/mbgl/text/glyph_set.cpp
index b8e155502e..3305d4136e 100644
--- a/src/mbgl/text/glyph_set.cpp
+++ b/src/mbgl/text/glyph_set.cpp
@@ -3,11 +3,11 @@
 
 namespace mbgl {
 
-void GlyphSet::insert(uint32_t id, SDFGlyph&& glyph) {
-    auto it = sdfs.find(id);
+void GlyphSet::insert(SDFGlyph&& glyph) {
+    auto it = sdfs.find(glyph.id);
     if (it == sdfs.end()) {
         // Glyph doesn't exist yet.
-        sdfs.emplace(id, std::move(glyph));
+        sdfs.emplace(glyph.id, std::move(glyph));
     } else if (it->second.metrics == glyph.metrics) {
         if (it->second.bitmap != glyph.bitmap) {
             // The actual bitmap was updated; this is unsupported.
diff --git a/src/mbgl/text/glyph_set.hpp b/src/mbgl/text/glyph_set.hpp
index 9f4bef94d2..01d89c7f79 100644
--- a/src/mbgl/text/glyph_set.hpp
+++ b/src/mbgl/text/glyph_set.hpp
@@ -7,7 +7,7 @@ namespace mbgl {
 
 class GlyphSet {
 public:
-    void insert(uint32_t id, SDFGlyph&&);
+    void insert(SDFGlyph&&);
     const std::map<uint32_t, SDFGlyph>& getSDFs() const;
 
 private:
diff --git a/test/text/glyph_atlas.test.cpp b/test/text/glyph_atlas.test.cpp
index fe27e4c6fe..f38c18c28c 100644
--- a/test/text/glyph_atlas.test.cpp
+++ b/test/text/glyph_atlas.test.cpp
@@ -166,11 +166,11 @@ TEST(GlyphAtlas, InvalidSDFGlyph) {
     GlyphAtlasTest test;
 
     auto& glyphSet = test.glyphAtlas.getGlyphSet(fontStack);
-    glyphSet.insert(66, SDFGlyph{ 66 /* ASCII 'B' */,
+    glyphSet.insert(SDFGlyph{ 66 /* ASCII 'B' */,
                                   AlphaImage({7, 7}), /* correct */
                                   { 1 /* width */, 1 /* height */, 0 /* left */, 0 /* top */,
                                     0 /* advance */ } });
-    glyphSet.insert(67, SDFGlyph{ 67 /* ASCII 'C' */,
+    glyphSet.insert(SDFGlyph{ 67 /* ASCII 'C' */,
                                   AlphaImage({518, 8}), /* correct */
                                   { 512 /* width */, 2 /* height */, 0 /* left */, 0 /* top */,
                                     0 /* advance */ } });
diff --git a/test/text/glyph_pbf.test.cpp b/test/text/glyph_pbf.test.cpp
index 48b2d7a07c..c222ec1dd9 100644
--- a/test/text/glyph_pbf.test.cpp
+++ b/test/text/glyph_pbf.test.cpp
@@ -1,73 +1,23 @@
 #include <mbgl/test/util.hpp>
 
-#include <mbgl/text/glyph_set.hpp>
-#include <mbgl/text/glyph_atlas_observer.hpp>
-#include <mbgl/text/glyph_atlas.hpp>
 #include <mbgl/text/glyph_pbf.hpp>
-#include <mbgl/storage/default_file_source.hpp>
-#include <mbgl/util/run_loop.hpp>
-#include <mbgl/util/string.hpp>
-
-#include <future>
+#include <mbgl/util/io.hpp>
 
 using namespace mbgl;
 
-using namespace std::string_literals;
-
-class MockGlyphAtlasObserver : public GlyphAtlasObserver {
-public:
-    std::function<void(const FontStack&, const GlyphRange&)> glyphsLoaded;
-    std::function<void(const FontStack&, const GlyphRange&, std::exception_ptr)> glyphsError;
-
-    void onGlyphsLoaded(const FontStack& fontStack, const GlyphRange& glyphRange) override {
-        if (glyphsLoaded) {
-            glyphsLoaded(fontStack, glyphRange);
-        }
-    }
-    void onGlyphsError(const FontStack& fontStack, const GlyphRange& glyphRange, std::exception_ptr error) override {
-        if (glyphsError) {
-            glyphsError(fontStack, glyphRange, error);
-        }
-    }
-};
-
 TEST(GlyphPBF, Parsing) {
-    util::RunLoop loop;
-    DefaultFileSource fileSource{ ":memory:", "." };
-    GlyphAtlas glyphAtlas{ { 1024, 1024 }, fileSource };
-    FontStack fontStack{ "fake_glyphs" };
-    GlyphRange glyphRange{ 0, 255 };
-
-    glyphAtlas.setURL("asset://test/fixtures/resources/{fontstack}-{range}.pbf");
-
-    MockGlyphAtlasObserver glyphAtlasObserver;
-    glyphAtlasObserver.glyphsLoaded = [&](const FontStack&, const GlyphRange&) {
-        loop.stop();
-
-        const auto& sdfs = glyphAtlas.getGlyphSet(fontStack).getSDFs();
-
-        // The fake glyphs don't contain a glyph that has the ID 0; it only contains glyphs with
-        // undefined IDs, but the parser should remove them.
-
-        EXPECT_TRUE(sdfs.size() == 1);
-        EXPECT_TRUE(sdfs.find(69) != sdfs.end());
-        auto& sdf = sdfs.at(69);
-        AlphaImage expected({7, 7});
-        expected.fill('x');
-        EXPECT_EQ(expected, sdf.bitmap);
-        EXPECT_EQ(1u, sdf.metrics.width);
-        EXPECT_EQ(1u, sdf.metrics.height);
-        EXPECT_EQ(20, sdf.metrics.left);
-        EXPECT_EQ(2, sdf.metrics.top);
-        EXPECT_EQ(8u, sdf.metrics.advance);
-    };
-
-    glyphAtlasObserver.glyphsError = [&](const FontStack&, const GlyphRange&, std::exception_ptr error) {
-        loop.stop();
-        FAIL() << util::toString(error);
-    };
-
-    GlyphPBF pbf(&glyphAtlas, fontStack, glyphRange, &glyphAtlasObserver, fileSource);
-
-    loop.run();
+    // The fake glyphs contain a number of invalid glyphs, which should be skipped by the parser.
+    auto sdfs = parseGlyphPBF(GlyphRange { 0, 255 }, util::read_file("test/fixtures/resources/fake_glyphs-0-255.pbf"));
+    EXPECT_TRUE(sdfs.size() == 1);
+
+    auto& sdf = sdfs[0];
+    EXPECT_EQ(69u, sdf.id);
+    AlphaImage expected({7, 7});
+    expected.fill('x');
+    EXPECT_EQ(expected, sdf.bitmap);
+    EXPECT_EQ(1u, sdf.metrics.width);
+    EXPECT_EQ(1u, sdf.metrics.height);
+    EXPECT_EQ(20, sdf.metrics.left);
+    EXPECT_EQ(2, sdf.metrics.top);
+    EXPECT_EQ(8u, sdf.metrics.advance);
 }