From 74b73e4f7917bb254b038e48edc32bb680cd71a2 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 29 Nov 2024 18:23:51 +0100 Subject: [PATCH 1/2] MVT: emit warning when the maximum tile size or feature count is reached and the user didn't explicitly set MAX_SIZE or MAX_FEATURES layer creation options CPL_DEBUG=ON already warns about that, but better be more verbose when the user didn't explicitly set limitations. Should help addressing https://mastodon.social/@RegisHaubourg/113567187835337586 --- autotest/ogr/ogr_mvt.py | 39 +++++++++++++++++--- ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp | 53 ++++++++++++++++++++++----- 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/autotest/ogr/ogr_mvt.py b/autotest/ogr/ogr_mvt.py index 3a6ad1dde9f8..ce652b661c0a 100755 --- a/autotest/ogr/ogr_mvt.py +++ b/autotest/ogr/ogr_mvt.py @@ -1220,7 +1220,9 @@ def test_ogr_mvt_write_mbtiles(): @pytest.mark.require_driver("SQLite") @pytest.mark.require_geos -def test_ogr_mvt_write_limitations_max_size(): +@pytest.mark.parametrize("implicit_limitation", [True, False]) +@gdaltest.enable_exceptions() +def test_ogr_mvt_write_limitations_max_size(implicit_limitation, tmp_path): src_ds = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown) lyr = src_ds.CreateLayer("mylayer") @@ -1244,10 +1246,22 @@ def test_ogr_mvt_write_limitations_max_size(): out_ds = gdal.VectorTranslate( "/vsimem/out.mbtiles", src_ds, - datasetCreationOptions=["MAX_SIZE=100", "SIMPLIFICATION=1"], + datasetCreationOptions=[ + "@MAX_SIZE_FOR_TEST=101" if implicit_limitation else "MAX_SIZE=101", + "SIMPLIFICATION=1", + ], ) assert out_ds is not None - out_ds = None + gdal.ErrorReset() + with gdal.quiet_errors(): + out_ds.Close() + if implicit_limitation: + assert ( + gdal.GetLastErrorMsg() + == "At least one tile exceeded the default maximum tile size of 101 bytes and was encoded at lower resolution" + ) + else: + assert gdal.GetLastErrorMsg() == "" out_ds = ogr.Open("/vsimem/out.mbtiles") assert out_ds is not None @@ -1441,7 +1455,9 @@ def test_ogr_mvt_write_limitations_max_size_polygon(): @pytest.mark.require_driver("SQLite") @pytest.mark.require_geos -def test_ogr_mvt_write_limitations_max_features(): +@pytest.mark.parametrize("implicit_limitation", [True, False]) +@gdaltest.enable_exceptions() +def test_ogr_mvt_write_limitations_max_features(implicit_limitation): src_ds = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown) lyr = src_ds.CreateLayer("mylayer") @@ -1462,10 +1478,21 @@ def test_ogr_mvt_write_limitations_max_features(): "/vsimem/out.mbtiles", src_ds, format="MVT", - datasetCreationOptions=["MAX_FEATURES=1"], + datasetCreationOptions=[ + "@MAX_FEATURES_FOR_TEST=1" if implicit_limitation else "MAX_FEATURES=1" + ], ) assert out_ds is not None - out_ds = None + gdal.ErrorReset() + with gdal.quiet_errors(): + out_ds.Close() + if implicit_limitation: + assert ( + gdal.GetLastErrorMsg() + == "At least one tile exceeded the default maximum number of features per tile (1) and was truncated to satisfy it." + ) + else: + assert gdal.GetLastErrorMsg() == "" out_ds = ogr.Open("/vsimem/out.mbtiles") assert out_ds is not None diff --git a/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp b/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp index 5b2cfbfd5069..62ac39d7a469 100644 --- a/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp +++ b/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp @@ -3301,6 +3301,8 @@ class OGRMVTWriterDataset final : public GDALDataset CPLString m_osType{"overlay"}; sqlite3 *m_hDBMBTILES = nullptr; OGREnvelope m_oEnvelope; + bool m_bMaxTileSizeOptSpecified = false; + bool m_bMaxFeaturesOptSpecified = false; unsigned m_nMaxTileSize = 500000; unsigned m_nMaxFeatures = 200000; std::map m_oMapLayerNameToDesc; @@ -4972,11 +4974,28 @@ std::string OGRMVTWriterDataset::EncodeTile( const double dfCompressionRatio = static_cast(nSizeAfter) / nSizeBefore; + const bool bTooManyFeatures = nFeaturesInTile >= m_nMaxFeatures; + if (bTooManyFeatures && !m_bMaxFeaturesOptSpecified) + { + m_bMaxFeaturesOptSpecified = true; + CPLError(CE_Warning, CPLE_AppDefined, + "At least one tile exceeded the default maximum number of " + "features per tile (%u) and was truncated to satisfy it.", + m_nMaxFeatures); + } + // If the tile size is above the allowed values or there are too many // features, then sort by descending area / length until we get to the // limit. bool bTooBigTile = oTileBuffer.size() > m_nMaxTileSize; - const bool bTooManyFeatures = nFeaturesInTile >= m_nMaxFeatures; + if (bTooBigTile && !m_bMaxTileSizeOptSpecified) + { + m_bMaxTileSizeOptSpecified = true; + CPLError(CE_Warning, CPLE_AppDefined, + "At least one tile exceeded the default maximum tile size of " + "%u bytes and was encoded at lower resolution", + m_nMaxTileSize); + } GUInt32 nExtent = m_nExtent; while (bTooBigTile && !bTooManyFeatures && nExtent >= 256) @@ -6087,14 +6106,30 @@ GDALDataset *OGRMVTWriterDataset::Create(const char *pszFilename, int nXSize, poDS->m_nBuffer = static_cast(atoi(CSLFetchNameValueDef( papszOptions, "BUFFER", CPLSPrintf("%u", 5 * poDS->m_nExtent / 256)))); - poDS->m_nMaxTileSize = - std::max(100U, static_cast(atoi(CSLFetchNameValueDef( - papszOptions, "MAX_SIZE", - CPLSPrintf("%u", poDS->m_nMaxTileSize))))); - poDS->m_nMaxFeatures = - std::max(1U, static_cast(atoi(CSLFetchNameValueDef( - papszOptions, "MAX_FEATURES", - CPLSPrintf("%u", poDS->m_nMaxFeatures))))); + { + const char *pszMaxSize = CSLFetchNameValue(papszOptions, "MAX_SIZE"); + poDS->m_bMaxTileSizeOptSpecified = pszMaxSize != nullptr; + pszMaxSize = CSLFetchNameValueDef(papszOptions, "@MAX_SIZE_FOR_TEST", + pszMaxSize); + if (pszMaxSize) + { + poDS->m_nMaxTileSize = + std::max(100U, static_cast(atoi(pszMaxSize))); + } + } + + { + const char *pszMaxFeatures = + CSLFetchNameValue(papszOptions, "MAX_FEATURES"); + poDS->m_bMaxFeaturesOptSpecified = pszMaxFeatures != nullptr; + pszMaxFeatures = CSLFetchNameValueDef( + papszOptions, "@MAX_FEATURES_FOR_TEST", pszMaxFeatures); + if (pszMaxFeatures) + { + poDS->m_nMaxFeatures = + std::max(1U, static_cast(atoi(pszMaxFeatures))); + } + } poDS->m_osName = CSLFetchNameValueDef(papszOptions, "NAME", CPLGetBasename(pszFilename)); From 503d62bfa14f796923d9c5774792d6d39cbaf61d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 2 Dec 2024 14:19:36 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Alessandro Pasotti --- ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp b/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp index 62ac39d7a469..9ab77fc4d3ce 100644 --- a/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp +++ b/ogr/ogrsf_frmts/mvt/ogrmvtdataset.cpp @@ -6109,6 +6109,7 @@ GDALDataset *OGRMVTWriterDataset::Create(const char *pszFilename, int nXSize, { const char *pszMaxSize = CSLFetchNameValue(papszOptions, "MAX_SIZE"); poDS->m_bMaxTileSizeOptSpecified = pszMaxSize != nullptr; + // This is used by unit tests pszMaxSize = CSLFetchNameValueDef(papszOptions, "@MAX_SIZE_FOR_TEST", pszMaxSize); if (pszMaxSize) @@ -6123,6 +6124,7 @@ GDALDataset *OGRMVTWriterDataset::Create(const char *pszFilename, int nXSize, CSLFetchNameValue(papszOptions, "MAX_FEATURES"); poDS->m_bMaxFeaturesOptSpecified = pszMaxFeatures != nullptr; pszMaxFeatures = CSLFetchNameValueDef( + // This is used by unit tests papszOptions, "@MAX_FEATURES_FOR_TEST", pszMaxFeatures); if (pszMaxFeatures) {