[core] Added GeometryTileWorker-owned image correlation IDupstream/revert-9739

8 files changed, 48 insertions, 24 deletions

diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp
index 692747bca4..365a2742de 100644
--- a/src/mbgl/renderer/image_manager.cpp
+++ b/src/mbgl/renderer/image_manager.cpp
@@ -52,23 +52,23 @@ const style::Image::Impl* ImageManager::getImage(const std::string& id) const {
     return nullptr;
 }
 
-void ImageManager::getImages(ImageRequestor& requestor, ImageDependencies dependencies) {
+void ImageManager::getImages(ImageRequestor& requestor, ImageRequestPair&& pair) {
     // If the sprite has been loaded, or if all the icon dependencies are already present
     // (i.e. if they've been addeded via runtime styling), then notify the requestor immediately.
     // Otherwise, delay notification until the sprite is loaded. At that point, if any of the
     // dependencies are still unavailable, we'll just assume they are permanently missing.
     bool hasAllDependencies = true;
     if (!isLoaded()) {
-        for (const auto& dependency : dependencies) {
+        for (const auto& dependency : pair.first) {
             if (images.find(dependency) == images.end()) {
                 hasAllDependencies = false;
             }
         }
     }
     if (isLoaded() || hasAllDependencies) {
-        notify(requestor, dependencies);
+        notify(requestor, std::move(pair));
     } else {
-        requestors.emplace(&requestor, std::move(dependencies));
+        requestors.emplace(&requestor, std::move(pair));
     }
 }
 
@@ -76,17 +76,17 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) {
     requestors.erase(&requestor);
 }
 
-void ImageManager::notify(ImageRequestor& requestor, const ImageDependencies& dependencies) const {
+void ImageManager::notify(ImageRequestor& requestor, const ImageRequestPair& pair) const {
     ImageMap response;
 
-    for (const auto& dependency : dependencies) {
+    for (const auto& dependency : pair.first) {
         auto it = images.find(dependency);
         if (it != images.end()) {
             response.emplace(*it);
         }
     }
 
-    requestor.onImagesAvailable(response);
+    requestor.onImagesAvailable(response, pair.second);
 }
 
 void ImageManager::dumpDebugLogs() const {
diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp
index 1c9d67f47d..f72ba9fb53 100644
--- a/src/mbgl/renderer/image_manager.hpp
+++ b/src/mbgl/renderer/image_manager.hpp
@@ -21,7 +21,7 @@ class Context;
 class ImageRequestor {
 public:
     virtual ~ImageRequestor() = default;
-    virtual void onImagesAvailable(ImageMap) = 0;
+    virtual void onImagesAvailable(ImageMap, uint64_t imageCorrelationID) = 0;
 };
 
 /*
@@ -50,15 +50,15 @@ public:
     void updateImage(Immutable<style::Image::Impl>);
     void removeImage(const std::string&);
 
-    void getImages(ImageRequestor&, ImageDependencies);
+    void getImages(ImageRequestor&, ImageRequestPair&&);
     void removeRequestor(ImageRequestor&);
 
 private:
-    void notify(ImageRequestor&, const ImageDependencies&) const;
+    void notify(ImageRequestor&, const ImageRequestPair&) const;
 
     bool loaded = false;
 
-    std::unordered_map<ImageRequestor*, ImageDependencies> requestors;
+    std::unordered_map<ImageRequestor*, ImageRequestPair> requestors;
     ImageMap images;
 
 // Pattern stuff
diff --git a/src/mbgl/style/image_impl.hpp b/src/mbgl/style/image_impl.hpp
index 75dc83206c..e439e42695 100644
--- a/src/mbgl/style/image_impl.hpp
+++ b/src/mbgl/style/image_impl.hpp
@@ -28,5 +28,6 @@ public:
 
 using ImageMap = std::unordered_map<std::string, Immutable<style::Image::Impl>>;
 using ImageDependencies = std::set<std::string>;
+using ImageRequestPair = std::pair<ImageDependencies, uint64_t>;
 
 } // namespace mbgl
diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index a3ccd86479..017861884b 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -28,6 +28,22 @@ namespace mbgl {
 
 using namespace style;
 
+/*
+   Correlation between GeometryTile and GeometryTileWorker is safeguarded by two
+   correlation schemes:
+
+   GeometryTile's 'correlationID' is used for ensuring the tile will be flagged
+   as non-pending only when the placement coming from the last operation (as in
+   'setData', 'setLayers',  'setPlacementConfig') occurs. This is important for
+   still mode rendering as we want to render only when all layout and placement
+   operations are completed.
+
+   GeometryTileWorker's 'imageCorrelationID' is used for checking whether an
+   image request reply coming from `GeometryTile` is valid. Previous image
+   request replies are ignored as they result in incomplete placement attempts
+   that could flag the tile as non-pending too early.
+ */
+
 GeometryTile::GeometryTile(const OverscaledTileID& id_,
                            std::string sourceID_,
                            const TileParameters& parameters)
@@ -170,12 +186,12 @@ void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) {
     glyphManager.getGlyphs(*this, std::move(glyphDependencies));
 }
 
-void GeometryTile::onImagesAvailable(ImageMap images) {
-    worker.invoke(&GeometryTileWorker::onImagesAvailable, std::move(images));
+void GeometryTile::onImagesAvailable(ImageMap images, uint64_t imageCorrelationID) {
+    worker.invoke(&GeometryTileWorker::onImagesAvailable, std::move(images), imageCorrelationID);
 }
 
-void GeometryTile::getImages(ImageDependencies imageDependencies) {
-    imageManager.getImages(*this, std::move(imageDependencies));
+void GeometryTile::getImages(ImageRequestPair pair) {
+    imageManager.getImages(*this, std::move(pair));
 }
 
 void GeometryTile::upload(gl::Context& context) {
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index 7d70e1cb9a..ebad0f5410 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -40,10 +40,10 @@ public:
     void setLayers(const std::vector<Immutable<style::Layer::Impl>>&) override;
     
     void onGlyphsAvailable(GlyphMap) override;
-    void onImagesAvailable(ImageMap) override;
+    void onImagesAvailable(ImageMap, uint64_t imageCorrelationID) override;
     
     void getGlyphs(GlyphDependencies);
-    void getImages(ImageDependencies);
+    void getImages(ImageRequestPair);
 
     void upload(gl::Context&) override;
     Bucket* getBucket(const style::Layer::Impl&) const override;
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index c622d82e31..4af35e2326 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -216,7 +216,10 @@ void GeometryTileWorker::onGlyphsAvailable(GlyphMap newGlyphMap) {
     symbolDependenciesChanged();
 }
 
-void GeometryTileWorker::onImagesAvailable(ImageMap newImageMap) {
+void GeometryTileWorker::onImagesAvailable(ImageMap newImageMap, uint64_t imageCorrelationID_) {
+    if (imageCorrelationID != imageCorrelationID_) {
+        return; // Ignore outdated image request replies.
+    }
     imageMap = std::move(newImageMap);
     pendingImageDependencies.clear();
     symbolDependenciesChanged();
@@ -239,7 +242,7 @@ void GeometryTileWorker::requestNewGlyphs(const GlyphDependencies& glyphDependen
 void GeometryTileWorker::requestNewImages(const ImageDependencies& imageDependencies) {
     pendingImageDependencies = imageDependencies;
     if (!pendingImageDependencies.empty()) {
-        parent.invoke(&GeometryTile::getImages, pendingImageDependencies);
+        parent.invoke(&GeometryTile::getImages, std::make_pair(pendingImageDependencies, ++imageCorrelationID));
     }
 }
 
diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp
index 7f80c3b4f7..1425daa7a1 100644
--- a/src/mbgl/tile/geometry_tile_worker.hpp
+++ b/src/mbgl/tile/geometry_tile_worker.hpp
@@ -38,7 +38,7 @@ public:
     void setPlacementConfig(PlacementConfig, uint64_t correlationID);
     
     void onGlyphsAvailable(GlyphMap glyphs);
-    void onImagesAvailable(ImageMap images);
+    void onImagesAvailable(ImageMap images, uint64_t imageCorrelationID);
 
 private:
     void coalesced();
@@ -70,6 +70,7 @@ private:
 
     State state = Idle;
     uint64_t correlationID = 0;
+    uint64_t imageCorrelationID = 0;
 
     // Outer optional indicates whether we've received it or not.
     optional<std::vector<Immutable<style::Layer::Impl>>> layers;
diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp
index 5e6da5c005..ebe1bcd72f 100644
--- a/test/renderer/image_manager.test.cpp
+++ b/test/renderer/image_manager.test.cpp
@@ -108,11 +108,12 @@ TEST(ImageManager, RemoveReleasesBinPackRect) {
 
 class StubImageRequestor : public ImageRequestor {
 public:
-    void onImagesAvailable(ImageMap images) final {
-        if (imagesAvailable) imagesAvailable(images);
+    void onImagesAvailable(ImageMap images, uint64_t imageCorrelationID_) final {
+        if (imagesAvailable && imageCorrelationID == imageCorrelationID_) imagesAvailable(images);
     }
 
     std::function<void (ImageMap)> imagesAvailable;
+    uint64_t imageCorrelationID = 0;
 };
 
 TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) {
@@ -124,7 +125,8 @@ TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) {
         notified = true;
     };
 
-    imageManager.getImages(requestor, {"one"});
+    uint64_t imageCorrelationID = 0;
+    imageManager.getImages(requestor, std::make_pair(std::set<std::string> {"one"}, imageCorrelationID));
     ASSERT_FALSE(notified);
 
     imageManager.setLoaded(true);
@@ -140,8 +142,9 @@ TEST(ImageManager, NotifiesRequestorImmediatelyIfDependenciesAreSatisfied) {
         notified = true;
     };
 
+    uint64_t imageCorrelationID = 0;
     imageManager.addImage(makeMutable<style::Image::Impl>("one", PremultipliedImage({ 16, 16 }), 2));
-    imageManager.getImages(requestor, {"one"});
+    imageManager.getImages(requestor, std::make_pair(std::set<std::string> {"one"}, imageCorrelationID));
 
     ASSERT_TRUE(notified);
 }