Report Sprite parsing errors to Observers

Parsing errors will be reported to Observers. In practice this will
bubble all the way to the Map object.

This will make possible to identify possible corrupted data instead
of rendering without sprites like we do in case of semantic errors.

src/mbgl/annotation/sprite_parser.cpp

@@ -9,6 +9,7 @@
 
 #include <cmath>
 #include <limits>
+#include <sstream>
 
 namespace mbgl {
 
@@ -99,7 +100,7 @@ inline bool getBoolean(const rapidjson::Value& value, const char* name, const bo
 
 } // namespace
 
-Sprites parseSprite(const std::string& image, const std::string& json) {
+SpriteParseResult parseSprite(const std::string& image, const std::string& json) {
     using namespace rapidjson;
 
     Sprites sprites;
@@ -107,20 +108,18 @@ Sprites parseSprite(const std::string& image, const std::string& json) {
     // Parse the sprite image.
     const util::Image raster(image);
     if (!raster) {
-        Log::Warning(Event::Sprite, "Could not parse sprite image");
-        return sprites;
+        return std::string("Could not parse sprite image");
     }
 
     Document doc;
     doc.Parse<0>(json.c_str());
 
     if (doc.HasParseError()) {
-        Log::Warning(Event::Sprite, std::string{ "Failed to parse JSON: " } + doc.GetParseError() +
-                                        " at offset " + std::to_string(doc.GetErrorOffset()));
-        return sprites;
+        std::stringstream message;
+        message << "Failed to parse JSON: " << doc.GetParseError() << " at offset " << doc.GetErrorOffset();
+        return message.str();
     } else if (!doc.IsObject()) {
-        Log::Warning(Event::Sprite, "Sprite JSON root must be an object");
-        return sprites;
+        return std::string("Sprite JSON root must be an object");
     } else {
         for (Value::ConstMemberIterator itr = doc.MemberBegin(); itr != doc.MemberEnd(); ++itr) {
             const std::string name = { itr->name.GetString(), itr->name.GetStringLength() };

src/mbgl/annotation/sprite_parser.hpp

@@ -3,6 +3,7 @@
 
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/geo.hpp>
+#include <mbgl/util/variant.hpp>
 
 #include <string>
 #include <memory>
@@ -31,8 +32,13 @@ SpriteImagePtr createSpriteImage(const util::Image& image,
 
 using Sprites = std::map<std::string, SpriteImagePtr>;
 
+
+using SpriteParseResult = mapbox::util::variant<
+    Sprites,      // success
+    std::string>; // error
+
 // Parses an image and an associated JSON file and returns the sprite objects.
-Sprites parseSprite(const std::string& image, const std::string& json);
+SpriteParseResult parseSprite(const std::string& image, const std::string& json);
 
 } // namespace mbgl
 

src/mbgl/map/sprite.cpp

@@ -85,11 +85,19 @@ Sprite::~Sprite() {
 
 void Sprite::emitSpriteLoadedIfComplete() {
     assert(loader);
-    if (loader->loadedImage && loader->loadedJSON && observer) {
-        std::unique_ptr<Data> data(std::move(loader->data));
-        loader.reset();
 
-        observer->onSpriteLoaded(std::move(data));
+    if (!loader->loadedImage || !loader->loadedJSON || !observer) {
+        return;
+    }
+
+    std::unique_ptr<Data> data(std::move(loader->data));
+    loader.reset();
+
+    auto result = parseSprite(data->image, data->json);
+    if (result.is<Sprites>()) {
+        observer->onSpriteLoaded(result.get<Sprites>());
+    } else {
+        emitSpriteLoadingFailed(result.get<std::string>());
     }
 }
 

src/mbgl/map/sprite.hpp

@@ -1,6 +1,7 @@
 #ifndef MBGL_STYLE_SPRITE
 #define MBGL_STYLE_SPRITE
 
+#include <mbgl/annotation/sprite_parser.hpp>
 #include <mbgl/util/image.hpp>
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/ptr.hpp>
@@ -27,7 +28,7 @@ public:
     public:
         virtual ~Observer() = default;
 
-        virtual void onSpriteLoaded(std::unique_ptr<Data>) = 0;
+        virtual void onSpriteLoaded(const Sprites& sprites) = 0;
         virtual void onSpriteLoadingFailed(std::exception_ptr error) = 0;
     };
 

src/mbgl/style/style.cpp

@@ -4,7 +4,6 @@
 #include <mbgl/map/source.hpp>
 #include <mbgl/map/transform_state.hpp>
 #include <mbgl/annotation/sprite_store.hpp>
-#include <mbgl/annotation/sprite_parser.hpp>
 #include <mbgl/style/style_layer.hpp>
 #include <mbgl/style/style_parser.hpp>
 #include <mbgl/style/style_bucket.hpp>
@@ -191,9 +190,9 @@ void Style::onTileLoadingFailed(std::exception_ptr error) {
     emitResourceLoadingFailed(error);
 }
 
-void Style::onSpriteLoaded(std::unique_ptr<Sprite::Data> spriteData) {
+void Style::onSpriteLoaded(const Sprites& sprites) {
     // Add all sprite images to the SpriteStore object
-    spriteStore->setSprites(parseSprite(spriteData->image, spriteData->json));
+    spriteStore->setSprites(sprites);
 
     shouldReparsePartialTiles = true;
     emitTileDataChanged();

src/mbgl/style/style.hpp

@@ -89,7 +89,7 @@ private:
     void onTileLoadingFailed(std::exception_ptr error) override;
 
     // Sprite::Observer implementation.
-    void onSpriteLoaded(std::unique_ptr<Sprite::Data>) override;
+    void onSpriteLoaded(const Sprites& sprites) override;
     void onSpriteLoadingFailed(std::exception_ptr error) override;
 
     void emitTileDataChanged();

test/annotations/sprite_atlas.cpp

@@ -12,9 +12,11 @@ using namespace mbgl;
 TEST(Annotations, SpriteAtlas) {
     FixtureLog log;
 
+    auto spriteParseResult = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
+                                         util::read_file("test/fixtures/annotations/emerald.json"));
+
     SpriteStore store;
-    store.setSprites(parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
-                                 util::read_file("test/fixtures/annotations/emerald.json")));
+    store.setSprites(spriteParseResult.get<Sprites>());
 
     SpriteAtlas atlas(63, 112, 1, store);
 
@@ -85,9 +87,12 @@ TEST(Annotations, SpriteAtlas) {
 }
 
 TEST(Annotations, SpriteAtlasSize) {
+    auto spriteParseResult = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
+                                         util::read_file("test/fixtures/annotations/emerald.json"));
+
     SpriteStore store;
-    store.setSprites(parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
-                                 util::read_file("test/fixtures/annotations/emerald.json")));
+    store.setSprites(spriteParseResult.get<Sprites>());
+
     SpriteAtlas atlas(63, 112, 1.4, store);
 
     EXPECT_DOUBLE_EQ(1.4f, atlas.getPixelRatio());

test/annotations/sprite_parser.cpp

@@ -117,7 +117,7 @@ TEST(Annotations, SpriteParsing) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = util::read_file("test/fixtures/annotations/emerald.json");
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
 
     std::set<std::string> names;
     std::transform(images.begin(), images.end(), std::inserter(names, names.begin()),
@@ -210,22 +210,13 @@ TEST(Annotations, SpriteParsing) {
 }
 
 TEST(Annotations, SpriteParsingInvalidJSON) {
-    FixtureLog log;
-
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": " })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
-    EXPECT_EQ(0u, images.size());
+    const auto error = parseSprite(image_1x, json_1x).get<std::string>();
 
-    EXPECT_EQ(
-        1u,
-        log.count({
-            EventSeverity::Warning,
-            Event::Sprite,
-            int64_t(-1),
-            "Failed to parse JSON: lacks ending quotation before the end of string at offset 10",
-        }));
+    EXPECT_EQ(error,
+              std::string("Failed to parse JSON: lacks ending quotation before the end of string at offset 10"));
 }
 
 TEST(Annotations, SpriteParsingEmptyImage) {
@@ -234,7 +225,7 @@ TEST(Annotations, SpriteParsingEmptyImage) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": {} })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
     EXPECT_EQ(0u, images.size());
 
     EXPECT_EQ(1u, log.count({
@@ -251,7 +242,7 @@ TEST(Annotations, SpriteParsingSimpleWidthHeight) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": { "width": 32, "height": 32 } })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
     EXPECT_EQ(1u, images.size());
 }
 
@@ -261,7 +252,7 @@ TEST(Annotations, SpriteParsingWidthTooBig) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": { "width": 65536, "height": 32 } })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
     EXPECT_EQ(0u, images.size());
 
     EXPECT_EQ(1u, log.count({
@@ -284,7 +275,7 @@ TEST(Annotations, SpriteParsingNegativeWidth) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": { "width": -1, "height": 32 } })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
     EXPECT_EQ(0u, images.size());
 
     EXPECT_EQ(1u, log.count({
@@ -307,7 +298,7 @@ TEST(Annotations, SpriteParsingNullRatio) {
     const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png");
     const auto json_1x = R"JSON({ "image": { "width": 32, "height": 32, "pixelRatio": 0 } })JSON";
 
-    const auto images = parseSprite(image_1x, json_1x);
+    const auto images = parseSprite(image_1x, json_1x).get<Sprites>();
     EXPECT_EQ(0u, images.size());
 
     EXPECT_EQ(1u, log.count({