From a53c4593d64bbe23a34b0ade142c94d09f64b02c Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 9 May 2019 10:49:10 +0300 Subject: [core] Schedule invocation of onStyleImageMissing completion callback on the same thread Before this change, ImageManger's 'done' callback for onStyleImageMissing observer notification that was created on renderer thread, could be called from different thread, therefore, is not thread safe. For example, on Android platform, callback was invoked from UI thread. This change makes callback to be scheduled on originating thread. --- src/mbgl/renderer/image_manager.cpp | 28 ++++++++++++++++++++-------- src/mbgl/renderer/image_manager.hpp | 7 ++++++- test/renderer/image_manager.test.cpp | 22 ++++++++++++++++++++++ 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index 8e584ffd34..860de487e7 100644 --- a/src/mbgl/renderer/image_manager.cpp +++ b/src/mbgl/renderer/image_manager.cpp @@ -1,4 +1,6 @@ #include +#include +#include #include #include #include @@ -128,7 +130,7 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) { void ImageManager::notifyIfMissingImageAdded() { for (auto it = missingImageRequestors.begin(); it != missingImageRequestors.end();) { - if (it->second.callbacksRemaining == 0) { + if (it->second.callbacks.empty()) { notify(*it->first, it->second.pair); it = missingImageRequestors.erase(it); } else { @@ -162,19 +164,29 @@ void ImageManager::checkMissingAndNotify(ImageRequestor& requestor, const ImageR if (missing > 0) { ImageRequestor* requestorPtr = &requestor; - missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair { std::move(pair), missing }); + auto emplaced = missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair { pair, {} }); + assert(emplaced.second); for (const auto& dependency : pair.first) { auto it = images.find(dependency.first); if (it == images.end()) { assert(observer != nullptr); - observer->onStyleImageMissing(dependency.first, [this, requestorPtr]() { - auto requestorIt = missingImageRequestors.find(requestorPtr); - if (requestorIt != missingImageRequestors.end()) { - assert(requestorIt->second.callbacksRemaining > 0); - requestorIt->second.callbacksRemaining--; - } + auto callback = std::make_unique( + *Scheduler::GetCurrent(), + [this, requestorPtr, imageId = dependency.first] { + auto requestorIt = missingImageRequestors.find(requestorPtr); + if (requestorIt != missingImageRequestors.end()) { + assert(requestorIt->second.callbacks.find(imageId) != requestorIt->second.callbacks.end()); + requestorIt->second.callbacks.erase(imageId); + } + }); + + auto actorRef = callback->self(); + emplaced.first->second.callbacks.emplace(dependency.first, std::move(callback)); + observer->onStyleImageMissing(dependency.first, [actorRef]() mutable { + actorRef.invoke(&Callback::operator()); }); + } } diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp index 61f3f3c276..c2d6466b3a 100644 --- a/src/mbgl/renderer/image_manager.hpp +++ b/src/mbgl/renderer/image_manager.hpp @@ -15,6 +15,9 @@ namespace mbgl { +template +class Actor; + namespace gfx { class Context; } // namespace gfx @@ -64,9 +67,11 @@ private: bool loaded = false; std::map requestors; + using Callback = std::function; + using ActorCallback = Actor; struct MissingImageRequestPair { ImageRequestPair pair; - unsigned int callbacksRemaining; + std::map> callbacks; }; std::map missingImageRequestors; std::map> requestedImages; diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index a5fffce479..7f0d670846 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -119,6 +119,7 @@ public: }; TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor(imageManager); bool notified = false; @@ -134,11 +135,15 @@ TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { ImageDependencies dependencies; dependencies.emplace("one", ImageType::Icon); imageManager.getImages(requestor, std::make_pair(dependencies, imageCorrelationID)); + runLoop.runOnce(); + ASSERT_FALSE(notified); imageManager.setLoaded(true); + runLoop.runOnce(); ASSERT_FALSE(notified); imageManager.notifyIfMissingImageAdded(); + runLoop.runOnce(); ASSERT_TRUE(notified); } @@ -173,6 +178,7 @@ class StubImageManagerObserver : public ImageManagerObserver { }; TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor(imageManager); StubImageManagerObserver observer; @@ -189,16 +195,19 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { ImageDependencies dependencies; dependencies.emplace("pre", ImageType::Icon); imageManager.getImages(requestor, std::make_pair(dependencies, imageCorrelationID)); + runLoop.runOnce(); EXPECT_EQ(observer.count, 0); ASSERT_FALSE(notified); imageManager.setLoaded(true); + runLoop.runOnce(); EXPECT_EQ(observer.count, 1); ASSERT_FALSE(notified); imageManager.notifyIfMissingImageAdded(); + runLoop.runOnce(); EXPECT_EQ(observer.count, 1); ASSERT_TRUE(notified); @@ -206,6 +215,7 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { } TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor(imageManager); StubImageManagerObserver observer; @@ -222,22 +232,26 @@ TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { ASSERT_FALSE(notified); imageManager.setLoaded(true); + runLoop.runOnce(); uint64_t imageCorrelationID = 0; ImageDependencies dependencies; dependencies.emplace("after", ImageType::Icon); imageManager.getImages(requestor, std::make_pair(dependencies, imageCorrelationID)); + runLoop.runOnce(); EXPECT_EQ(observer.count, 1); ASSERT_FALSE(notified); imageManager.notifyIfMissingImageAdded(); + runLoop.runOnce(); EXPECT_EQ(observer.count, 1); ASSERT_TRUE(notified); } TEST(ImageManager, ReduceMemoryUsage) { + util::RunLoop runLoop; ImageManager imageManager; StubImageManagerObserver observer; @@ -247,17 +261,20 @@ TEST(ImageManager, ReduceMemoryUsage) { imageManager.setObserver(&observer); imageManager.setLoaded(true); + runLoop.runOnce(); // Single requestor { std::unique_ptr requestor = std::make_unique(imageManager); imageManager.getImages(*requestor, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 0ull)); + runLoop.runOnce(); EXPECT_EQ(observer.count, 1); ASSERT_FALSE(imageManager.getImage("missing") == nullptr); } // Reduce memory usage and check that unused image was deleted. imageManager.reduceMemoryUse(); + runLoop.runOnce(); ASSERT_TRUE(imageManager.getImage("missing") == nullptr); // Multiple requestors @@ -266,12 +283,14 @@ TEST(ImageManager, ReduceMemoryUsage) { std::unique_ptr requestor2 = std::make_unique(imageManager); imageManager.getImages(*requestor1, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 0ull)); imageManager.getImages(*requestor2, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 1ull)); + runLoop.runOnce(); EXPECT_EQ(observer.count, 2); ASSERT_FALSE(imageManager.getImage("missing") == nullptr); } // Reduce memory usage and check that unused image was deleted when all requestors are destructed. imageManager.reduceMemoryUse(); + runLoop.runOnce(); ASSERT_TRUE(imageManager.getImage("missing") == nullptr); // Multiple requestors, check that image resource is not destroyed if there is at least 1 requestor that uses it. @@ -280,16 +299,19 @@ TEST(ImageManager, ReduceMemoryUsage) { std::unique_ptr requestor1 = std::make_unique(imageManager); imageManager.getImages(*requestor, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 0ull)); imageManager.getImages(*requestor1, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 1ull)); + runLoop.runOnce(); EXPECT_EQ(observer.count, 3); ASSERT_FALSE(imageManager.getImage("missing") == nullptr); } // Reduce memory usage and check that requested image is not destructed. imageManager.reduceMemoryUse(); + runLoop.runOnce(); ASSERT_FALSE(imageManager.getImage("missing") == nullptr); // Release last requestor and check if resource was released after reduceMemoryUse(). requestor.reset(); imageManager.reduceMemoryUse(); + runLoop.runOnce(); ASSERT_TRUE(imageManager.getImage("missing") == nullptr); } -- cgit v1.2.1