Hold internal reference to Sprite in SpriteAtlas

6 files changed, 40 insertions, 51 deletions

diff --git a/include/mbgl/geometry/sprite_atlas.hpp b/include/mbgl/geometry/sprite_atlas.hpp
index 0946f0fc48..9fb42a30b5 100644
--- a/include/mbgl/geometry/sprite_atlas.hpp
+++ b/include/mbgl/geometry/sprite_atlas.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/geometry/binpack.hpp>
 #include <mbgl/util/noncopyable.hpp>
+#include <mbgl/util/ptr.hpp>
 
 #include <string>
 #include <map>
@@ -26,7 +27,6 @@ class SpriteAtlas : public util::noncopyable {
 public:
     typedef uint16_t dimension;
 
-public:
     // Add way to construct this from another SpriteAtlas (e.g. with another pixelRatio)
     SpriteAtlas(dimension width, dimension height);
     ~SpriteAtlas();
@@ -34,22 +34,16 @@ public:
     // Changes the pixel ratio.
     bool resize(float newRatio);
 
-    // Update uninitialized (= outdated) sprites in this atlas from the given sprite.
-    void update(const Sprite &sprite);
+    // Changes the source sprite.
+    void setSprite(util::ptr<Sprite> sprite);
 
     // Returns the coordinates of an image that is sourced from the sprite image.
     // This getter attempts to read the image from the sprite if it is already loaded.
     // In that case, it copies it into the sprite atlas and returns the dimensions.
     // Otherwise, it returns a 0/0/0/0 rect.
-    Rect<dimension> getImage(const std::string &name, const Sprite &sprite);
-
-    // Returns the coordinates of an image that is sourced from the sprite image.
-    // This getter waits until the sprite image was loaded, copies it into the sprite
-    // image and returns the dimensions.
-    // NEVER CALL THIS FUNCTION FROM THE RENDER THREAD! it is blocking.
-    Rect<dimension> waitForImage(const std::string &name, const Sprite &sprite);
+    Rect<dimension> getImage(const std::string& name);
 
-    SpriteAtlasPosition getPosition(const std::string &name, const Sprite &sprite, bool repeating = false);
+    SpriteAtlasPosition getPosition(const std::string& name, bool repeating = false);
 
     // Binds the image buffer of this sprite atlas to the GPU, and uploads data if it is out
     // of date.
@@ -61,19 +55,18 @@ public:
     inline float getTextureHeight() const { return height * pixelRatio; }
     inline float getPixelRatio() const { return pixelRatio; }
 
-private:
-    void allocate();
-    Rect<SpriteAtlas::dimension> allocateImage(size_t width, size_t height);
-    void copy(const Rect<dimension> &dst, const SpritePosition &src, const Sprite &sprite);
-
-public:
     const dimension width = 0;
     const dimension height = 0;
 
 private:
-    std::mutex mtx;
+    void allocate();
+    Rect<SpriteAtlas::dimension> allocateImage(size_t width, size_t height);
+    void copy(const Rect<dimension>& dst, const SpritePosition& src);
+
+    std::recursive_mutex mtx;
     float pixelRatio = 1.0f;
     BinPack<dimension> bin;
+    util::ptr<Sprite> sprite;
     std::map<std::string, Rect<dimension>> images;
     std::set<std::string> uninitialized;
     uint32_t *data = nullptr;
diff --git a/src/geometry/sprite_atlas.cpp b/src/geometry/sprite_atlas.cpp
index 22e6a29a0b..5271122919 100644
--- a/src/geometry/sprite_atlas.cpp
+++ b/src/geometry/sprite_atlas.cpp
@@ -24,7 +24,7 @@ SpriteAtlas::SpriteAtlas(dimension width_, dimension height_)
 bool SpriteAtlas::resize(const float newRatio) {
     if (pixelRatio == newRatio) return false;
 
-    std::lock_guard<std::mutex> lock(mtx);
+    std::lock_guard<std::recursive_mutex> lock(mtx);
 
     const float oldRatio = pixelRatio;
     pixelRatio = newRatio;
@@ -104,15 +104,15 @@ Rect<SpriteAtlas::dimension> SpriteAtlas::allocateImage(size_t pixel_width, size
     return rect;
 }
 
-Rect<SpriteAtlas::dimension> SpriteAtlas::getImage(const std::string &name, const Sprite &sprite) {
-    std::lock_guard<std::mutex> lock(mtx);
+Rect<SpriteAtlas::dimension> SpriteAtlas::getImage(const std::string& name) {
+    std::lock_guard<std::recursive_mutex> lock(mtx);
 
     auto rect_it = images.find(name);
     if (rect_it != images.end()) {
         return rect_it->second;
     }
 
-    const SpritePosition &pos = sprite.getSpritePosition(name);
+    const SpritePosition &pos = sprite->getSpritePosition(name);
     if (!pos.width || !pos.height) {
         return Rect<dimension> { 0, 0, 0, 0 };
     }
@@ -127,16 +127,17 @@ Rect<SpriteAtlas::dimension> SpriteAtlas::getImage(const std::string &name, cons
 
     images.emplace(name, rect);
 
-    copy(rect, pos, sprite);
+    copy(rect, pos);
 
     return rect;
 }
 
-SpriteAtlasPosition SpriteAtlas::getPosition(const std::string& name, const Sprite& sprite, bool repeating) {
+SpriteAtlasPosition SpriteAtlas::getPosition(const std::string& name, bool repeating) {
+    std::lock_guard<std::recursive_mutex> lock(mtx);
     // `repeating` indicates that the image will be used in a repeating pattern
     // repeating pattern images are assumed to have a 1px padding that mirrors the opposite edge
     // positions for repeating images are adjusted to exclude the edge
-    Rect<dimension> rect = getImage(name, sprite);
+    Rect<dimension> rect = getImage(name);
     const int r = repeating ? 1 : 0;
     return SpriteAtlasPosition {
         {{ float(rect.w)           / pixelRatio, float(rect.h)            / pixelRatio }},
@@ -145,11 +146,6 @@ SpriteAtlasPosition SpriteAtlas::getPosition(const std::string& name, const Spri
     };
 }
 
-Rect<SpriteAtlas::dimension> SpriteAtlas::waitForImage(const std::string &name, const Sprite &sprite) {
-    sprite.waitUntilLoaded();
-    return getImage(name, sprite);
-}
-
 void SpriteAtlas::allocate() {
     if (!data) {
         dimension w = static_cast<dimension>(width * pixelRatio);
@@ -159,15 +155,15 @@ void SpriteAtlas::allocate() {
     }
 }
 
-void SpriteAtlas::copy(const Rect<dimension> &dst, const SpritePosition &src, const Sprite &sprite) {
-    if (!sprite.raster) return;
-    const uint32_t *src_img = reinterpret_cast<const uint32_t *>(sprite.raster->getData());
+void SpriteAtlas::copy(const Rect<dimension>& dst, const SpritePosition& src) {
+    if (!sprite->raster) return;
+    const uint32_t *src_img = reinterpret_cast<const uint32_t *>(sprite->raster->getData());
     allocate();
     uint32_t *dst_img = reinterpret_cast<uint32_t *>(data);
 
     copy_bitmap(
         /* source buffer */  src_img,
-        /* source stride */  sprite.raster->getWidth(),
+        /* source stride */  sprite->raster->getWidth(),
         /* source x */       src.x,
         /* source y */       src.y,
         /* dest buffer */    dst_img,
@@ -181,14 +177,16 @@ void SpriteAtlas::copy(const Rect<dimension> &dst, const SpritePosition &src, co
     dirty = true;
 }
 
+void SpriteAtlas::setSprite(util::ptr<Sprite> sprite_) {
+    std::lock_guard<std::recursive_mutex> lock(mtx);
+
+    sprite = sprite_;
 
-void SpriteAtlas::update(const Sprite &sprite) {
-    if (!sprite.isLoaded()) return;
+    if (!sprite->isLoaded()) return;
 
-    SpriteAtlas &atlas = *this;
-    util::erase_if(uninitialized, [&sprite, &atlas](const std::string &name) {
-        Rect<dimension> dst = atlas.getImage(name, sprite);
-        const SpritePosition &src = sprite.getSpritePosition(name);
+    util::erase_if(uninitialized, [this](const std::string &name) {
+        Rect<dimension> dst = getImage(name);
+        const SpritePosition& src = sprite->getSpritePosition(name);
         if (!src) {
             if (debug::spriteWarnings) {
                 fprintf(stderr, "[WARNING] sprite doesn't have image with name '%s'\n", name.c_str());
@@ -196,8 +194,8 @@ void SpriteAtlas::update(const Sprite &sprite) {
             return true;
         }
 
-        if (src.width == dst.w * atlas.pixelRatio && src.height == dst.h * atlas.pixelRatio && src.pixelRatio == atlas.pixelRatio) {
-            atlas.copy(dst, src, sprite);
+        if (src.width == dst.w * pixelRatio && src.height == dst.h * pixelRatio && src.pixelRatio == pixelRatio) {
+            copy(dst, src);
             return true;
         } else {
             if (debug::spriteWarnings) {
@@ -228,7 +226,7 @@ void SpriteAtlas::bind(bool linear) {
     }
 
     if (dirty) {
-        std::lock_guard<std::mutex> lock(mtx);
+        std::lock_guard<std::recursive_mutex> lock(mtx);
         allocate();
 
         if (first) {
@@ -257,15 +255,12 @@ void SpriteAtlas::bind(bool linear) {
             );
         }
 
-#if defined(DEBUG)
-        // platform::show_color_debug_image("Sprite Atlas", reinterpret_cast<char *>(data), width, height, width * pixelRatio, height * pixelRatio);
-#endif
         dirty = false;
     }
 };
 
 SpriteAtlas::~SpriteAtlas() {
-    std::lock_guard<std::mutex> lock(mtx);
+    std::lock_guard<std::recursive_mutex> lock(mtx);
 
     glDeleteTextures(1, &texture);
     texture = 0;
diff --git a/src/map/map.cpp b/src/map/map.cpp
index 2f0dceb427..82108d0006 100644
--- a/src/map/map.cpp
+++ b/src/map/map.cpp
@@ -646,7 +646,7 @@ void Map::prepare() {
 
     // Allow the sprite atlas to potentially pull new sprite images if needed.
     spriteAtlas->resize(state.getPixelRatio());
-    spriteAtlas->update(*getSprite());
+    spriteAtlas->setSprite(getSprite());
 
     updateTiles();
 }
diff --git a/src/renderer/painter.cpp b/src/renderer/painter.cpp
index 38c2176aed..a5f4a5b009 100644
--- a/src/renderer/painter.cpp
+++ b/src/renderer/painter.cpp
@@ -218,7 +218,7 @@ void Painter::renderBackground(util::ptr<StyleLayer> layer_desc) {
             return;
 
         SpriteAtlas &spriteAtlas = *map.getSpriteAtlas();
-        SpriteAtlasPosition imagePos = spriteAtlas.getPosition(properties.image, *sprite, true);
+        SpriteAtlasPosition imagePos = spriteAtlas.getPosition(properties.image, true);
         float zoomFraction = map.getState().getZoomFraction();
 
         useProgram(patternShader->program);
diff --git a/src/renderer/painter_fill.cpp b/src/renderer/painter_fill.cpp
index 6c17ab037a..0f7eadce63 100644
--- a/src/renderer/painter_fill.cpp
+++ b/src/renderer/painter_fill.cpp
@@ -61,7 +61,7 @@ void Painter::renderFill(FillBucket& bucket, util::ptr<StyleLayer> layer_desc, c
         Sprite &sprite = *map.getSprite();
         if (pass == RenderPass::Translucent && sprite) {
             SpriteAtlas &spriteAtlas = *map.getSpriteAtlas();
-            const SpriteAtlasPosition pos = spriteAtlas.getPosition(properties.image, sprite, true);
+            const SpriteAtlasPosition pos = spriteAtlas.getPosition(properties.image, true);
             const float mix = std::fmod(float(map.getState().getZoom()), 1.0f);
             const float factor = 8.0 / std::pow(2, map.getState().getIntegerZoom() - id.z);
 
diff --git a/src/renderer/symbol_bucket.cpp b/src/renderer/symbol_bucket.cpp
index 7d5036eea3..1fe91bffee 100644
--- a/src/renderer/symbol_bucket.cpp
+++ b/src/renderer/symbol_bucket.cpp
@@ -148,7 +148,8 @@ void SymbolBucket::addFeatures(const VectorTileLayer &layer, const FilterExpress
 
         // if feature has icon, get sprite atlas position
         if (feature.sprite.length()) {
-            image = spriteAtlas.waitForImage(feature.sprite, sprite);
+            sprite.waitUntilLoaded();
+            image = spriteAtlas.getImage(feature.sprite);
 
             if (sprite.getSpritePosition(feature.sprite).sdf) {
                 sdfIcons = true;