From 64e2bcdf4395a1c65cd9faca81f98c4c5d974bd6 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Tue, 11 Feb 2020 01:54:05 +0200 Subject: [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. --- include/mbgl/style/style.hpp | 6 ++- platform/android/src/native_map_view.cpp | 8 ++-- src/mbgl/map/map_impl.cpp | 1 - src/mbgl/sprite/sprite_loader.cpp | 2 +- src/mbgl/sprite/sprite_loader.hpp | 5 +-- src/mbgl/sprite/sprite_loader_observer.hpp | 7 +-- src/mbgl/sprite/sprite_parser.cpp | 72 ++++++++++++++++-------------- src/mbgl/sprite/sprite_parser.hpp | 2 +- src/mbgl/style/style.cpp | 16 ++++--- src/mbgl/style/style_impl.cpp | 44 +++++++++++++----- src/mbgl/style/style_impl.hpp | 9 ++-- test/renderer/image_manager.test.cpp | 6 +-- test/renderer/pattern_atlas.test.cpp | 4 +- test/sprite/sprite_loader.test.cpp | 8 ++-- test/sprite/sprite_parser.test.cpp | 24 +++++----- 15 files changed, 123 insertions(+), 91 deletions(-) diff --git a/include/mbgl/style/style.hpp b/include/mbgl/style/style.hpp index 83d6ad5bb4..2e47198930 100644 --- a/include/mbgl/style/style.hpp +++ b/include/mbgl/style/style.hpp @@ -1,8 +1,10 @@ #pragma once -#include #include +#include #include +#include +#include #include #include @@ -45,7 +47,7 @@ public: void setLight(std::unique_ptr); // Images - const Image* getImage(const std::string&) const; + const PremultipliedImage* getImage(const std::string&) const; void addImage(std::unique_ptr); void removeImage(const std::string&); diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp index 920779c18f..604fdbe2ae 100644 --- 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> NativeMapView::getImage(JNIEnv& env, const jni::String& name) { - const mbgl::style::Image *image = map->getStyle().getImage(jni::Make(env, name)); - if (image) { - return Bitmap::CreateBitmap(env, image->getImage()); - } else { - return jni::Local>(); + if (auto* image = map->getStyle().getImage(jni::Make(env, name))) { + return Bitmap::CreateBitmap(env, *image); } + return jni::Local>(); } void NativeMapView::setPrefetchTiles(JNIEnv&, jni::jboolean enable) { diff --git a/src/mbgl/map/map_impl.cpp b/src/mbgl/map/map_impl.cpp index bc2a37fe07..e4eb2e3e8c 100644 --- 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 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 index d4b1cade13..56f7abd50b 100644 --- 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>&& result) { +void SpriteLoader::onParsed(std::vector> result) { observer->onSpriteLoaded(std::move(result)); } diff --git a/src/mbgl/sprite/sprite_loader.hpp b/src/mbgl/sprite/sprite_loader.hpp index 0b7d37fa14..d82c51e2da 100644 --- a/src/mbgl/sprite/sprite_loader.hpp +++ b/src/mbgl/sprite/sprite_loader.hpp @@ -1,6 +1,5 @@ #pragma once -#include #include #include @@ -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 @@ private: // Invoked by SpriteAtlasWorker friend class SpriteLoaderWorker; - void onParsed(std::vector>&&); + void onParsed(std::vector>); 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 index c730549c2c..3846d069f0 100644 --- a/src/mbgl/sprite/sprite_loader_observer.hpp +++ b/src/mbgl/sprite/sprite_loader_observer.hpp @@ -1,7 +1,9 @@ #pragma once +#include +#include + #include -#include #include namespace mbgl { @@ -13,8 +15,7 @@ class Image; class SpriteLoaderObserver { public: virtual ~SpriteLoaderObserver() = default; - - virtual void onSpriteLoaded(std::vector>&&) {} + virtual void onSpriteLoaded(std::vector>) {} virtual void onSpriteError(std::exception_ptr) {} }; diff --git a/src/mbgl/sprite/sprite_parser.cpp b/src/mbgl/sprite/sprite_parser.cpp index d39428a848..40e63d2d7d 100644 --- a/src/mbgl/sprite/sprite_parser.cpp +++ b/src/mbgl/sprite/sprite_parser.cpp @@ -149,50 +149,54 @@ optional getContent(const JSValue& value, const char* prope } // namespace -std::vector> parseSprite(const std::string& encodedImage, const std::string& json) { +std::vector> 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> 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 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> 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 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 index 9656bcbf2a..f2e7358ac2 100644 --- a/src/mbgl/sprite/sprite_parser.hpp +++ b/src/mbgl/sprite/sprite_parser.hpp @@ -20,6 +20,6 @@ std::unique_ptr createStyleImage(const std::string& id, optional content = nullopt); // Parses an image and an associated JSON file and returns the sprite objects. -std::vector> parseSprite(const std::string& image, const std::string& json); +std::vector> 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 index 8a821e5a5e..1814ba8adb 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -1,9 +1,10 @@ -#include -#include -#include #include -#include +#include #include +#include +#include +#include +#include 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) { diff --git a/src/mbgl/style/style_impl.cpp b/src/mbgl/style/style_impl.cpp index 5b52f1d72c..2d4376073d 100644 --- 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(); transitionOptions = parser.transition; @@ -274,17 +274,37 @@ bool Style::Impl::isLoaded() const { } void Style::Impl::addImage(std::unique_ptr image) { - images.remove(image->getID()); // We permit using addImage to update. - images.add(std::move(image)); + auto newImages = makeMutable(*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(*images); + auto found = + std::find_if(newImages->begin(), newImages->end(), [&id](const auto& image) { return image->id == id; }); + if (found == images->end()) { + Log::Warning(Event::General, "Image '%s' is not present in style, cannot remove", id.c_str()); + return; + } + newImages->erase(found); + 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 +339,12 @@ void Style::Impl::onSourceDescriptionChanged(Source& source) { } } -void Style::Impl::onSpriteLoaded(std::vector>&& images_) { - for (auto& image : images_) { - addImage(std::move(image)); - } +void Style::Impl::onSpriteLoaded(std::vector> images_) { + auto newImages = makeMutable(*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 +379,7 @@ const std::string& Style::Impl::getGlyphURL() const { } Immutable>> Style::Impl::getImageImpls() const { - return images.getImpls(); + return images; } Immutable>> Style::Impl::getSourceImpls() const { diff --git a/src/mbgl/style/style_impl.hpp b/src/mbgl/style/style_impl.hpp index ca165e24f0..f7dc7af293 100644 --- a/src/mbgl/style/style_impl.hpp +++ b/src/mbgl/style/style_impl.hpp @@ -78,13 +78,14 @@ public: void setLight(std::unique_ptr); 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); void removeImage(const std::string&); const std::string& getGlyphURL() const; - Immutable>> getImageImpls() const; + using ImageImpls = std::vector>; + Immutable getImageImpls() const; Immutable>> getSourceImpls() const; Immutable>> getLayerImpls() const; @@ -106,7 +107,7 @@ private: std::unique_ptr spriteLoader; std::string glyphURL; - CollectionWithPersistentOrder images; + Immutable images = makeMutable(); CollectionWithPersistentOrder sources; Collection layers; TransitionOptions transitionOptions; @@ -117,7 +118,7 @@ private: CameraOptions defaultCamera; // SpriteLoaderObserver implementation. - void onSpriteLoaded(std::vector>&&) override; + void onSpriteLoaded(std::vector>) 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 index a891f48b48..c3ab0d8227 100644 --- 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 index 350a64adb4..143ee928b5 100644 --- 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 index 122e711f51..4e68bff505 100644 --- a/test/sprite/sprite_loader.test.cpp +++ b/test/sprite/sprite_loader.test.cpp @@ -17,7 +17,7 @@ using namespace mbgl::style; class StubSpriteLoaderObserver : public SpriteLoaderObserver { public: - void onSpriteLoaded(std::vector>&& images) override { + void onSpriteLoaded(std::vector> images) override { if (spriteLoaded) spriteLoaded(std::move(images)); } @@ -25,7 +25,7 @@ public: if (spriteError) spriteError(error); } - std::function>&&)> spriteLoaded; + std::function>)> spriteLoaded; std::function spriteError; }; @@ -92,7 +92,7 @@ TEST(SpriteLoader, LoadingSuccess) { test.end(); }; - test.observer.spriteLoaded = [&] (std::vector>&& images) { + test.observer.spriteLoaded = [&](std::vector> images) { EXPECT_EQ(images.size(), 367u); test.end(); }; @@ -169,7 +169,7 @@ TEST(SpriteLoader, LoadingCancel) { return optional(); }; - test.observer.spriteLoaded = [&] (const std::vector>&) { + test.observer.spriteLoaded = [&](std::vector>) { FAIL() << "Should never be called"; }; diff --git a/test/sprite/sprite_parser.test.cpp b/test/sprite/sprite_parser.test.cpp index c37fee02b9..58979a0e61 100644 --- a/test/sprite/sprite_parser.test.cpp +++ b/test/sprite/sprite_parser.test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -242,8 +243,8 @@ TEST(Sprite, SpriteParsing) { const auto images = parseSprite(image_1x, json_1x); std::set names; - std::transform(images.begin(), images.end(), std::inserter(names, names.begin()), - [](const auto& image) { return image->getID(); }); + std::transform( + images.begin(), images.end(), std::inserter(names, names.begin()), [](const auto& image) { return image->id; }); EXPECT_EQ(std::set({ "airfield_icon", "airport_icon", @@ -321,11 +322,12 @@ TEST(Sprite, SpriteParsing) { names); { - auto& sprite = *std::find_if(images.begin(), images.end(), [] (const auto& image) { return image->getID() == "generic-metro"; }); - EXPECT_EQ(18u, sprite->getImage().size.width); - EXPECT_EQ(18u, sprite->getImage().size.height); - EXPECT_EQ(1, sprite->getPixelRatio()); - EXPECT_EQ(readImage("test/fixtures/annotations/result-spriteparsing.png"), sprite->getImage()); + auto& sprite = + *std::find_if(images.begin(), images.end(), [](const auto& image) { return image->id == "generic-metro"; }); + EXPECT_EQ(18u, sprite->image.size.width); + EXPECT_EQ(18u, sprite->image.size.height); + EXPECT_EQ(1, sprite->pixelRatio); + EXPECT_EQ(readImage("test/fixtures/annotations/result-spriteparsing.png"), sprite->image); } } @@ -460,10 +462,10 @@ TEST(Sprite, SpriteParsingStretchAndContent) { } })JSON"); EXPECT_EQ(1u, images.size()); - EXPECT_EQ("image", images[0]->getID()); - EXPECT_EQ((style::ImageStretches{{2, 14}}), images[0]->getStretchX()); - EXPECT_EQ((style::ImageStretches{{0, 4}, {12, 16}}), images[0]->getStretchY()); - EXPECT_EQ((style::ImageContent{2, 2, 14, 14}), images[0]->getContent()); + EXPECT_EQ("image", images[0]->id); + EXPECT_EQ((style::ImageStretches{{2, 14}}), images[0]->stretchX); + EXPECT_EQ((style::ImageStretches{{0, 4}, {12, 16}}), images[0]->stretchY); + EXPECT_EQ((style::ImageContent{2, 2, 14, 14}), images[0]->content); } TEST(Sprite, SpriteParsingEmptyImage) { -- cgit v1.2.1