Android/core: Couple a sequenced scheduler to the lifetime of a GeoJSON source (#2182)

include/mbgl/style/sources/geojson_source.hpp

@@ -41,8 +41,8 @@ public:
     using TileFeatures = mapbox::feature::feature_collection<int16_t>;
     using Features = mapbox::feature::feature_collection<double>;
     static std::shared_ptr<GeoJSONData> create(const GeoJSON&,
-                                               const Immutable<GeoJSONOptions>& = GeoJSONOptions::defaultOptions(),
-                                               std::shared_ptr<Scheduler> scheduler = nullptr);
+                                               std::shared_ptr<Scheduler> scheduler,
+                                               const Immutable<GeoJSONOptions>& = GeoJSONOptions::defaultOptions());
 
     virtual ~GeoJSONData() = default;
     virtual void getTile(const CanonicalTileID&, const std::function<void(TileFeatures)>&) = 0;
@@ -51,8 +51,6 @@ public:
     virtual Features getChildren(std::uint32_t) = 0;
     virtual Features getLeaves(std::uint32_t, std::uint32_t limit, std::uint32_t offset) = 0;
     virtual std::uint8_t getClusterExpansionZoom(std::uint32_t) = 0;
-
-    virtual std::shared_ptr<Scheduler> getScheduler() { return nullptr; }
 };
 
 class GeoJSONSource final : public Source {
@@ -83,6 +81,7 @@ private:
     std::optional<std::string> url;
     std::unique_ptr<AsyncRequest> req;
     std::shared_ptr<Scheduler> threadPool;
+    std::shared_ptr<Scheduler> sequencedScheduler;
     mapbox::base::WeakPtrFactory<Source> weakFactory{this};
 };
 

platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp

@@ -245,7 +245,8 @@ void FeatureConverter::convertJson(std::shared_ptr<std::string> json, ActorRef<G
         return;
     }
 
-    callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(*converted, options));
+    callback.invoke(&GeoJSONDataCallback::operator(),
+                    style::GeoJSONData::create(*converted, sequencedScheduler, options));
 }
 
 template <class JNIType>
@@ -257,7 +258,8 @@ void FeatureConverter::convertObject(
     android::UniqueEnv _env = android::AttachEnv();
     // Convert the jni object
     auto geometry = JNIType::convert(*_env, *jObject);
-    callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(geometry, options));
+    callback.invoke(&GeoJSONDataCallback::operator(),
+                    style::GeoJSONData::create(geometry, sequencedScheduler, options));
 }
 
 Update::Update(Converter _converterFn, std::unique_ptr<Actor<GeoJSONDataCallback>> _callback)

platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp

@@ -16,7 +16,8 @@ using GeoJSONDataCallback = std::function<void(std::shared_ptr<style::GeoJSONDat
 class FeatureConverter {
 public:
     explicit FeatureConverter(Immutable<style::GeoJSONOptions> options_)
-        : options(std::move(options_)) {}
+        : options(std::move(options_)),
+          sequencedScheduler(Scheduler::GetSequenced()) {}
     void convertJson(std::shared_ptr<std::string>, ActorRef<GeoJSONDataCallback>);
 
     template <class JNIType>
@@ -25,6 +26,7 @@ public:
 
 private:
     Immutable<style::GeoJSONOptions> options;
+    std::shared_ptr<Scheduler> sequencedScheduler;
 };
 
 struct Update {

src/mbgl/style/sources/geojson_source.cpp

@@ -21,7 +21,8 @@ Immutable<GeoJSONOptions> GeoJSONOptions::defaultOptions() {
 
 GeoJSONSource::GeoJSONSource(std::string id, Immutable<GeoJSONOptions> options)
     : Source(makeMutable<Impl>(std::move(id), std::move(options))),
-      threadPool(Scheduler::GetBackground()) {}
+      threadPool(Scheduler::GetBackground()),
+      sequencedScheduler(Scheduler::GetSequenced()) {}
 
 GeoJSONSource::~GeoJSONSource() = default;
 
@@ -40,20 +41,8 @@ void GeoJSONSource::setURL(const std::string& url_) {
     }
 }
 
-namespace {
-
-inline std::shared_ptr<GeoJSONData> createGeoJSONData(const mapbox::geojson::geojson& geoJSON,
-                                                      const GeoJSONSource::Impl& impl) {
-    if (auto data = impl.getData().lock()) {
-        return GeoJSONData::create(geoJSON, impl.getOptions(), data->getScheduler());
-    }
-    return GeoJSONData::create(geoJSON, impl.getOptions());
-}
-
-} // namespace
-
 void GeoJSONSource::setGeoJSON(const mapbox::geojson::geojson& geoJSON) {
-    setGeoJSONData(createGeoJSONData(geoJSON, impl()));
+    setGeoJSONData(GeoJSONData::create(geoJSON, sequencedScheduler, impl().getOptions()));
 }
 
 void GeoJSONSource::setGeoJSONData(std::shared_ptr<GeoJSONData> geoJSONData) {
@@ -88,13 +77,15 @@ void GeoJSONSource::loadDescription(FileSource& fileSource) {
         } else if (res.noContent) {
             observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error("unexpectedly empty GeoJSON")));
         } else {
-            auto makeImplInBackground = [currentImpl = baseImpl, data = res.data]() -> Immutable<Source::Impl> {
+            auto makeImplInBackground = [currentImpl = baseImpl,
+                                         data = res.data,
+                                         seqScheduler{sequencedScheduler}]() -> Immutable<Source::Impl> {
                 assert(data);
                 auto& current = static_cast<const Impl&>(*currentImpl);
                 conversion::Error error;
                 std::shared_ptr<GeoJSONData> geoJSONData;
                 if (std::optional<GeoJSON> geoJSON = conversion::convertJSON<GeoJSON>(*data, error)) {
-                    geoJSONData = createGeoJSONData(*geoJSON, current);
+                    geoJSONData = GeoJSONData::create(*geoJSON, std::move(seqScheduler), current.getOptions());
                 } else {
                     // Create an empty GeoJSON VT object to make sure we're not
                     // infinitely waiting for tiles to load.

src/mbgl/style/sources/geojson_source_impl.cpp

@@ -38,8 +38,6 @@ class GeoJSONVTData final : public GeoJSONData {
 
     std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; }
 
-    std::shared_ptr<Scheduler> getScheduler() final { return scheduler; }
-
     friend GeoJSONData;
     GeoJSONVTData(const GeoJSON& geoJSON,
                   const mapbox::geojsonvt::Options& options,
@@ -91,8 +89,8 @@ T evaluateFeature(const mapbox::feature::feature<double>& f,
 
 // static
 std::shared_ptr<GeoJSONData> GeoJSONData::create(const GeoJSON& geoJSON,
-                                                 const Immutable<GeoJSONOptions>& options,
-                                                 std::shared_ptr<Scheduler> scheduler) {
+                                                 std::shared_ptr<Scheduler> scheduler,
+                                                 const Immutable<GeoJSONOptions>& options) {
     constexpr double scale = util::EXTENT / util::tileSize_D;
     if (options->cluster && geoJSON.is<Features>() && !geoJSON.get<Features>().empty()) {
         mapbox::supercluster::Options clusterOptions;
@@ -128,7 +126,6 @@ std::shared_ptr<GeoJSONData> GeoJSONData::create(const GeoJSON& geoJSON,
     vtOptions.buffer = static_cast<uint16_t>(::round(scale * options->buffer));
     vtOptions.tolerance = scale * options->tolerance;
     vtOptions.lineMetrics = options->lineMetrics;
-    if (!scheduler) scheduler = Scheduler::GetSequenced();
     return std::shared_ptr<GeoJSONData>(new GeoJSONVTData(geoJSON, vtOptions, std::move(scheduler)));
 }
 

test/style/source.test.cpp

@@ -969,8 +969,10 @@ TEST(Source, VectorSourceUrlSetTiles) {
 TEST(Source, GeoJSONSourceTilesAfterDataReset) {
     SourceTest test;
     GeoJSONSource source("source");
-    auto geoJSONData = GeoJSONData::create(mapbox::geojson::parse(
-        R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"));
+    auto geoJSONData = GeoJSONData::create(
+        mapbox::geojson::parse(
+            R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"),
+        Scheduler::GetSequenced());
     source.setGeoJSONData(geoJSONData);
     RenderGeoJSONSource renderSource{staticImmutableCast<GeoJSONSource::Impl>(source.baseImpl), test.threadPool};