From 11463b796b37183cdfb528f2492537613bf86511 Mon Sep 17 00:00:00 2001 From: Bruno de Oliveira Abinader Date: Fri, 25 Aug 2017 11:47:34 +0300 Subject: [core] Added GeometryTileWorker-owned image correlation ID --- src/mbgl/renderer/image_manager.cpp | 14 +++++++------- src/mbgl/renderer/image_manager.hpp | 8 ++++---- src/mbgl/style/image_impl.hpp | 1 + src/mbgl/tile/geometry_tile.cpp | 24 ++++++++++++++++++++---- src/mbgl/tile/geometry_tile.hpp | 4 ++-- src/mbgl/tile/geometry_tile_worker.cpp | 7 +++++-- src/mbgl/tile/geometry_tile_worker.hpp | 3 ++- test/renderer/image_manager.test.cpp | 11 +++++++---- 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); 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 requestors; + std::unordered_map 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>; using ImageDependencies = std::set; +using ImageRequestPair = std::pair; } // 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>&) 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>> 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 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 {"one"}, imageCorrelationID)); ASSERT_FALSE(notified); imageManager.setLoaded(true); @@ -140,8 +142,9 @@ TEST(ImageManager, NotifiesRequestorImmediatelyIfDependenciesAreSatisfied) { notified = true; }; + uint64_t imageCorrelationID = 0; imageManager.addImage(makeMutable("one", PremultipliedImage({ 16, 16 }), 2)); - imageManager.getImages(requestor, {"one"}); + imageManager.getImages(requestor, std::make_pair(std::set {"one"}, imageCorrelationID)); ASSERT_TRUE(notified); } -- cgit v1.2.1