[core] Revert coalescing requests to the client for the same missing image

3 files changed, 34 insertions, 80 deletions

diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp
index 7b51fb9a2e..d001084f92 100644
--- a/src/mbgl/renderer/image_manager.cpp
+++ b/src/mbgl/renderer/image_manager.cpp
@@ -138,9 +138,8 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) {
 
 void ImageManager::notifyIfMissingImageAdded() {
     for (auto it = missingImageRequestors.begin(); it != missingImageRequestors.end();) {
-        ImageRequestor& requestor = *it->first;
-        if (!requestor.hasPendingRequests()) {
-            notify(requestor, it->second);
+        if (it->second.callbacks.empty()) {
+            notify(*it->first, it->second.pair);
             it = missingImageRequestors.erase(it);
         } else {
             ++it;
@@ -170,56 +169,38 @@ void ImageManager::reduceMemoryUseIfCacheSizeExceedsLimit() {
 }
 
 void ImageManager::checkMissingAndNotify(ImageRequestor& requestor, const ImageRequestPair& pair) {
-    ImageDependencies missingDependencies;
-
+    std::vector<std::string> missingImages;
+    missingImages.reserve(pair.first.size());
     for (const auto& dependency : pair.first) {
         if (images.find(dependency.first) == images.end()) {
-            missingDependencies.emplace(dependency);
+            missingImages.push_back(dependency.first);
         }
     }
 
-    if (!missingDependencies.empty()) {
+    if (!missingImages.empty()) {
         ImageRequestor* requestorPtr = &requestor;
-        assert(!missingImageRequestors.count(requestorPtr));
-        missingImageRequestors.emplace(requestorPtr, pair);
 
-        for (const auto& dependency : missingDependencies) {
-            const std::string& missingImage = dependency.first;
-            assert(observer != nullptr);
+        auto emplaced = missingImageRequestors.emplace(requestorPtr, MissingImageRequestPair { pair, {} });
+        assert(emplaced.second);
 
-            auto existingRequestorsIt = requestedImages.find(missingImage);
-            if (existingRequestorsIt != requestedImages.end()) { // Already asked client about this image.
-                std::set<ImageRequestor*>& 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)));
+        for (const auto& missingImage : missingImages) {
+            assert(observer != nullptr);
+            requestedImages[missingImage].emplace(&requestor);
+            auto callback = std::make_unique<ActorCallback>(
+                *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());
+            });
         }
     } else {
         // Associate requestor with an image that was provided by the client.
@@ -249,7 +230,7 @@ void ImageManager::notify(ImageRequestor& requestor, const ImageRequestPair& pai
         }
     }
 
-    requestor.onImagesAvailable(std::move(iconMap), std::move(patternMap), std::move(versionMap), pair.second);
+    requestor.onImagesAvailable(iconMap, 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 5ed6e237f0..9097418681 100644
--- a/src/mbgl/renderer/image_manager.hpp
+++ b/src/mbgl/renderer/image_manager.hpp
@@ -58,7 +58,13 @@ private:
     bool loaded = false;
 
     std::map<ImageRequestor*, ImageRequestPair> requestors;
-    std::map<ImageRequestor*, ImageRequestPair> missingImageRequestors;
+    using Callback = std::function<void()>;
+    using ActorCallback = Actor<Callback>;
+    struct MissingImageRequestPair {
+        ImageRequestPair pair;
+        std::map<std::string, std::unique_ptr<ActorCallback>> callbacks;
+    };
+    std::map<ImageRequestor*, MissingImageRequestPair> missingImageRequestors;
     std::map<std::string, std::set<ImageRequestor*>> requestedImages;
     std::size_t requestedImagesCacheSize = 0ul;
     ImageMap images;
@@ -71,17 +77,8 @@ 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<std::string> pendingRequests;
 };
 
 } // namespace mbgl
diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp
index 20c0a3a7f3..16700d713f 100644
--- a/test/renderer/image_manager.test.cpp
+++ b/test/renderer/image_manager.test.cpp
@@ -187,30 +187,6 @@ 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) {