From a2a8a39ba691379081e9bef9a927d931590c813d Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 20 Nov 2019 21:05:45 +0200 Subject: [PATCH 1/3] [core] Calculate GeoJSON tile geometries in a background thread Call `mapbox::geojsonvt::GeoJSONVT::getTile()` in a background thread, so that the rendering thread is not blocked. --- include/mbgl/style/sources/geojson_source.hpp | 12 +++--- .../sources/render_geojson_source.cpp | 7 ++- .../style/sources/geojson_source_impl.cpp | 43 ++++++++----------- src/mbgl/tile/geojson_tile.cpp | 23 +++++++--- src/mbgl/tile/geojson_tile.hpp | 11 ++++- test/tile/geojson_tile.test.cpp | 42 ++++++++++++++---- 6 files changed, 86 insertions(+), 52 deletions(-) diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index 1cc104b2455..9569b61df46 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -39,17 +39,19 @@ struct GeoJSONOptions { }; class GeoJSONData { public: + using TileFeatures = mapbox::feature::feature_collection; + using Features = mapbox::feature::feature_collection; static std::shared_ptr create(const GeoJSON&, Immutable = GeoJSONOptions::defaultOptions()); virtual ~GeoJSONData() = default; - virtual mapbox::feature::feature_collection getTile(const CanonicalTileID&) = 0; + virtual void getTile(const CanonicalTileID&, const std::function&) = 0; // SuperclusterData - virtual mapbox::feature::feature_collection getChildren(const std::uint32_t) = 0; - virtual mapbox::feature::feature_collection getLeaves(const std::uint32_t, - const std::uint32_t limit = 10u, - const std::uint32_t offset = 0u) = 0; + virtual Features getChildren(const std::uint32_t) = 0; + virtual Features getLeaves(const std::uint32_t, + const std::uint32_t limit = 10u, + const std::uint32_t offset = 0u) = 0; virtual std::uint8_t getClusterExpansionZoom(std::uint32_t) = 0; }; diff --git a/src/mbgl/renderer/sources/render_geojson_source.cpp b/src/mbgl/renderer/sources/render_geojson_source.cpp index 4ed9dd9334f..94d171cf18b 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.cpp +++ b/src/mbgl/renderer/sources/render_geojson_source.cpp @@ -95,8 +95,7 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, const uint8_t maxZ = impl().getZoomRange().max; for (const auto& pair : tilePyramid.getTiles()) { if (pair.first.canonical.z <= maxZ) { - static_cast(pair.second.get()) - ->updateData(data_->getTile(pair.first.canonical), needsRelayout); + static_cast(pair.second.get())->updateData(data_, needsRelayout); } } } @@ -112,8 +111,8 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, util::tileSize, impl().getZoomRange(), optional{}, - [&, data_] (const OverscaledTileID& tileID) { - return std::make_unique(tileID, impl().id, parameters, data_->getTile(tileID.canonical)); + [&, data_](const OverscaledTileID& tileID) { + return std::make_unique(tileID, impl().id, parameters, data_); }); } diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 962d6bd060e..e4b48223f42 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -18,18 +19,16 @@ class GeoJSONVTData : public GeoJSONData { : impl(geoJSON, options) { } - mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { - return impl.getTile(tileID.z, tileID.x, tileID.y).features; + void getTile(const CanonicalTileID& id, const std::function& fn) final { + assert(fn); + // It's safe to pass `this` as scheduler will die earlier. + scheduler.scheduleAndReplyValue( + [id, this]() -> TileFeatures { return impl.getTile(id.z, id.x, id.y).features; }, fn); } - mapbox::feature::feature_collection getChildren(const std::uint32_t) final { - return {}; - } + Features getChildren(const std::uint32_t) final { return {}; } - mapbox::feature::feature_collection - getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { - return {}; - } + Features getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { return {}; } std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; @@ -37,26 +36,22 @@ class GeoJSONVTData : public GeoJSONData { private: mapbox::geojsonvt::GeoJSONVT impl; + SequencedScheduler scheduler; }; class SuperclusterData : public GeoJSONData { public: - SuperclusterData(const mapbox::feature::feature_collection& features, - const mapbox::supercluster::Options& options) - : impl(features, options) { - } + SuperclusterData(const Features& features, const mapbox::supercluster::Options& options) + : impl(features, options) {} - mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { - return impl.getTile(tileID.z, tileID.x, tileID.y); + void getTile(const CanonicalTileID& id, const std::function& fn) final { + assert(fn); + fn(impl.getTile(id.z, id.x, id.y)); } - mapbox::feature::feature_collection getChildren(const std::uint32_t cluster_id) final { - return impl.getChildren(cluster_id); - } + Features getChildren(const std::uint32_t cluster_id) final { return impl.getChildren(cluster_id); } - mapbox::feature::feature_collection getLeaves(const std::uint32_t cluster_id, - const std::uint32_t limit, - const std::uint32_t offset) final { + Features getLeaves(const std::uint32_t cluster_id, const std::uint32_t limit, const std::uint32_t offset) final { return impl.getLeaves(cluster_id, limit, offset); } @@ -85,8 +80,7 @@ T evaluateFeature(const mapbox::feature::feature& f, // static std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutable options) { constexpr double scale = util::EXTENT / util::tileSize; - if (options->cluster && geoJSON.is>() && - !geoJSON.get>().empty()) { + if (options->cluster && geoJSON.is() && !geoJSON.get().empty()) { mapbox::supercluster::Options clusterOptions; clusterOptions.maxZoom = options->clusterMaxZoom; clusterOptions.extent = util::EXTENT; @@ -111,8 +105,7 @@ std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutab toReturn[p.first] = evaluateFeature(*feature, p.second.second, accumulated); } }; - return std::make_shared(geoJSON.get>(), - clusterOptions); + return std::make_shared(geoJSON.get(), clusterOptions); } mapbox::geojsonvt::Options vtOptions; diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp index 0782f74d5d7..a4c48fb809b 100644 --- a/src/mbgl/tile/geojson_tile.cpp +++ b/src/mbgl/tile/geojson_tile.cpp @@ -1,20 +1,29 @@ -#include -#include #include #include - +#include +#include +#include namespace mbgl { GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID, std::string sourceID_, const TileParameters& parameters, - mapbox::feature::feature_collection features) + std::shared_ptr data_) : GeometryTile(overscaledTileID, sourceID_, parameters) { - updateData(std::move(features)); + updateData(std::move(data_)); } -void GeoJSONTile::updateData(mapbox::feature::feature_collection features, bool resetLayers) { - setData(std::make_unique(std::move(features)), resetLayers); +void GeoJSONTile::updateData(std::shared_ptr data_, bool resetLayers) { + assert(data_); + data = std::move(data_); + data->getTile(id.canonical, + [this, self = weakFactory.makeWeakPtr(), capturedData = data.get(), resetLayers]( + style::GeoJSONData::TileFeatures features) { + if (!self) return; + if (data.get() != capturedData) return; + auto tileData = std::make_unique(std::move(features)); + setData(std::move(tileData), resetLayers); + }); } void GeoJSONTile::querySourceFeatures( diff --git a/src/mbgl/tile/geojson_tile.hpp b/src/mbgl/tile/geojson_tile.hpp index 9161e33f0c5..26dc300bdc3 100644 --- a/src/mbgl/tile/geojson_tile.hpp +++ b/src/mbgl/tile/geojson_tile.hpp @@ -4,6 +4,9 @@ #include namespace mbgl { +namespace style { +class GeoJSONData; +} // namespace style class TileParameters; @@ -12,13 +15,17 @@ class GeoJSONTile : public GeometryTile { GeoJSONTile(const OverscaledTileID&, std::string sourceID, const TileParameters&, - mapbox::feature::feature_collection); + std::shared_ptr); - void updateData(mapbox::feature::feature_collection, bool resetLayers = false); + void updateData(std::shared_ptr data, bool resetLayers = false); void querySourceFeatures( std::vector& result, const SourceQueryOptions&) override; + +private: + std::shared_ptr data; + mapbox::base::WeakPtrFactory weakFactory{this}; }; } // namespace mbgl diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index cc2dbfced8e..d4bf1e07527 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -4,15 +4,16 @@ #include #include -#include +#include #include +#include #include -#include #include #include -#include -#include +#include +#include #include +#include #include @@ -43,6 +44,29 @@ class GeoJSONTileTest { }; }; +namespace { + +class FakeGeoJSONData : public GeoJSONData { +public: + FakeGeoJSONData(TileFeatures features_) : features(std::move(features_)) {} + + void getTile(const CanonicalTileID&, const std::function& fn) final { + assert(fn); + fn(features); + } + + Features getChildren(const std::uint32_t) final { return {}; } + + Features getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { return {}; } + + std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; } + +private: + TileFeatures features; +}; + +} // namespace + TEST(GeoJSONTile, Issue7648) { GeoJSONTileTest test; @@ -50,8 +74,8 @@ TEST(GeoJSONTile, Issue7648) { mapbox::feature::feature_collection features; features.push_back(mapbox::feature::feature { mapbox::geometry::point(0, 0) }); - - GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features); + auto data = std::make_shared(std::move(features)); + GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, data); Immutable layerProperties = makeMutable(staticImmutableCast(layer.baseImpl)); StubTileObserver observer; observer.tileChanged = [&] (const Tile&) { @@ -68,7 +92,7 @@ TEST(GeoJSONTile, Issue7648) { test.loop.runOnce(); } - tile.updateData(features); + tile.updateData(data); while (!tile.isComplete()) { test.loop.runOnce(); } @@ -83,8 +107,8 @@ TEST(GeoJSONTile, Issue9927) { mapbox::feature::feature_collection features; features.push_back(mapbox::feature::feature { mapbox::geometry::point(0, 0) }); - - GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features); + auto data = std::make_shared(std::move(features)); + GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, data); Immutable layerProperties = makeMutable(staticImmutableCast(layer.baseImpl)); std::vector> layers { layerProperties }; From 5043a239d5e4dda7450e9645de5df53f69044f26 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Fri, 22 Nov 2019 13:58:21 +0200 Subject: [PATCH 2/3] [core] Introduce GeometryTile::reset() The newly introduced `GeometryTile::reset()` is used while GeoJSON tile update in order to prevent from the parsing of the new data with the stale layers or vice verse. --- src/mbgl/tile/geojson_tile.cpp | 21 +++++++++++---------- src/mbgl/tile/geojson_tile.hpp | 2 +- src/mbgl/tile/geometry_tile.cpp | 13 +++++++++++-- src/mbgl/tile/geometry_tile.hpp | 5 ++++- src/mbgl/tile/geometry_tile_worker.cpp | 18 ++++++++++++++++-- src/mbgl/tile/geometry_tile_worker.hpp | 2 +- 6 files changed, 44 insertions(+), 17 deletions(-) diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp index a4c48fb809b..65a9d80aec4 100644 --- a/src/mbgl/tile/geojson_tile.cpp +++ b/src/mbgl/tile/geojson_tile.cpp @@ -10,20 +10,21 @@ GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID, const TileParameters& parameters, std::shared_ptr data_) : GeometryTile(overscaledTileID, sourceID_, parameters) { - updateData(std::move(data_)); + updateData(std::move(data_), false /*needsRelayout*/); } -void GeoJSONTile::updateData(std::shared_ptr data_, bool resetLayers) { +void GeoJSONTile::updateData(std::shared_ptr data_, bool needsRelayout) { assert(data_); data = std::move(data_); - data->getTile(id.canonical, - [this, self = weakFactory.makeWeakPtr(), capturedData = data.get(), resetLayers]( - style::GeoJSONData::TileFeatures features) { - if (!self) return; - if (data.get() != capturedData) return; - auto tileData = std::make_unique(std::move(features)); - setData(std::move(tileData), resetLayers); - }); + if (needsRelayout) reset(); + data->getTile( + id.canonical, + [this, self = weakFactory.makeWeakPtr(), capturedData = data.get()](style::GeoJSONData::TileFeatures features) { + if (!self) return; + if (data.get() != capturedData) return; + auto tileData = std::make_unique(std::move(features)); + setData(std::move(tileData)); + }); } void GeoJSONTile::querySourceFeatures( diff --git a/src/mbgl/tile/geojson_tile.hpp b/src/mbgl/tile/geojson_tile.hpp index 26dc300bdc3..240b23e9bce 100644 --- a/src/mbgl/tile/geojson_tile.hpp +++ b/src/mbgl/tile/geojson_tile.hpp @@ -17,7 +17,7 @@ class GeoJSONTile : public GeometryTile { const TileParameters&, std::shared_ptr); - void updateData(std::shared_ptr data, bool resetLayers = false); + void updateData(std::shared_ptr data, bool needsRelayout = false); void querySourceFeatures( std::vector& result, diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 4a3d91455ab..7df81fa30f6 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -176,14 +176,23 @@ void GeometryTile::setError(std::exception_ptr err) { observer->onTileError(*this, err); } -void GeometryTile::setData(std::unique_ptr data_, bool resetLayers) { +void GeometryTile::setData(std::unique_ptr data_) { // Mark the tile as pending again if it was complete before to prevent signaling a complete // state despite pending parse operations. pending = true; ++correlationID; worker.self().invoke( - &GeometryTileWorker::setData, std::move(data_), imageManager.getAvailableImages(), resetLayers, correlationID); + &GeometryTileWorker::setData, std::move(data_), imageManager.getAvailableImages(), correlationID); +} + +void GeometryTile::reset() { + // Mark the tile as pending again if it was complete before to prevent signaling a complete + // state despite pending parse operations. + pending = true; + + ++correlationID; + worker.self().invoke(&GeometryTileWorker::reset, correlationID); } std::unique_ptr GeometryTile::createRenderData() { diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 566b359547e..cd771b109f8 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -34,7 +34,10 @@ class GeometryTile : public Tile, public GlyphRequestor, public ImageRequestor { ~GeometryTile() override; void setError(std::exception_ptr); - void setData(std::unique_ptr, bool resetLayers = false); + void setData(std::unique_ptr); + // Resets the tile's data and layers and leaves the tile in pending state, waiting for the new + // data and layers to come. + void reset(); std::unique_ptr createRenderData() override; void setLayers(const std::vector>&) override; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index de53c69aee2..a61321b3636 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -118,13 +118,11 @@ GeometryTileWorker::~GeometryTileWorker() = default; void GeometryTileWorker::setData(std::unique_ptr data_, std::set availableImages_, - bool resetLayers_, uint64_t correlationID_) { try { data = std::move(data_); correlationID = correlationID_; availableImages = std::move(availableImages_); - if (resetLayers_) layers = nullopt; switch (state) { case Idle: @@ -170,6 +168,22 @@ void GeometryTileWorker::setLayers(std::vector> layer } } +void GeometryTileWorker::reset(uint64_t correlationID_) { + layers = nullopt; + data = nullopt; + correlationID = correlationID_; + + switch (state) { + case Idle: + case NeedsParse: + break; + case Coalescing: + case NeedsSymbolLayout: + state = NeedsParse; + break; + } +} + void GeometryTileWorker::setShowCollisionBoxes(bool showCollisionBoxes_, uint64_t correlationID_) { try { showCollisionBoxes = showCollisionBoxes_; diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp index 38f7cb9fa6f..08dafc67888 100644 --- a/src/mbgl/tile/geometry_tile_worker.hpp +++ b/src/mbgl/tile/geometry_tile_worker.hpp @@ -43,8 +43,8 @@ class GeometryTileWorker { uint64_t correlationID); void setData(std::unique_ptr, std::set availableImages, - bool resetLayers, uint64_t correlationID); + void reset(uint64_t correlationID_); void setShowCollisionBoxes(bool showCollisionBoxes_, uint64_t correlationID_); void onGlyphsAvailable(GlyphMap glyphs); From da21ebe2dc474874aa1c1898230d877925420914 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Thu, 28 Nov 2019 14:54:54 +0200 Subject: [PATCH 3/3] [core] GeoJSONVTData uses Scheduler::GetSequenced() --- .../style/sources/geojson_source_impl.cpp | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index e4b48223f42..9a883ba96de 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -13,17 +13,19 @@ namespace mbgl { namespace style { -class GeoJSONVTData : public GeoJSONData { +class GeoJSONVTData : public GeoJSONData, public std::enable_shared_from_this { public: - GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options) - : impl(geoJSON, options) { - } - void getTile(const CanonicalTileID& id, const std::function& fn) final { assert(fn); - // It's safe to pass `this` as scheduler will die earlier. - scheduler.scheduleAndReplyValue( - [id, this]() -> TileFeatures { return impl.getTile(id.z, id.x, id.y).features; }, fn); + std::weak_ptr weak = shared_from_this(); + scheduler->scheduleAndReplyValue( + [id, weak, this]() -> TileFeatures { + if (auto self = weak.lock()) { + return impl.getTile(id.z, id.x, id.y).features; + } + return {}; + }, + fn); } Features getChildren(const std::uint32_t) final { return {}; } @@ -35,15 +37,15 @@ class GeoJSONVTData : public GeoJSONData { } private: + friend GeoJSONData; + GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options) + : impl(geoJSON, options), scheduler(Scheduler::GetSequenced()) {} mapbox::geojsonvt::GeoJSONVT impl; - SequencedScheduler scheduler; + std::shared_ptr scheduler; }; class SuperclusterData : public GeoJSONData { public: - SuperclusterData(const Features& features, const mapbox::supercluster::Options& options) - : impl(features, options) {} - void getTile(const CanonicalTileID& id, const std::function& fn) final { assert(fn); fn(impl.getTile(id.z, id.x, id.y)); @@ -60,6 +62,9 @@ class SuperclusterData : public GeoJSONData { } private: + friend GeoJSONData; + SuperclusterData(const Features& features, const mapbox::supercluster::Options& options) + : impl(features, options) {} mapbox::supercluster::Supercluster impl; }; @@ -105,7 +110,7 @@ std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutab toReturn[p.first] = evaluateFeature(*feature, p.second.second, accumulated); } }; - return std::make_shared(geoJSON.get(), clusterOptions); + return std::shared_ptr(new SuperclusterData(geoJSON.get(), clusterOptions)); } mapbox::geojsonvt::Options vtOptions; @@ -114,7 +119,7 @@ std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutab vtOptions.buffer = ::round(scale * options->buffer); vtOptions.tolerance = scale * options->tolerance; vtOptions.lineMetrics = options->lineMetrics; - return std::make_shared(geoJSON, vtOptions); + return std::shared_ptr(new GeoJSONVTData(geoJSON, vtOptions)); } GeoJSONSource::Impl::Impl(std::string id_, Immutable options_)