From aa36dc87fb5583425ab29b4a15407186c31c492f Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Thu, 24 Oct 2019 17:05:01 +0300 Subject: Revert "[core] Revert coalescing requests to the client for the same missing image" This reverts commit 22132683797b5ea5cbe8622f77c01b399ad9c71e. --- src/mbgl/renderer/image_manager.cpp | 70 +++++++++++++++++++++++------------- src/mbgl/renderer/image_manager.hpp | 17 +++++---- test/renderer/image_manager.test.cpp | 24 +++++++++++++ 3 files changed, 80 insertions(+), 31 deletions(-) diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index cc3e29cc79..7b51fb9a2e 100644 --- a/src/mbgl/renderer/image_manager.cpp +++ b/src/mbgl/renderer/image_manager.cpp @@ -138,8 +138,9 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) { void ImageManager::notifyIfMissingImageAdded() { for (auto it = missingImageRequestors.begin(); it != missingImageRequestors.end();) { - if (it->second.callbacks.empty()) { - notify(*it->first, it->second.pair); + ImageRequestor& requestor = *it->first; + if (!requestor.hasPendingRequests()) { + notify(requestor, it->second); it = missingImageRequestors.erase(it); } else { ++it; @@ -169,35 +170,56 @@ void ImageManager::reduceMemoryUseIfCacheSizeExceedsLimit() { } void ImageManager::checkMissingAndNotify(ImageRequestor& requestor, const ImageRequestPair& pair) { - std::vector missingImages; - missingImages.reserve(pair.first.size()); + ImageDependencies missingDependencies; + for (const auto& dependency : pair.first) { if (images.find(dependency.first) == images.end()) { - missingImages.push_back(dependency.first); + missingDependencies.emplace(dependency); } } - if (!missingImages.empty()) { + if (!missingDependencies.empty()) { ImageRequestor* requestorPtr = &requestor; + assert(!missingImageRequestors.count(requestorPtr)); + missingImageRequestors.emplace(requestorPtr, pair); - auto emplaced = missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair{pair, {}}); - assert(emplaced.second); - - for (const auto& missingImage : missingImages) { + for (const auto& dependency : missingDependencies) { + const std::string& missingImage = dependency.first; assert(observer != nullptr); - requestedImages[missingImage].emplace(&requestor); - auto callback = - std::make_unique(*Scheduler::GetCurrent(), [this, requestorPtr, missingImage] { - auto requestorIt = missingImageRequestors.find(requestorPtr); - if (requestorIt != missingImageRequestors.end()) { - assert(requestorIt->second.callbacks.find(missingImage) != requestorIt->second.callbacks.end()); - requestorIt->second.callbacks.erase(missingImage); - } - }); - - auto actorRef = callback->self(); - emplaced.first->second.callbacks.emplace(missingImage, std::move(callback)); - observer->onStyleImageMissing(missingImage, [actorRef] { actorRef.invoke(&Callback::operator()); }); + + auto existingRequestorsIt = requestedImages.find(missingImage); + if (existingRequestorsIt != requestedImages.end()) { // Already asked client about this image. + std::set& existingRequestors = existingRequestorsIt->second; + if (!existingRequestors.empty() && + (*existingRequestors.begin()) + ->hasPendingRequest(missingImage)) { // Still waiting for the client response for this image. + requestorPtr->addPendingRequest(missingImage); + existingRequestors.emplace(requestorPtr); + continue; + } + // Unlike icons, pattern changes are not caught + // with style-diff meaning that the existing request + // could be from the previous style and we cannot + // coalesce requests for them. + if (dependency.second != ImageType::Pattern) { + continue; + } + } + requestedImages[missingImage].emplace(requestorPtr); + requestor.addPendingRequest(missingImage); + + auto removePendingRequests = [this, missingImage] { + auto existingRequest = requestedImages.find(missingImage); + if (existingRequest == requestedImages.end()) { + return; + } + + for (auto* req : existingRequest->second) { + req->removePendingRequest(missingImage); + } + }; + observer->onStyleImageMissing(missingImage, + Scheduler::GetCurrent()->bindOnce(std::move(removePendingRequests))); } } else { // Associate requestor with an image that was provided by the client. @@ -227,7 +249,7 @@ void ImageManager::notify(ImageRequestor& requestor, const ImageRequestPair& pai } } - requestor.onImagesAvailable(iconMap, patternMap, std::move(versionMap), pair.second); + requestor.onImagesAvailable(std::move(iconMap), std::move(patternMap), std::move(versionMap), pair.second); } void ImageManager::dumpDebugLogs() const { diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp index 9097418681..5ed6e237f0 100644 --- a/src/mbgl/renderer/image_manager.hpp +++ b/src/mbgl/renderer/image_manager.hpp @@ -58,13 +58,7 @@ private: bool loaded = false; std::map requestors; - using Callback = std::function; - using ActorCallback = Actor; - struct MissingImageRequestPair { - ImageRequestPair pair; - std::map> callbacks; - }; - std::map missingImageRequestors; + std::map missingImageRequestors; std::map> requestedImages; std::size_t requestedImagesCacheSize = 0ul; ImageMap images; @@ -77,8 +71,17 @@ public: explicit ImageRequestor(ImageManager&); virtual ~ImageRequestor(); virtual void onImagesAvailable(ImageMap icons, ImageMap patterns, ImageVersionMap versionMap, uint64_t imageCorrelationID) = 0; + + void addPendingRequest(const std::string& imageId) { pendingRequests.insert(imageId); } + bool hasPendingRequest(const std::string& imageId) const { return pendingRequests.count(imageId); } + bool hasPendingRequests() const { return !pendingRequests.empty(); } + void removePendingRequest(const std::string& imageId) { pendingRequests.erase(imageId); } + private: ImageManager& imageManager; + + // Pending requests are image requests that are waiting to be dispatched to the client. + std::set pendingRequests; }; } // namespace mbgl diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index 16700d713f..20c0a3a7f3 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -187,6 +187,30 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { EXPECT_EQ(observer.count, 1); ASSERT_TRUE(notified); + // Repeated request of the same image shall not result another + // `ImageManagerObserver.onStyleImageMissing()` call. + imageManager.getImages(requestor, std::make_pair(dependencies, ++imageCorrelationID)); + runLoop.runOnce(); + + EXPECT_EQ(observer.count, 1); + + // Request for updated dependencies must be dispatched to the + // observer. + dependencies.emplace("post", ImageType::Icon); + imageManager.getImages(requestor, std::make_pair(dependencies, ++imageCorrelationID)); + runLoop.runOnce(); + + EXPECT_EQ(observer.count, 2); + + // Another requestor shall not have pending requests for already obtained images. + StubImageRequestor anotherRequestor(imageManager); + imageManager.getImages(anotherRequestor, std::make_pair(dependencies, ++imageCorrelationID)); + ASSERT_FALSE(anotherRequestor.hasPendingRequests()); + + dependencies.emplace("unfamiliar", ImageType::Icon); + imageManager.getImages(anotherRequestor, std::make_pair(dependencies, ++imageCorrelationID)); + EXPECT_TRUE(anotherRequestor.hasPendingRequests()); + EXPECT_TRUE(anotherRequestor.hasPendingRequest("unfamiliar")); } TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { -- cgit v1.2.1