Improve tile lifecycle determinism (maplibre#2819)

platform/android/MapLibreAndroid/proguard-rules.pro

@@ -9,6 +9,12 @@
 -keep class com.google.gson.JsonObject { *; }
 -keep class com.google.gson.JsonPrimitive { *; }
 -dontnote com.google.gson.**
+-keep enum org.maplibre.android.tile.TileOperation
+-keepclassmembers class * extends java.lang.Enum {
+    <fields>;
+    public static **[] values();
+    public static ** valueOf(java.lang.String);
+}
 
 # dontnote for keeps the entry point x but not the descriptor class y
 -dontnote org.maplibre.android.maps.MapLibreMap$OnFpsChangedListener

src/mbgl/tile/geometry_tile.cpp

@@ -229,7 +229,7 @@ void GeometryTile::setData(std::unique_ptr<const GeometryTileData> data_) {
         return;
     }
 
-    if (data_) {
+    if (!pending) {
         observer->onTileAction(id, sourceID, TileOperation::StartParse);
     }
 
@@ -245,12 +245,18 @@ void GeometryTile::setData(std::unique_ptr<const GeometryTileData> data_) {
 void GeometryTile::reset() {
     MLN_TRACE_FUNC();
 
-    // Mark the tile as pending again if it was complete before to prevent
-    // signaling a complete state despite pending parse operations.
-    pending = true;
+    // If there is pending work, indicate that work has been cancelled.
+    // Clear the pending status.
+    if (pending) {
+        observer->onTileAction(id, sourceID, TileOperation::Cancelled);
+        pending = false;
+    }
 
-    observer->onTileAction(id, sourceID, TileOperation::Cancelled);
+    // Reset the tile to an unloaded state to avoid signaling completion
+    // after clearing the tile's pending status.
+    loaded = false;
 
+    // Reset the worker to the `NeedsParse` state.
     ++correlationID;
     worker.self().invoke(&GeometryTileWorker::reset, correlationID);
 }
@@ -266,7 +272,10 @@ void GeometryTile::setLayers(const std::vector<Immutable<LayerProperties>>& laye
 
     // Mark the tile as pending again if it was complete before to prevent
     // signaling a complete state despite pending parse operations.
-    pending = true;
+    if (!pending) {
+        pending = true;
+        observer->onTileAction(id, sourceID, TileOperation::StartParse);
+    }
 
     std::vector<Immutable<LayerProperties>> impls;
     impls.reserve(layers.size());
@@ -308,6 +317,7 @@ void GeometryTile::onLayout(std::shared_ptr<LayoutResult> result, const uint64_t
     renderable = true;
     if (resultCorrelationID == correlationID) {
         pending = false;
+        observer->onTileAction(id, sourceID, TileOperation::EndParse);
     }
 
     layoutResult = std::move(result);
@@ -332,16 +342,13 @@ void GeometryTile::onLayout(std::shared_ptr<LayoutResult> result, const uint64_t
             }
         }
     }
-
-    if (!pending) {
-        observer->onTileAction(id, sourceID, TileOperation::EndParse);
-    }
 }
 
 void GeometryTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) {
     loaded = true;
     if (resultCorrelationID == correlationID) {
         pending = false;
+        observer->onTileAction(id, sourceID, TileOperation::Error);
     }
     observer->onTileError(*this, std::move(err));
 }

src/mbgl/tile/raster_dem_tile.cpp

@@ -66,13 +66,13 @@ void RasterDEMTile::setMetadata(std::optional<Timestamp> modified_, std::optiona
 
 void RasterDEMTile::setData(const std::shared_ptr<const std::string>& data) {
     if (!obsolete) {
-        pending = true;
         ++correlationID;
 
-        if (data) {
+        if (!pending) {
             observer->onTileAction(id, sourceID, TileOperation::StartParse);
         }
 
+        pending = true;
         worker.self().invoke(&RasterDEMTileWorker::parse, data, correlationID, encoding);
     }
 }
@@ -83,17 +83,18 @@ void RasterDEMTile::onParsed(std::unique_ptr<HillshadeBucket> result, const uint
         loaded = true;
         if (resultCorrelationID == correlationID) {
             pending = false;
+            observer->onTileAction(id, sourceID, TileOperation::EndParse);
         }
         renderable = static_cast<bool>(bucket);
         observer->onTileChanged(*this);
-        observer->onTileAction(id, sourceID, TileOperation::EndParse);
     }
 }
 
 void RasterDEMTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) {
     loaded = true;
     if (resultCorrelationID == correlationID) {
         pending = false;
+        observer->onTileAction(id, sourceID, TileOperation::Error);
     }
     observer->onTileError(*this, std::move(err));
 }
@@ -158,6 +159,9 @@ void RasterDEMTile::cancel() {
 
 void RasterDEMTile::markObsolete() {
     obsolete = true;
+    if (pending) {
+        observer->onTileAction(id, sourceID, TileOperation::Cancelled);
+    }
     pending = false;
     mailbox->abandon();
 }

src/mbgl/tile/raster_tile.cpp

@@ -55,13 +55,13 @@ void RasterTile::setMetadata(std::optional<Timestamp> modified_, std::optional<T
 
 void RasterTile::setData(const std::shared_ptr<const std::string>& data) {
     if (!obsolete) {
-        pending = true;
         ++correlationID;
 
-        if (data) {
+        if (!pending) {
             observer->onTileAction(id, sourceID, TileOperation::StartParse);
         }
 
+        pending = true;
         worker.self().invoke(&RasterTileWorker::parse, data, correlationID);
     }
 }
@@ -72,17 +72,18 @@ void RasterTile::onParsed(std::unique_ptr<RasterBucket> result, const uint64_t r
         loaded = true;
         if (resultCorrelationID == correlationID) {
             pending = false;
+            observer->onTileAction(id, sourceID, TileOperation::EndParse);
         }
         renderable = static_cast<bool>(bucket);
         observer->onTileChanged(*this);
-        observer->onTileAction(id, sourceID, TileOperation::EndParse);
     }
 }
 
 void RasterTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) {
     loaded = true;
     if (resultCorrelationID == correlationID) {
         pending = false;
+        observer->onTileAction(id, sourceID, TileOperation::Error);
     }
     observer->onTileError(*this, std::move(err));
 }
@@ -111,6 +112,9 @@ void RasterTile::cancel() {
 
 void RasterTile::markObsolete() {
     obsolete = true;
+    if (pending) {
+        observer->onTileAction(id, sourceID, TileOperation::Cancelled);
+    }
     pending = false;
     mailbox->abandon();
 }

test/map/map.test.cpp

@@ -1698,6 +1698,12 @@ TEST(Map, ObserveTileLifecycle) {
     };
     std::mutex tileMutex;
     std::vector<TileEntry> tileOps;
+    // We expect to see a valid lifecycle for every tile in this list.
+    const std::vector<OverscaledTileID> expectedTiles = {
+        {10, 0, 10, 163, 395}, {10, 0, 10, 163, 396}, {10, 0, 10, 164, 395}, {10, 0, 10, 164, 396},
+        // Lower zooms can also be seen, but not always, so we
+        // ignore them.
+    };
 
     struct ShaderEntry {
         shaders::BuiltIn id;
@@ -1762,17 +1768,14 @@ TEST(Map, ObserveTileLifecycle) {
         }
     }
 
-    // We expect to see a valid lifecycle for every tile in this list.
-    const std::vector<OverscaledTileID> expectedTiles = {
-        {10, 0, 10, 163, 395}, {10, 0, 10, 163, 396}, {10, 0, 10, 164, 395}, {10, 0, 10, 164, 396},
-        // Lower zooms can also be seen, but not always, so we
-        // ignore them.
-    };
-
     for (const auto& tile : expectedTiles) {
         TileOperation stage = TileOperation::NullOp;
+        bool parsing = false;
 
-        for (const auto& op : tileOps) {
+        // Suppressing warning-as-error to use range-based for. Index is desired for debugging context.
+        // NOLINTNEXTLINE
+        for (size_t i = 0; i < tileOps.size(); i++) {
+            const auto& op = tileOps[i];
             if (op.id != tile) continue;
             switch (op.op) {
                 case TileOperation::RequestedFromCache: {
@@ -1781,26 +1784,34 @@ TEST(Map, ObserveTileLifecycle) {
                     break;
                 }
                 case TileOperation::RequestedFromNetwork: {
-                    EXPECT_THAT(
-                        stage,
-                        testing::AnyOf(
-                            TileOperation::StartParse, TileOperation::EndParse, TileOperation::RequestedFromCache));
+                    // Parsing happens concurrently with the file source request and can start and stop between requests
+                    EXPECT_THAT(stage,
+                                testing::AnyOf(
+                                    TileOperation::StartParse, TileOperation::EndParse, TileOperation::LoadFromCache));
                     stage = TileOperation::RequestedFromNetwork;
                     break;
                 }
                 case TileOperation::LoadFromNetwork: {
-                    EXPECT_THAT(stage, TileOperation::RequestedFromNetwork);
+                    // Parsing happens concurrently with the file source request and can start and stop between requests
+                    EXPECT_THAT(
+                        stage,
+                        testing::AnyOf(
+                            TileOperation::StartParse, TileOperation::EndParse, TileOperation::RequestedFromNetwork));
                     stage = TileOperation::LoadFromNetwork;
                     break;
                 }
                 case TileOperation::LoadFromCache: {
-                    EXPECT_EQ(stage, TileOperation::RequestedFromCache);
+                    EXPECT_THAT(stage, testing::AnyOf(TileOperation::RequestedFromCache, TileOperation::StartParse));
                     stage = TileOperation::LoadFromCache;
                     break;
                 }
                 case TileOperation::StartParse: {
-                    EXPECT_THAT(stage, testing::AnyOf(TileOperation::LoadFromNetwork, TileOperation::LoadFromCache));
+                    // Parsing is expected to be started early during the request process by a call to `setLayers`
+                    EXPECT_THAT(stage,
+                                testing::AnyOf(TileOperation::RequestedFromCache, TileOperation::RequestedFromNetwork));
+                    EXPECT_FALSE(parsing); // We must not already be parsing when seeing this marker.
                     stage = TileOperation::StartParse;
+                    parsing = true;
                     break;
                 }
                 case TileOperation::Cancelled: {
@@ -1811,6 +1822,7 @@ TEST(Map, ObserveTileLifecycle) {
                                                TileOperation::LoadFromCache,
                                                TileOperation::StartParse));
                     stage = TileOperation::Cancelled;
+                    parsing = false;
                     break;
                 }
                 case TileOperation::EndParse: {
@@ -1831,6 +1843,8 @@ TEST(Map, ObserveTileLifecycle) {
                                 testing::AnyOf(TileOperation::StartParse,
                                                TileOperation::LoadFromNetwork,
                                                TileOperation::RequestedFromNetwork));
+                    EXPECT_TRUE(parsing); // We must have been parsing to see the EndParse marker.
+                    parsing = false;
                     stage = TileOperation::EndParse;
                     break;
                 }
@@ -1844,5 +1858,6 @@ TEST(Map, ObserveTileLifecycle) {
         }
 
         EXPECT_THAT(stage, testing::AnyOf(TileOperation::EndParse, TileOperation::Cancelled));
+        EXPECT_FALSE(parsing);
     }
 }