[core] Attempt placement if at least one image is ready

[brunoabinader]

This patch fixes an edge case when two or more images are referred on a style, but only one of them gets actually fetched.

There are times (like in the test case from #9792) where two or more images are needed, but only one (or more up to n - 1) gets successfully fetched. In those cases, we still want to attempt placement, so hasPendingSymbolDependencies should return false. We force this behavior by checking whether imageMap is empty before actually checking for any pending image dependencies.

This works as long as imageMap state is always up-to-date. This isn't true in cases e.g. when we had at least one placement attempt already. To fix that, we clear up imageMap after using it, as it gets updated in GeometryTileWorker::onImagesAvailable anyway.

Fixes #9792.

[brunoabinader]

Additionally, there is a race condition that sometimes prevents the images from being emplaced after GeometryTileWorker receives them: If redoLayout() happens before onImagesAvailable, symbolLayoutsNeedPreparation will be false and thus it'll never attempt a new placement in symbolDependenciesChanged. This is the reason why some CI failed with only the first patch.

[brunoabinader]

android-debug-arm-v7 is also failing due to Firebase timing out in some other recent PRs, so this looks like an unrelated CI failure.

[ChrisLoer]

This looks good to me, but I'm just looking at the new per-tile atlas logic for the first time.

This works as long as imageMap state is always up-to-date. This isn't true in cases e.g. when we had at least one placement attempt already. To fix that, we clear up imageMap after using it, as it gets updated in GeometryTileWorker::onImagesAvailable anyway.

How would placement have already happened? Are there cases where onImagesAvailable will get called multiple times?

Your changes make it look like we're going in the direction of supporting progressive loading/placement of images (so, we could get all glyphs and one of two images, do placement and render the tile, then get the second image, and redo placement (including text shaping) and render the tile again). I didn't think that was something we actually support (i.e. ImageManager calls onImagesAvailable once per request).

Is there a possible simplification where we make the "images get loaded all at once" expectation more explicit? In that case instead of trying to manage the state of imageMap, pendingImageDependencies, and symbolLayoutsNeedPreparation, we could just always completely erase pendingImageDependencies in onImagesAvailable.

[jfirebaugh]

Yeah -- can we revisit #9739? What exactly about the prior logic was a problem?

[jfirebaugh]

Started to do some analysis... Tracing into the RecycleMapUpdateImages test added there, I see the following sequence of events following loadStyle("flipped_marker", ...):

main	worker
sends setData
	receives setData, redoes layout, requests the default_marker image (because it's still holding the old layer information)
sends setLayers
	processes setLayers, redoes layout, requests the flipped_marker image
receives getImages, sends onImagesAvailable
	receives onImagesAvailable
receives getImages, onImagesAvailable
	receives onImagesAvailable

This particular ordering is not guaranteed; the relative order between main and worker could vary.

Also, I've seen this test pass even after reverting the geometry_tile_worker.cpp changes in #9739.

[brunoabinader]

Also, I've seen this test pass even after reverting the geometry_tile_worker.cpp changes in #9739.

I don't think so: I've added a new branch https://github.com/mapbox/mapbox-gl-native/tree/9739-revert-test that reverts cd8eb13, and API.RecycleMapUpdateImages fails both locally and on CI:

• https://circleci.com/gh/mapbox/mapbox-gl-native/tree/9739-revert-test
• https://www.bitrise.io/build/52bfc016bd46f17c
• https://www.bitrise.io/build/09a4128b63363146

[brunoabinader]

Is there a possible simplification where we make the "images get loaded all at once" expectation more explicit? In that case instead of trying to manage the state of imageMap, pendingImageDependencies, and symbolLayoutsNeedPreparation, we could just always completely erase pendingImageDependencies in onImagesAvailable.

This is exactly how the previous implementation works (and causes the test from #9739 to fail).

[brunoabinader]

Your changes make it look like we're going in the direction of supporting progressive loading/placement of images (so, we could get all glyphs and one of two images, do placement and render the tile, then get the second image, and redo placement (including text shaping) and render the tile again). I didn't think that was something we actually support (i.e. ImageManager calls onImagesAvailable once per request).

I believe progressive loading/placement of images is exactly the intent - as per @jfirebaugh comments in #9739 (comment):

The intent behind the prior logic was that once the style's sprite has been loaded, missing images should no longer block symbol layers from being placed and rendered.

[brunoabinader]

I'll try to reply both questions in a single answer:

How would placement have already happened? Are there cases where onImagesAvailable will get called multiple times?

can we revisit #9739? What exactly about the prior logic was a problem?

In GeometryTileWorker, we have a rule that is:

Do not attempt symbol placement if there is no data, layers, placement configuration or if there is any pending symbol dependencies left.

The problem here lies in the pending symbol dependencies. Before #9739, whenever onImagesAvailable gets called, we were clearing the list of pending images, causing hasPendingSymbolDependencies to return true and thus allowing attemptPlacement to be made. This can lead to erroneous scenarios, as described below:

Given the analysis in #9816 (comment), the first call for onImagesAvailable (which contains an empty imageMap) leads up to a call for attemptPlacement, which is not early aborted (because pendingImageDependencies was previously cleared in onImagesAvailable) and causes the symbol layout to be prepared using an empty imageMap.

This should be fine as long as the second onImagesAvailable (which now contains flipped_image inside of imageMap) attempts placement - which does not happen because symbolLayoutsNeedPreparation is now false, and causes the placement attempt to not happen inside symbolDependenciesChanged.

The changes in #9739 changes this behavior by making the first call for onImagesAvailable to not attempt placement (because hasPendingSymbolDependencies will now return true), and when the second onImagesAvailable gets called, the placement attempt will prepare the symbol layout using the now-correct imageMap containing flipped_image.

However, this raises a new issue (as pointed out by @jfirebaugh in #9792) in which it causes symbol placement to never happen because pendingImageDependencies is never empty. In c73ae50 we circumvent that by checking if imageMap is non-empty: if true, it clears attemptPlacement for resuming its work. Because we cannot assume the previous imageMap represents the images required by the current tile state, we also need to clear it up after using it in attemptPlacement.

In some cases, we've already done layout and attempted placement before new images have arrived (which is the case for API.ReaddImage and when onImagesAvailable is called, symbolLayoutsNeedPreparation will already be false and thus the symbol layout won't re-prepare using the updated images. To prevent that, we mark symbolLayoutsNeedPreparation as true whenever onImagesAvailable gets called in e0608df.

[kkaefer]

@jfirebaugh, @brunoabinader can we solve the race condition by using the same correlationID approach that we use when going the other way around? i.e. as a way to guarantee that we only respond/attemptPlacement once we got an answer to the last image map we requested?

As far as what happens when there's an error loading an image: I think we should treat resource loading errors as a fatal error for still image rendering, but mark the image as "completed" in continuous rendering mode so that we can run attemptPlacement, and potentially wait for a refreshed request or for network connectivity coming back. It seems like the underlying issue is that we don't have a strategy for how to explicitly handle resource loading failure. Tiles can be in a "loaded" state, which means that we've made an attempt to load it and received a reply (either positive or negative). We should look at adopting a similar concept for resources so that we know whether we're still waiting for a resource to load or can start attempting placement, even if some resources are missing.

[brunoabinader]

can we solve the race condition by using the same correlationID approach that we use when going the other way around? i.e. as a way to guarantee that we only respond/attemptPlacement once we got an answer to the last image map we requested?

I think that would be a problem e.g. in continuous mode like you mentioned, in a sense that we want to start rendering tiles even though not all images are yet loaded.

Also, from my tests the correlation does not solve the case where a placement attempt did not re-prepare the symbol layouts because a previous layout+placement sequence caused symbolLayoutsNeedPreparation to be false - and thus causes the placement attempt to skip re-preparing the symbol layout features with the updated images.

[brunoabinader]

We should look at adopting a similar concept for resources so that we know whether we're still waiting for a resource to load or can start attempting placement, even if some resources are missing.

My very first attempt to make API.RecycleMapUpdateImages pass is the following:

@@ -58,14 +59,22 @@ void ImageManager::getImages(ImageRequestor& requestor, ImageDependencies depend
     // Otherwise, delay notification until the sprite is loaded. At that point, if any of the
     // dependencies are still unavailable, we'll just assume they are permanently missing.
     bool hasAllDependencies = true;
-    if (!isLoaded()) {
-        for (const auto& dependency : dependencies) {
-            if (images.find(dependency) == images.end()) {
-                hasAllDependencies = false;
-            }
+    for (const auto& dependency : dependencies) {
+        if (images.find(dependency) == images.end()) {
+            hasAllDependencies = false;
         }
     }

This causes hasAllDependencies to be false even though we're loaded, and thus not causing the first of the onImagesAvailable sequence from #9816 (comment) to be called.

[brunoabinader]

After discussing with @ChrisLoer I came to conclusion these patches were in fact working around the fact that onImagesAvailable gets called multiple times, and due to the way how GeometryTileWorker is implemented, it expects that to be called only when actually all the image dependencies requested are met. Thus, I'm abandoning this PR in favor of a simpler approach proposed in #9842.