MapRenderer::update/requestRender should coalesce requests once per event loop

[ChrisLoer]

Since we introduced asynchronous rendering on Android, modifications to the map made on the UI thread can be picked up immediately, even if the UI thread is in the middle of making changes that are meant to be grouped together. For example, if a user wanted to move the order of a layer, they might code something like:

removeLayer(layerToMove)
addLayerAbove(layerToMove, topLayer.id)

In this case, the call to removeLayer would update the map parameters and immediately wake up the render thread, which could start rendering the map with the removed layer. The layer wouldn't get added back until the next frame. In many cases this could cause more than a frame's worth of flicker by causing the map to throw away "unused" data.

Instead, we should coalesce map updates on the UI thread and apply them once per event loop.

cc @tobrun @ivovandongen @jfirebaugh @LindaRosa

[themics]

I had the same issue, so I just changed Map::Impl::onUpdate() to send rendererFrontend.update() to the event loop. It would be great if the official version have a built-in support about it. Here's my ad-hoc patch:

--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -11,6 +11,7 @@
 #include <mbgl/storage/file_source.hpp>
 #include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
+#include <mbgl/util/async_task.hpp>
 #include <mbgl/util/constants.hpp>
 #include <mbgl/util/math.hpp>
 #include <mbgl/util/exception.hpp>
@@ -86,6 +87,7 @@ public:
     bool loading = false;
     bool rendererFullyLoaded;
     std::unique_ptr<StillImageRequest> stillImageRequest;
+    std::unique_ptr<util::AsyncTask> updateAsyncTask;
 };
 
 Map::Map(RendererFrontend& rendererFrontend,
@@ -762,31 +764,34 @@ void Map::Impl::onUpdate() {
     if (mode != MapMode::Continuous && !stillImageRequest) {
         return;
     }
-    
-    TimePoint timePoint = mode == MapMode::Continuous ? Clock::now() : Clock::time_point::max();
-
-    transform.updateTransitions(timePoint);
-
-    UpdateParameters params = {
-        style->impl->isLoaded(),
-        mode,
-        pixelRatio,
-        debugOptions,
-        timePoint,
-        transform.getState(),
-        style->impl->getGlyphURL(),
-        style->impl->spriteLoaded,
-        style->impl->getTransitionOptions(),
-        style->impl->getLight()->impl,
-        style->impl->getImageImpls(),
-        style->impl->getSourceImpls(),
-        style->impl->getLayerImpls(),
-        annotationManager,
-        prefetchZoomDelta,
-        bool(stillImageRequest)
-    };
 
-    rendererFrontend.update(std::make_shared<UpdateParameters>(std::move(params)));
+    updateAsyncTask = std::make_unique<util::AsyncTask>([this]() {
+        TimePoint timePoint = mode == MapMode::Continuous ? Clock::now() : Clock::time_point::max();
+
+        transform.updateTransitions(timePoint);
+
+        UpdateParameters params = {
+            style->impl->isLoaded(),
+            mode,
+            pixelRatio,
+            debugOptions,
+            timePoint,
+            transform.getState(),
+            style->impl->getGlyphURL(),
+            style->impl->spriteLoaded,
+            style->impl->getTransitionOptions(),
+            style->impl->getLight()->impl,
+            style->impl->getImageImpls(),
+            style->impl->getSourceImpls(),
+            style->impl->getLayerImpls(),
+            annotationManager,
+            prefetchZoomDelta,
+            bool(stillImageRequest)
+        };
+
+        rendererFrontend.update(std::make_shared<UpdateParameters>(std::move(params)));
+    });
+    updateAsyncTask->send();
 }
 
 void Map::Impl::onStyleLoading() {

[tobrun]

@themics capturing from @ivovandongen that this is what he had in mind to fix this issue. Would you be able to provide a PR so you get contribution for your efforts?

[themics]

@tobrun Sure, but please note that I didn't test platforms other than Android & iOS.

[tobrun]

@themics thanks for pointing that out, capturing from renderer_frontend.hpp that // Coalescing updates is up to the implementer so the actual coalescing should happen in the android specific renderer_frontend (android_renderer_frontend) ?

[themics]

@tobrun Ah, didn't notice that comment. I'll update my PR.