From 5dcc9c700d8ebdf6ba4381af1ef03b6164b739dd Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 9 May 2019 10:49:10 +0300 Subject: [PATCH 1/3] [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 | 13 +++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index 25acd6bd251..2ea47225f80 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 @@ -116,7 +118,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); missingImageRequestors.erase(it++); } else { @@ -137,19 +139,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 99887ae3843..bec811317ee 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 @@ -66,9 +69,11 @@ class ImageManager : public util::noncopyable { 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; ImageMap images; diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index d124e67e104..b9ec099f93d 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -117,6 +117,7 @@ class StubImageRequestor : public ImageRequestor { }; TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor; bool notified = false; @@ -132,11 +133,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); } @@ -169,6 +174,7 @@ class StubImageManagerObserver : public ImageManagerObserver { }; TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor; StubImageManagerObserver observer; @@ -185,16 +191,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); @@ -202,6 +211,7 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { } TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { + util::RunLoop runLoop; ImageManager imageManager; StubImageRequestor requestor; StubImageManagerObserver observer; @@ -218,16 +228,19 @@ 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); From 89bb1289657fa9d076e7ea93d07f6ea496624b8a Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Tue, 7 May 2019 18:49:47 +0300 Subject: [PATCH 2/3] [core] Failed sprite requests do not block tiles rendering --- src/mbgl/style/style_impl.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/mbgl/style/style_impl.cpp b/src/mbgl/style/style_impl.cpp index fde5aa632de..d527b8440b9 100644 --- a/src/mbgl/style/style_impl.cpp +++ b/src/mbgl/style/style_impl.cpp @@ -309,6 +309,9 @@ void Style::Impl::onSpriteError(std::exception_ptr error) { lastError = error; Log::Error(Event::Style, "Failed to load sprite: %s", util::toString(error).c_str()); observer->onResourceError(error); + // Unblock rendering tiles (even though sprite request has failed). + spriteLoaded = true; + observer->onUpdate(); } void Style::Impl::onLayerChanged(Layer& layer) { From fd47872409de03ee98e96f883d62972adfac0feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Paczos?= Date: Tue, 7 May 2019 11:04:50 +0200 Subject: [PATCH 3/3] [android] remove missing image log --- .../src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java | 1 - 1 file changed, 1 deletion(-) diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java index 249a5f8d7b9..8d75d3e76f7 100755 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java @@ -1045,7 +1045,6 @@ private void onSourceChanged(String sourceId) { @Keep private void onStyleImageMissing(String imageId) { - Logger.e(TAG, "OnStyleImageMissing: " + imageId); if (stateCallback != null) { stateCallback.onStyleImageMissing(imageId); }