[core] Loading images to style optimization

This change enables attaching images to the style with batches and avoids massive re-allocations. Thus, it improves UI performance especially at start-up time.
mapbox · Feb 11, 2020 · dc136ce · dc136ce
1 parent a231005
commit dc136ce
Show file tree

Hide file tree

Showing 15 changed files with 117 additions and 91 deletions.
diff --git a/include/mbgl/style/style.hpp b/include/mbgl/style/style.hpp
@@ -1,8 +1,10 @@
 #pragma once
 
-#include <mbgl/style/transition_options.hpp>
 #include <mbgl/map/camera.hpp>
+#include <mbgl/style/transition_options.hpp>
 #include <mbgl/util/geo.hpp>
+#include <mbgl/util/image.hpp>
+#include <mbgl/util/immutable.hpp>
 
 #include <string>
 #include <vector>
@@ -45,7 +47,7 @@ class Style {
     void setLight(std::unique_ptr<Light>);
 
     // Images
-    const Image* getImage(const std::string&) const;
+    const PremultipliedImage* getImage(const std::string&) const;
     void addImage(std::unique_ptr<Image>);
     void removeImage(const std::string&);
 

diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp
@@ -1099,12 +1099,10 @@ void NativeMapView::removeImage(JNIEnv& env, const jni::String& name) {
 }
 
 jni::Local<jni::Object<Bitmap>> NativeMapView::getImage(JNIEnv& env, const jni::String& name) {
-    const mbgl::style::Image *image = map->getStyle().getImage(jni::Make<std::string>(env, name));
-    if (image) {
-        return Bitmap::CreateBitmap(env, image->getImage());
-    } else {
-        return jni::Local<jni::Object<Bitmap>>();
+    if (auto* image = map->getStyle().getImage(jni::Make<std::string>(env, name))) {
+        return Bitmap::CreateBitmap(env, *image);
     }
+    return jni::Local<jni::Object<Bitmap>>();
 }
 
 void NativeMapView::setPrefetchTiles(JNIEnv&, jni::jboolean enable) {

diff --git a/src/mbgl/map/map_impl.cpp b/src/mbgl/map/map_impl.cpp
@@ -170,7 +170,6 @@ void Map::Impl::jumpTo(const CameraOptions& camera) {
 }
 
 void Map::Impl::onStyleImageMissing(const std::string& id, std::function<void()> done) {
-
     if (style->getImage(id) == nullptr) {
         observer.onStyleImageMissing(id);
     }

diff --git a/src/mbgl/sprite/sprite_loader.cpp b/src/mbgl/sprite/sprite_loader.cpp
@@ -90,7 +90,7 @@ void SpriteLoader::emitSpriteLoadedIfComplete() {
     loader->worker.self().invoke(&SpriteLoaderWorker::parse, loader->image, loader->json);
 }
 
-void SpriteLoader::onParsed(std::vector<std::unique_ptr<style::Image>>&& result) {
+void SpriteLoader::onParsed(std::vector<Immutable<style::Image::Impl>> result) {
     observer->onSpriteLoaded(std::move(result));
 }
 

diff --git a/src/mbgl/sprite/sprite_loader.hpp b/src/mbgl/sprite/sprite_loader.hpp
@@ -1,6 +1,5 @@
 #pragma once
 
-#include <mbgl/util/noncopyable.hpp>
 #include <mbgl/style/image.hpp>
 
 #include <string>
@@ -15,7 +14,7 @@ namespace mbgl {
 class FileSource;
 class SpriteLoaderObserver;
 
-class SpriteLoader : public util::noncopyable {
+class SpriteLoader {
 public:
     SpriteLoader(float pixelRatio);
     ~SpriteLoader();
@@ -29,7 +28,7 @@ class SpriteLoader : public util::noncopyable {
 
     // Invoked by SpriteAtlasWorker
     friend class SpriteLoaderWorker;
-    void onParsed(std::vector<std::unique_ptr<style::Image>>&&);
+    void onParsed(std::vector<Immutable<style::Image::Impl>>);
     void onError(std::exception_ptr);
 
     const float pixelRatio;

diff --git a/src/mbgl/sprite/sprite_loader_observer.hpp b/src/mbgl/sprite/sprite_loader_observer.hpp
@@ -1,7 +1,9 @@
 #pragma once
 
+#include <mbgl/style/image.hpp>
+#include <mbgl/util/immutable.hpp>
+
 #include <exception>
-#include <memory>
 #include <vector>
 
 namespace mbgl {
@@ -13,8 +15,7 @@ class Image;
 class SpriteLoaderObserver {
 public:
     virtual ~SpriteLoaderObserver() = default;
-
-    virtual void onSpriteLoaded(std::vector<std::unique_ptr<style::Image>>&&) {}
+    virtual void onSpriteLoaded(std::vector<Immutable<style::Image::Impl>>) {}
     virtual void onSpriteError(std::exception_ptr) {}
 };
 

diff --git a/src/mbgl/sprite/sprite_parser.cpp b/src/mbgl/sprite/sprite_parser.cpp
@@ -149,50 +149,54 @@ optional<style::ImageContent> getContent(const JSValue& value, const char* prope
 
 } // namespace
 
-std::vector<std::unique_ptr<style::Image>> parseSprite(const std::string& encodedImage, const std::string& json) {
+std::vector<Immutable<style::Image::Impl>> parseSprite(const std::string& encodedImage, const std::string& json) {
     const PremultipliedImage raster = decodeImage(encodedImage);
 
     JSDocument doc;
     doc.Parse<0>(json.c_str());
     if (doc.HasParseError()) {
         throw std::runtime_error("Failed to parse JSON: " + formatJSONParseError(doc));
-    } else if (!doc.IsObject()) {
+    }
+
+    if (!doc.IsObject()) {
         throw std::runtime_error("Sprite JSON root must be an object");
-    } else {
-        std::vector<std::unique_ptr<style::Image>> images;
-        for (const auto& property : doc.GetObject()) {
-            const std::string name = { property.name.GetString(), property.name.GetStringLength() };
-            const JSValue& value = property.value;
-
-            if (value.IsObject()) {
-                const uint16_t x = getUInt16(value, "x", name.c_str(), 0);
-                const uint16_t y = getUInt16(value, "y", name.c_str(), 0);
-                const uint16_t width = getUInt16(value, "width", name.c_str(), 0);
-                const uint16_t height = getUInt16(value, "height", name.c_str(), 0);
-                const double pixelRatio = getDouble(value, "pixelRatio", name.c_str(), 1);
-                const bool sdf = getBoolean(value, "sdf", name.c_str(), false);
-                style::ImageStretches stretchX = getStretches(value, "stretchX", name.c_str());
-                style::ImageStretches stretchY = getStretches(value, "stretchY", name.c_str());
-                optional<style::ImageContent> content = getContent(value, "content", name.c_str());
-
-                auto image = createStyleImage(name,
-                                              raster,
-                                              x,
-                                              y,
-                                              width,
-                                              height,
-                                              pixelRatio,
-                                              sdf,
-                                              std::move(stretchX),
-                                              std::move(stretchY),
-                                              std::move(content));
-                if (image) {
-                    images.push_back(std::move(image));
-                }
+    }
+
+    const auto& properties = doc.GetObject();
+    std::vector<Immutable<style::Image::Impl>> images;
+    images.reserve(properties.MemberCount());
+    for (const auto& property : properties) {
+        const std::string name = {property.name.GetString(), property.name.GetStringLength()};
+        const JSValue& value = property.value;
+
+        if (value.IsObject()) {
+            const uint16_t x = getUInt16(value, "x", name.c_str(), 0);
+            const uint16_t y = getUInt16(value, "y", name.c_str(), 0);
+            const uint16_t width = getUInt16(value, "width", name.c_str(), 0);
+            const uint16_t height = getUInt16(value, "height", name.c_str(), 0);
+            const double pixelRatio = getDouble(value, "pixelRatio", name.c_str(), 1);
+            const bool sdf = getBoolean(value, "sdf", name.c_str(), false);
+            style::ImageStretches stretchX = getStretches(value, "stretchX", name.c_str());
+            style::ImageStretches stretchY = getStretches(value, "stretchY", name.c_str());
+            optional<style::ImageContent> content = getContent(value, "content", name.c_str());
+
+            auto image = createStyleImage(name,
+                                          raster,
+                                          x,
+                                          y,
+                                          width,
+                                          height,
+                                          pixelRatio,
+                                          sdf,
+                                          std::move(stretchX),
+                                          std::move(stretchY),
+                                          std::move(content));
+            if (image) {
+                images.push_back(std::move(image->baseImpl));
             }
         }
-        return images;
     }
+    return images;
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/sprite/sprite_parser.hpp b/src/mbgl/sprite/sprite_parser.hpp
@@ -20,6 +20,6 @@ std::unique_ptr<style::Image> createStyleImage(const std::string& id,
                                                optional<style::ImageContent> content = nullopt);
 
 // Parses an image and an associated JSON file and returns the sprite objects.
-std::vector<std::unique_ptr<style::Image>> parseSprite(const std::string& image, const std::string& json);
+std::vector<Immutable<style::Image::Impl>> parseSprite(const std::string& image, const std::string& json);
 
 } // namespace mbgl
diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp
@@ -1,9 +1,10 @@
-#include <mbgl/style/style.hpp>
-#include <mbgl/style/style_impl.hpp>
-#include <mbgl/style/light.hpp>
 #include <mbgl/style/image.hpp>
-#include <mbgl/style/source.hpp>
+#include <mbgl/style/image_impl.hpp>
 #include <mbgl/style/layer.hpp>
+#include <mbgl/style/light.hpp>
+#include <mbgl/style/source.hpp>
+#include <mbgl/style/style.hpp>
+#include <mbgl/style/style_impl.hpp>
 
 namespace mbgl {
 namespace style {
@@ -59,8 +60,11 @@ const Light* Style::getLight() const {
     return impl->getLight();
 }
 
-const Image* Style::getImage(const std::string& name) const {
-    return impl->getImage(name);
+const PremultipliedImage* Style::getImage(const std::string& name) const {
+    if (auto* image = impl->getImage(name)) {
+        return &(image->image);
+    }
+    return nullptr;
 }
 
 void Style::addImage(std::unique_ptr<Image> image) {

diff --git a/src/mbgl/style/style_impl.cpp b/src/mbgl/style/style_impl.cpp
@@ -98,7 +98,7 @@ void Style::Impl::parse(const std::string& json_) {
 
     sources.clear();
     layers.clear();
-    images.clear();
+    images = makeMutable<ImageImpls>();
 
     transitionOptions = parser.transition;
 
@@ -274,17 +274,31 @@ bool Style::Impl::isLoaded() const {
 }
 
 void Style::Impl::addImage(std::unique_ptr<style::Image> image) {
-    images.remove(image->getID()); // We permit using addImage to update.
-    images.add(std::move(image));
+    auto newImages = makeMutable<ImageImpls>(*images);
+    auto it =
+        std::lower_bound(newImages->begin(), newImages->end(), image->getID(), [](const auto& a, const std::string& b) {
+            return a->id < b;
+        });
+    if (it != newImages->end() && (*it)->id == image->getID()) {
+        // We permit using addImage to update.
+        *it = std::move(image->baseImpl);
+    } else {
+        newImages->insert(it, std::move(image->baseImpl));
+    }
+    images = std::move(newImages);
     observer->onUpdate();
 }
 
 void Style::Impl::removeImage(const std::string& id) {
-    images.remove(id);
+    auto newImages = makeMutable<ImageImpls>(*images);
+    std::remove_if(newImages->begin(), newImages->end(), [&id](const auto& image) { return image->id == id; });
+    images = std::move(newImages);
 }
 
-const style::Image* Style::Impl::getImage(const std::string& id) const {
-    return images.get(id);
+const style::Image::Impl* Style::Impl::getImage(const std::string& id) const {
+    auto found = std::find_if(images->begin(), images->end(), [&id](const auto& image) { return image->id == id; });
+    if (found == images->end()) return nullptr;
+    return found->get();
 }
 
 void Style::Impl::setObserver(style::Observer* observer_) {
@@ -319,10 +333,12 @@ void Style::Impl::onSourceDescriptionChanged(Source& source) {
     }
 }
 
-void Style::Impl::onSpriteLoaded(std::vector<std::unique_ptr<Image>>&& images_) {
-    for (auto& image : images_) {
-        addImage(std::move(image));
-    }
+void Style::Impl::onSpriteLoaded(std::vector<Immutable<style::Image::Impl>> images_) {
+    auto newImages = makeMutable<ImageImpls>(*images);
+    newImages->insert(
+        newImages->end(), std::make_move_iterator(images_.begin()), std::make_move_iterator(images_.end()));
+    std::sort(newImages->begin(), newImages->end(), [](const auto& a, const auto& b) { return a->id < b->id; });
+    images = std::move(newImages);
     spriteLoaded = true;
     observer->onUpdate(); // For *-pattern properties.
 }
@@ -357,7 +373,7 @@ const std::string& Style::Impl::getGlyphURL() const {
 }
 
 Immutable<std::vector<Immutable<Image::Impl>>> Style::Impl::getImageImpls() const {
-    return images.getImpls();
+    return images;
 }
 
 Immutable<std::vector<Immutable<Source::Impl>>> Style::Impl::getSourceImpls() const {

diff --git a/src/mbgl/style/style_impl.hpp b/src/mbgl/style/style_impl.hpp
@@ -78,13 +78,14 @@ class Style::Impl : public SpriteLoaderObserver,
     void setLight(std::unique_ptr<Light>);
     Light* getLight() const;
 
-    const style::Image* getImage(const std::string&) const;
+    const style::Image::Impl* getImage(const std::string&) const;
     void addImage(std::unique_ptr<style::Image>);
     void removeImage(const std::string&);
 
     const std::string& getGlyphURL() const;
 
-    Immutable<std::vector<Immutable<Image::Impl>>> getImageImpls() const;
+    using ImageImpls = std::vector<Immutable<Image::Impl>>;
+    Immutable<ImageImpls> getImageImpls() const;
     Immutable<std::vector<Immutable<Source::Impl>>> getSourceImpls() const;
     Immutable<std::vector<Immutable<Layer::Impl>>> getLayerImpls() const;
 
@@ -106,7 +107,7 @@ class Style::Impl : public SpriteLoaderObserver,
     std::unique_ptr<SpriteLoader> spriteLoader;
 
     std::string glyphURL;
-    CollectionWithPersistentOrder<style::Image> images;
+    Immutable<ImageImpls> images = makeMutable<ImageImpls>();
     CollectionWithPersistentOrder<Source> sources;
     Collection<Layer> layers;
     TransitionOptions transitionOptions;
@@ -117,7 +118,7 @@ class Style::Impl : public SpriteLoaderObserver,
     CameraOptions defaultCamera;
 
     // SpriteLoaderObserver implementation.
-    void onSpriteLoaded(std::vector<std::unique_ptr<Image>>&&) override;
+    void onSpriteLoaded(std::vector<Immutable<style::Image::Impl>>) override;
     void onSpriteError(std::exception_ptr) override;
 
     // SourceObserver implementation.

diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp
@@ -27,10 +27,10 @@ TEST(ImageManager, Basic) {
     auto images = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
                               util::read_file("test/fixtures/annotations/emerald.json"));
     for (auto& image : images) {
-        imageManager.addImage(image->baseImpl);
-        auto* stored = imageManager.getImage(image->getID());
+        imageManager.addImage(image);
+        auto* stored = imageManager.getImage(image->id);
         ASSERT_TRUE(stored);
-        EXPECT_EQ(image->getImage().size, stored->image.size);
+        EXPECT_EQ(image->image.size, stored->image.size);
     }
 }
 

diff --git a/test/renderer/pattern_atlas.test.cpp b/test/renderer/pattern_atlas.test.cpp
@@ -21,8 +21,8 @@ TEST(PatternAtlas, Basic) {
     auto images = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"),
                               util::read_file("test/fixtures/annotations/emerald.json"));
     for (auto& image : images) {
-        if (image->getID() == "metro") {
-            ASSERT_TRUE(patternAtlas.addPattern(*image->baseImpl));
+        if (image->id == "metro") {
+            ASSERT_TRUE(patternAtlas.addPattern(*image));
         }
     }
     auto found = patternAtlas.getPattern("metro");

diff --git a/test/sprite/sprite_loader.test.cpp b/test/sprite/sprite_loader.test.cpp
@@ -17,15 +17,15 @@ using namespace mbgl::style;
 
 class StubSpriteLoaderObserver : public SpriteLoaderObserver {
 public:
-    void onSpriteLoaded(std::vector<std::unique_ptr<style::Image>>&& images) override {
+    void onSpriteLoaded(std::vector<Immutable<style::Image::Impl>> images) override {
         if (spriteLoaded) spriteLoaded(std::move(images));
     }
 
     void onSpriteError(std::exception_ptr error) override {
         if (spriteError) spriteError(error);
     }
 
-    std::function<void (std::vector<std::unique_ptr<style::Image>>&&)> spriteLoaded;
+    std::function<void(std::vector<Immutable<style::Image::Impl>>)> spriteLoaded;
     std::function<void (std::exception_ptr)> spriteError;
 };
 
@@ -92,7 +92,7 @@ TEST(SpriteLoader, LoadingSuccess) {
         test.end();
     };
 
-    test.observer.spriteLoaded = [&] (std::vector<std::unique_ptr<style::Image>>&& images) {
+    test.observer.spriteLoaded = [&](std::vector<Immutable<style::Image::Impl>> images) {
         EXPECT_EQ(images.size(), 367u);
         test.end();
     };
@@ -169,7 +169,7 @@ TEST(SpriteLoader, LoadingCancel) {
         return optional<Response>();
     };
 
-    test.observer.spriteLoaded = [&] (const std::vector<std::unique_ptr<style::Image>>&) {
+    test.observer.spriteLoaded = [&](std::vector<Immutable<style::Image::Impl>>) {
         FAIL() << "Should never be called";
     };