From 69e9605c0c5e903453cd1acd5fa861d3b9b5d561 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 04:06:15 +0200 Subject: [PATCH 01/14] OGRCurvePolygon::addRing(): take const pointer --- ogr/ogr_geometry.h | 2 +- ogr/ogrcurvepolygon.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ogr/ogr_geometry.h b/ogr/ogr_geometry.h index 3a957c5e8e08..42144dce1419 100644 --- a/ogr/ogr_geometry.h +++ b/ogr/ogr_geometry.h @@ -2577,7 +2577,7 @@ class CPL_DLL OGRCurvePolygon : public OGRSurface virtual void assignSpatialReference(const OGRSpatialReference *poSR) override; - virtual OGRErr addRing(OGRCurve *); + virtual OGRErr addRing(const OGRCurve *); virtual OGRErr addRingDirectly(OGRCurve *); OGRErr addRing(std::unique_ptr); diff --git a/ogr/ogrcurvepolygon.cpp b/ogr/ogrcurvepolygon.cpp index af8d744d4150..206745d38a0b 100644 --- a/ogr/ogrcurvepolygon.cpp +++ b/ogr/ogrcurvepolygon.cpp @@ -342,7 +342,7 @@ OGRErr OGRCurvePolygon::removeRing(int iIndex, bool bDelete) * @return OGRERR_NONE in case of success */ -OGRErr OGRCurvePolygon::addRing(OGRCurve *poNewRing) +OGRErr OGRCurvePolygon::addRing(const OGRCurve *poNewRing) { OGRCurve *poNewRingCloned = poNewRing->clone(); From 682c8f7b63fc3a914aa25a6751d95cdc752c9742 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 04:06:44 +0200 Subject: [PATCH 02/14] [Lint] OSM: const'ify and smartptr'ify --- ogr/ogrsf_frmts/osm/ogr_osm.h | 37 ++--- ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp | 177 ++++++++++------------- ogr/ogrsf_frmts/osm/ogrosmlayer.cpp | 3 +- 3 files changed, 101 insertions(+), 116 deletions(-) diff --git a/ogr/ogrsf_frmts/osm/ogr_osm.h b/ogr/ogrsf_frmts/osm/ogr_osm.h index 5407175d58a5..7985caca6397 100644 --- a/ogr/ogrsf_frmts/osm/ogr_osm.h +++ b/ogr/ogrsf_frmts/osm/ogr_osm.h @@ -269,8 +269,8 @@ class OGROSMLayer final : public OGRLayer } void SetFieldsFromTags(OGRFeature *poFeature, GIntBig nID, bool bIsWayID, - unsigned int nTags, OSMTag *pasTags, - OSMInfo *psInfo); + unsigned int nTags, const OSMTag *pasTags, + const OSMInfo *psInfo); void SetDeclareInterest(bool bIn) { @@ -522,30 +522,31 @@ class OGROSMDataSource final : public OGRDataSource static const GIntBig FILESIZE_INVALID = -1; GIntBig m_nFileSize = FILESIZE_NOT_INIT; - void CompressWay(bool bIsArea, unsigned int nTags, IndexedKVP *pasTags, - int nPoints, LonLat *pasLonLatPairs, OSMInfo *psInfo, + void CompressWay(bool bIsArea, unsigned int nTags, + const IndexedKVP *pasTags, int nPoints, + const LonLat *pasLonLatPairs, const OSMInfo *psInfo, std::vector &abyCompressedWay); void UncompressWay(int nBytes, const GByte *pabyCompressedWay, bool *pbIsArea, std::vector &asCoords, unsigned int *pnTags, OSMTag *pasTags, OSMInfo *psInfo); - bool ParseConf(char **papszOpenOptions); + bool ParseConf(CSLConstList papszOpenOptions); bool CreateTempDB(); bool SetDBOptions(); void SetCacheSize(); bool CreatePreparedStatements(); void CloseDB(); - bool IndexPoint(OSMNode *psNode); - bool IndexPointSQLite(OSMNode *psNode); + bool IndexPoint(const OSMNode *psNode); + bool IndexPointSQLite(const OSMNode *psNode); bool FlushCurrentSector(); bool FlushCurrentSectorCompressedCase(); bool FlushCurrentSectorNonCompressedCase(); - bool IndexPointCustom(OSMNode *psNode); + bool IndexPointCustom(const OSMNode *psNode); void IndexWay(GIntBig nWayID, bool bIsArea, unsigned int nTags, - IndexedKVP *pasTags, LonLat *pasLonLatPairs, int nPairs, - OSMInfo *psInfo); + const IndexedKVP *pasTags, const LonLat *pasLonLatPairs, + int nPairs, const OSMInfo *psInfo); bool StartTransactionCacheDB(); bool CommitTransactionCacheDB(); @@ -563,12 +564,12 @@ class OGROSMDataSource final : public OGRDataSource unsigned int LookupWays(std::map> &aoMapWays, - OSMRelation *psRelation); + const OSMRelation *psRelation); - OGRGeometry *BuildMultiPolygon(OSMRelation *psRelation, + OGRGeometry *BuildMultiPolygon(const OSMRelation *psRelation, unsigned int *pnTags, OSMTag *pasTags); - OGRGeometry *BuildGeometryCollection(OSMRelation *psRelation, - int bMultiLineString); + OGRGeometry *BuildGeometryCollection(const OSMRelation *psRelation, + bool bMultiLineString); bool TransferToDiskIfNecesserary(); @@ -610,7 +611,7 @@ class OGROSMDataSource final : public OGRDataSource GDALProgressFunc pfnProgress, void *pProgressData) override; - int Open(const char *pszFilename, char **papszOpenOptions); + int Open(const char *pszFilename, CSLConstList papszOpenOptions); int MyResetReading(); bool ParseNextChunk(int nIdxLayer, GDALProgressFunc pfnProgress, @@ -618,9 +619,9 @@ class OGROSMDataSource final : public OGRDataSource OGRErr GetExtent(OGREnvelope *psExtent); int IsInterleavedReading(); - void NotifyNodes(unsigned int nNodes, OSMNode *pasNodes); - void NotifyWay(OSMWay *psWay); - void NotifyRelation(OSMRelation *psRelation); + void NotifyNodes(unsigned int nNodes, const OSMNode *pasNodes); + void NotifyWay(const OSMWay *psWay); + void NotifyRelation(const OSMRelation *psRelation); void NotifyBounds(double dfXMin, double dfYMin, double dfXMax, double dfYMax); diff --git a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp index a00d31dab42d..e421b9640111 100644 --- a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp +++ b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp @@ -317,21 +317,20 @@ OGROSMDataSource::~OGROSMDataSource() } CPLFree(m_pabySector); - std::map::iterator oIter = m_oMapBuckets.begin(); - for (; oIter != m_oMapBuckets.end(); ++oIter) + for (auto &oIter : m_oMapBuckets) { if (m_bCompressNodes) { int nRem = - oIter->first % (knPAGE_SIZE / BUCKET_SECTOR_SIZE_ARRAY_SIZE); + oIter.first % (knPAGE_SIZE / BUCKET_SECTOR_SIZE_ARRAY_SIZE); if (nRem == 0) - CPLFree(oIter->second.u.panSectorSize); + CPLFree(oIter.second.u.panSectorSize); } else { - int nRem = oIter->first % (knPAGE_SIZE / BUCKET_BITMAP_SIZE); + int nRem = oIter.first % (knPAGE_SIZE / BUCKET_BITMAP_SIZE); if (nRem == 0) - CPLFree(oIter->second.u.pabyBitmap); + CPLFree(oIter.second.u.pabyBitmap); } } } @@ -408,7 +407,7 @@ constexpr GByte abyBitsCount[] = { 4, 5, 5, 6, 5, 6, 6, 7, 3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7, 4, 5, 5, 6, 5, 6, 6, 7, 5, 6, 6, 7, 6, 7, 7, 8}; -bool OGROSMDataSource::IndexPoint(OSMNode *psNode) +bool OGROSMDataSource::IndexPoint(const OSMNode *psNode) { if (!m_bIndexPoints) return true; @@ -423,7 +422,7 @@ bool OGROSMDataSource::IndexPoint(OSMNode *psNode) /* IndexPointSQLite() */ /************************************************************************/ -bool OGROSMDataSource::IndexPointSQLite(OSMNode *psNode) +bool OGROSMDataSource::IndexPointSQLite(const OSMNode *psNode) { sqlite3_bind_int64(m_hInsertNodeStmt, 1, psNode->nID); @@ -517,7 +516,7 @@ Bucket *OGROSMDataSource::AllocBucket(int iBucket) Bucket *OGROSMDataSource::GetBucket(int nBucketId) { - std::map::iterator oIter = m_oMapBuckets.find(nBucketId); + auto oIter = m_oMapBuckets.find(nBucketId); if (oIter == m_oMapBuckets.end()) { Bucket *psBucket = &m_oMapBuckets[nBucketId]; @@ -638,7 +637,7 @@ bool OGROSMDataSource::FlushCurrentSectorNonCompressedCase() /* IndexPointCustom() */ /************************************************************************/ -bool OGROSMDataSource::IndexPointCustom(OSMNode *psNode) +bool OGROSMDataSource::IndexPointCustom(const OSMNode *psNode) { if (psNode->nID <= m_nPrevNodeId) { @@ -720,7 +719,7 @@ bool OGROSMDataSource::IndexPointCustom(OSMNode *psNode) /* NotifyNodes() */ /************************************************************************/ -void OGROSMDataSource::NotifyNodes(unsigned int nNodes, OSMNode *pasNodes) +void OGROSMDataSource::NotifyNodes(unsigned int nNodes, const OSMNode *pasNodes) { const OGREnvelope *psEnvelope = m_apoLayers[IDX_LYR_POINTS]->GetSpatialFilterEnvelope(); @@ -742,7 +741,7 @@ void OGROSMDataSource::NotifyNodes(unsigned int nNodes, OSMNode *pasNodes) continue; bool bInterestingTag = m_bReportAllNodes; - OSMTag *pasTags = pasNodes[i].pasTags; + const OSMTag *pasTags = pasNodes[i].pasTags; if (!m_bReportAllNodes) { @@ -1025,8 +1024,7 @@ void OGROSMDataSource::LookupNodesCustom() int nOffInBucket = static_cast(id % NODE_PER_BUCKET); int nOffInBucketReduced = nOffInBucket >> NODE_PER_SECTOR_SHIFT; - std::map::const_iterator oIter = - m_oMapBuckets.find(nBucket); + const auto oIter = m_oMapBuckets.find(nBucket); if (oIter == m_oMapBuckets.end()) continue; const Bucket *psBucket = &(oIter->second); @@ -1109,8 +1107,7 @@ void OGROSMDataSource::LookupNodesCustomCompressedCase() if (nOffInBucketReduced != l_nOffInBucketReducedOld) { - std::map::const_iterator oIter = - m_oMapBuckets.find(nBucket); + const auto oIter = m_oMapBuckets.find(nBucket); if (oIter == m_oMapBuckets.end()) { CPLError(CE_Failure, CPLE_AppDefined, @@ -1223,8 +1220,7 @@ void OGROSMDataSource::LookupNodesCustomNonCompressedCase() if (psBucket == nullptr || nBucket != l_nBucketOld) { - std::map::const_iterator oIter = - m_oMapBuckets.find(nBucket); + const auto oIter = m_oMapBuckets.find(nBucket); if (oIter == m_oMapBuckets.end()) { CPLError(CE_Failure, CPLE_AppDefined, @@ -1383,8 +1379,9 @@ static void WriteVarSInt64(GIntBig nSVal, GByte **ppabyData) /************************************************************************/ void OGROSMDataSource::CompressWay(bool bIsArea, unsigned int nTags, - IndexedKVP *pasTags, int nPoints, - LonLat *pasLonLatPairs, OSMInfo *psInfo, + const IndexedKVP *pasTags, int nPoints, + const LonLat *pasLonLatPairs, + const OSMInfo *psInfo, std::vector &abyCompressedWay) { abyCompressedWay.clear(); @@ -1560,9 +1557,9 @@ void OGROSMDataSource::UncompressWay(int nBytes, const GByte *pabyCompressedWay, /************************************************************************/ void OGROSMDataSource::IndexWay(GIntBig nWayID, bool bIsArea, - unsigned int nTags, IndexedKVP *pasTags, - LonLat *pasLonLatPairs, int nPairs, - OSMInfo *psInfo) + unsigned int nTags, const IndexedKVP *pasTags, + const LonLat *pasLonLatPairs, int nPairs, + const OSMInfo *psInfo) { if (!m_bIndexWays) return; @@ -1732,7 +1729,7 @@ void OGROSMDataSource::ProcessWaysBatch() OGRGeometry *poGeom = poLS; const int nPoints = static_cast(m_asLonLatCache.size()); - poLS->setNumPoints(nPoints); + poLS->setNumPoints(nPoints, /*bZeroizeNewContent=*/false); for (int i = 0; i < nPoints; i++) { poLS->setPoint(i, INT_TO_DBL(m_asLonLatCache[i].nLon), @@ -1860,7 +1857,7 @@ bool OGROSMDataSource::IsClosedWayTaggedAsPolygon(unsigned int nTags, /* NotifyWay() */ /************************************************************************/ -void OGROSMDataSource::NotifyWay(OSMWay *psWay) +void OGROSMDataSource::NotifyWay(const OSMWay *psWay) { m_nWaysProcessed++; if (m_nWaysProcessed % 10000 == 0) @@ -2158,7 +2155,7 @@ static void OGROSMNotifyWay(OSMWay *psWay, OSMContext * /* psOSMContext */, unsigned int OGROSMDataSource::LookupWays( std::map> &aoMapWays, - OSMRelation *psRelation) + const OSMRelation *psRelation) { unsigned int nFound = 0; unsigned int iCur = 0; @@ -2224,7 +2221,7 @@ unsigned int OGROSMDataSource::LookupWays( /* BuildMultiPolygon() */ /************************************************************************/ -OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, +OGRGeometry *OGROSMDataSource::BuildMultiPolygon(const OSMRelation *psRelation, unsigned int *pnTags, OSMTag *pasTags) { @@ -2260,9 +2257,8 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, return nullptr; } - OGRMultiLineString *poMLS = new OGRMultiLineString(); - OGRGeometry **papoPolygons = static_cast( - CPLMalloc(sizeof(OGRGeometry *) * psRelation->nMembers)); + OGRMultiLineString oMLS; + std::vector apoPolygons(psRelation->nMembers); int nPolys = 0; if (pnTags != nullptr) @@ -2305,7 +2301,7 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, OGRPolygon *poPoly = new OGRPolygon(); OGRLinearRing *poRing = new OGRLinearRing(); poPoly->addRingDirectly(poRing); - papoPolygons[nPolys++] = poPoly; + apoPolygons[nPolys++] = poPoly; poLS = poRing; if (strcmp(psRelation->pasMembers[i].pszRole, "outer") == 0) @@ -2320,11 +2316,11 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, else { poLS = new OGRLineString(); - poMLS->addGeometryDirectly(poLS); + oMLS.addGeometryDirectly(poLS); } const int nPoints = static_cast(m_asLonLatCache.size()); - poLS->setNumPoints(nPoints); + poLS->setNumPoints(nPoints, /*bZeroizeNewContent=*/false); for (int j = 0; j < nPoints; j++) { poLS->setPoint(j, INT_TO_DBL(m_asLonLatCache[j].nLon), @@ -2333,21 +2329,16 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, } } - if (poMLS->getNumGeometries() > 0) + if (oMLS.getNumGeometries() > 0) { - OGRGeometryH hPoly = OGRBuildPolygonFromEdges( - OGRGeometry::ToHandle(poMLS), TRUE, FALSE, 0, nullptr); - if (hPoly != nullptr && OGR_G_GetGeometryType(hPoly) == wkbPolygon) + auto poPolyFromEdges = std::unique_ptr( + OGRGeometry::FromHandle(OGRBuildPolygonFromEdges( + OGRGeometry::ToHandle(&oMLS), TRUE, FALSE, 0, nullptr))); + if (poPolyFromEdges && poPolyFromEdges->getGeometryType() == wkbPolygon) { - OGRPolygon *poSuperPoly = - OGRGeometry::FromHandle(hPoly)->toPolygon(); - const unsigned nRings = 1 + static_cast( - poSuperPoly->getNumInteriorRings()); - for (unsigned int i = 0; i < nRings; i++) + const OGRPolygon *poSuperPoly = poPolyFromEdges->toPolygon(); + for (const OGRLinearRing *poRing : *poSuperPoly) { - OGRLinearRing *poRing = - (i == 0) ? poSuperPoly->getExteriorRing() - : poSuperPoly->getInteriorRing(i - 1); if (poRing != nullptr && poRing->getNumPoints() >= 4 && poRing->getX(0) == poRing->getX(poRing->getNumPoints() - 1) && @@ -2355,34 +2346,32 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, { OGRPolygon *poPoly = new OGRPolygon(); poPoly->addRing(poRing); - papoPolygons[nPolys++] = poPoly; + apoPolygons[nPolys++] = poPoly; } } } - - OGR_G_DestroyGeometry(hPoly); } - delete poMLS; - OGRGeometry *poRet = nullptr; + std::unique_ptr poRet; if (nPolys > 0) { int bIsValidGeometry = FALSE; const char *apszOptions[2] = {"METHOD=DEFAULT", nullptr}; - OGRGeometry *poGeom = OGRGeometryFactory::organizePolygons( - papoPolygons, nPolys, &bIsValidGeometry, apszOptions); + auto poGeom = + std::unique_ptr(OGRGeometryFactory::organizePolygons( + apoPolygons.data(), nPolys, &bIsValidGeometry, apszOptions)); - if (poGeom != nullptr && poGeom->getGeometryType() == wkbPolygon) + if (poGeom && poGeom->getGeometryType() == wkbPolygon) { - OGRMultiPolygon *poMulti = new OGRMultiPolygon(); - poMulti->addGeometryDirectly(poGeom); - poGeom = poMulti; + auto poMulti = std::make_unique(); + poMulti->addGeometryDirectly(poGeom.release()); + poGeom = std::move(poMulti); } - if (poGeom != nullptr && poGeom->getGeometryType() == wkbMultiPolygon) + if (poGeom && poGeom->getGeometryType() == wkbMultiPolygon) { - poRet = poGeom; + poRet = std::move(poGeom); } else { @@ -2390,35 +2379,33 @@ OGRGeometry *OGROSMDataSource::BuildMultiPolygon(OSMRelation *psRelation, "Relation " CPL_FRMT_GIB ": Geometry has incompatible type : %s", psRelation->nID, - poGeom != nullptr - ? OGR_G_GetGeometryName(OGRGeometry::ToHandle(poGeom)) - : "null"); - delete poGeom; + poGeom ? OGR_G_GetGeometryName( + OGRGeometry::ToHandle(poGeom.get())) + : "null"); } } - CPLFree(papoPolygons); - // cppcheck-suppress constVariableReference for (auto &oIter : aoMapWays) CPLFree(oIter.second.second); - return poRet; + return poRet.release(); } /************************************************************************/ /* BuildGeometryCollection() */ /************************************************************************/ -OGRGeometry *OGROSMDataSource::BuildGeometryCollection(OSMRelation *psRelation, - int bMultiLineString) +OGRGeometry * +OGROSMDataSource::BuildGeometryCollection(const OSMRelation *psRelation, + bool bMultiLineString) { std::map> aoMapWays; LookupWays(aoMapWays, psRelation); - OGRGeometryCollection *poColl = (bMultiLineString) - ? new OGRMultiLineString() - : new OGRGeometryCollection(); + std::unique_ptr poColl = + bMultiLineString ? std::make_unique() + : std::make_unique(); for (unsigned int i = 0; i < psRelation->nMembers; i++) { @@ -2460,7 +2447,7 @@ OGRGeometry *OGROSMDataSource::BuildGeometryCollection(OSMRelation *psRelation, } const int nPoints = static_cast(m_asLonLatCache.size()); - poLS->setNumPoints(nPoints); + poLS->setNumPoints(nPoints, /*bZeroizeNewContent=*/false); for (int j = 0; j < nPoints; j++) { poLS->setPoint(j, INT_TO_DBL(m_asLonLatCache[j].nLon), @@ -2471,22 +2458,21 @@ OGRGeometry *OGROSMDataSource::BuildGeometryCollection(OSMRelation *psRelation, if (poColl->getNumGeometries() == 0) { - delete poColl; - poColl = nullptr; + poColl.reset(); } // cppcheck-suppress constVariableReference for (auto &oIter : aoMapWays) CPLFree(oIter.second.second); - return poColl; + return poColl.release(); } /************************************************************************/ /* NotifyRelation() */ /************************************************************************/ -void OGROSMDataSource::NotifyRelation(OSMRelation *psRelation) +void OGROSMDataSource::NotifyRelation(const OSMRelation *psRelation) { if (!m_asWayFeaturePairs.empty()) ProcessWaysBatch(); @@ -2669,7 +2655,8 @@ void OGROSMDataSource::ProcessPolygonsStandalone() poPoly->addRingDirectly(poRing); OGRLineString *poLS = poRing; - poLS->setNumPoints(static_cast(m_asLonLatCache.size())); + poLS->setNumPoints(static_cast(m_asLonLatCache.size()), + /*bZeroizeNewContent=*/false); for (int j = 0; j < static_cast(m_asLonLatCache.size()); j++) { poLS->setPoint(j, INT_TO_DBL(m_asLonLatCache[j].nLon), @@ -2738,7 +2725,8 @@ static void OGROSMNotifyBounds(double dfXMin, double dfYMin, double dfXMax, /* Open() */ /************************************************************************/ -int OGROSMDataSource::Open(const char *pszFilename, char **papszOpenOptionsIn) +int OGROSMDataSource::Open(const char *pszFilename, + CSLConstList papszOpenOptionsIn) { m_pszName = CPLStrdup(pszFilename); @@ -2953,7 +2941,7 @@ int OGROSMDataSource::Open(const char *pszFilename, char **papszOpenOptionsIn) CPLString osInterestLayers = GetInterestLayersForDSName(GetName()); if (!osInterestLayers.empty()) { - delete ExecuteSQL(osInterestLayers, nullptr, nullptr); + ReleaseResultSet(ExecuteSQL(osInterestLayers, nullptr, nullptr)); } } return bRet; @@ -3371,13 +3359,12 @@ bool OGROSMDataSource::CommitTransactionCacheDB() void OGROSMDataSource::AddComputedAttributes( int iCurLayer, const std::vector &oAttributes) { - for (size_t i = 0; i < oAttributes.size(); i++) + for (const auto &oAttribute : oAttributes) { - if (!oAttributes[i].osSQL.empty()) + if (!oAttribute.osSQL.empty()) { - m_apoLayers[iCurLayer]->AddComputedAttribute(oAttributes[i].osName, - oAttributes[i].eType, - oAttributes[i].osSQL); + m_apoLayers[iCurLayer]->AddComputedAttribute( + oAttribute.osName, oAttribute.eType, oAttribute.osSQL); } } } @@ -3386,7 +3373,7 @@ void OGROSMDataSource::AddComputedAttributes( /* ParseConf() */ /************************************************************************/ -bool OGROSMDataSource::ParseConf(char **papszOpenOptionsIn) +bool OGROSMDataSource::ParseConf(CSLConstList papszOpenOptionsIn) { const char *pszFilename = CSLFetchNameValueDef(papszOpenOptionsIn, "CONFIG_FILE", @@ -3859,21 +3846,20 @@ int OGROSMDataSource::MyResetReading() memset(m_pabySector, 0, SECTOR_SIZE); - std::map::iterator oIter = m_oMapBuckets.begin(); - for (; oIter != m_oMapBuckets.end(); ++oIter) + for (auto &oIter : m_oMapBuckets) { - Bucket *psBucket = &(oIter->second); - psBucket->nOff = -1; + Bucket &sBucket = oIter.second; + sBucket.nOff = -1; if (m_bCompressNodes) { - if (psBucket->u.panSectorSize) - memset(psBucket->u.panSectorSize, 0, + if (sBucket.u.panSectorSize) + memset(sBucket.u.panSectorSize, 0, BUCKET_SECTOR_SIZE_ARRAY_SIZE); } else { - if (psBucket->u.pabyBitmap) - memset(psBucket->u.pabyBitmap, 0, BUCKET_BITMAP_SIZE); + if (sBucket.u.pabyBitmap) + memset(sBucket.u.pabyBitmap, 0, BUCKET_BITMAP_SIZE); } } } @@ -4489,12 +4475,9 @@ OGRLayer *OGROSMDataSource::ExecuteSQL(const char *pszSQLCommand, if (pszDialect != nullptr && EQUAL(pszDialect, "SQLITE")) { - std::set oSetLayers = - OGRSQLiteGetReferencedLayers(pszSQLCommand); - std::set::iterator oIter = oSetLayers.begin(); - for (; oIter != oSetLayers.end(); ++oIter) + const auto oSetLayers = OGRSQLiteGetReferencedLayers(pszSQLCommand); + for (const LayerDesc &oLayerDesc : oSetLayers) { - const LayerDesc &oLayerDesc = *oIter; if (oLayerDesc.osDSName.empty()) { if (bLayerAlreadyAdded) diff --git a/ogr/ogrsf_frmts/osm/ogrosmlayer.cpp b/ogr/ogrsf_frmts/osm/ogrosmlayer.cpp index aec8754b49a5..c590c98c18aa 100644 --- a/ogr/ogrsf_frmts/osm/ogrosmlayer.cpp +++ b/ogr/ogrsf_frmts/osm/ogrosmlayer.cpp @@ -576,7 +576,8 @@ static const char *GetValueOfTag(const char *pszKeyToSearch, unsigned int nTags, void OGROSMLayer::SetFieldsFromTags(OGRFeature *poFeature, GIntBig nID, bool bIsWayID, unsigned int nTags, - OSMTag *pasTags, OSMInfo *psInfo) + const OSMTag *pasTags, + const OSMInfo *psInfo) { if (!bIsWayID) { From 9508995c921d1f9d14e272c3851fe6b032694ca5 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 11:36:07 +0200 Subject: [PATCH 03/14] OSM: fix master regression, without much visible consequence but noisy debug messages --- ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp index e421b9640111..02c2a8b94ea6 100644 --- a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp +++ b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp @@ -2838,7 +2838,7 @@ int OGROSMDataSource::Open(const char *pszFilename, VSI_MALLOC_VERBOSE(MAX_ACCUMULATED_NODES * sizeof(GIntBig))); try { - m_asWayFeaturePairs.resize(MAX_DELAYED_FEATURES); + m_asWayFeaturePairs.reserve(MAX_DELAYED_FEATURES); } catch (const std::exception &) { From 0159028d9fd9054d9a7dfef623e72dc6aa57394d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 12:50:37 +0200 Subject: [PATCH 04/14] /vsimem/: remove useless bExtendFileAtNextWrite member --- port/cpl_vsi_mem.cpp | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/port/cpl_vsi_mem.cpp b/port/cpl_vsi_mem.cpp index a4ccaedd9266..c297c0b5180a 100644 --- a/port/cpl_vsi_mem.cpp +++ b/port/cpl_vsi_mem.cpp @@ -139,7 +139,6 @@ class VSIMemHandle final : public VSIVirtualHandle bool bUpdate = false; bool bEOF = false; bool m_bError = false; - bool bExtendFileAtNextWrite = false; VSIMemHandle() = default; ~VSIMemHandle() override; @@ -350,7 +349,6 @@ int VSIMemHandle::Seek(vsi_l_offset nOffset, int nWhence) nLength = poFile->nLength; } - bExtendFileAtNextWrite = false; if (nWhence == SEEK_CUR) { if (nOffset > INT_MAX) @@ -375,14 +373,6 @@ int VSIMemHandle::Seek(vsi_l_offset nOffset, int nWhence) bEOF = false; - if (m_nOffset > nLength) - { - if (bUpdate) // Writable files are zero-extended by seek past end. - { - bExtendFileAtNextWrite = true; - } - } - return 0; } @@ -495,14 +485,6 @@ size_t VSIMemHandle::Write(const void *pBuffer, size_t nSize, size_t nCount) const size_t nBytesToWrite = nSize * nCount; - if (bExtendFileAtNextWrite) - { - bExtendFileAtNextWrite = false; - CPL_EXCLUSIVE_LOCK oLock(poFile->m_oMutex); - if (!poFile->SetLength(nOffset)) - return 0; - } - { CPL_EXCLUSIVE_LOCK oLock(poFile->m_oMutex); From 1b1dc79752550e232ef1977aa18d4df182024935 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 12:51:04 +0200 Subject: [PATCH 05/14] /vsimem/: more efficient SetLength() --- port/cpl_vsi_mem.cpp | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/port/cpl_vsi_mem.cpp b/port/cpl_vsi_mem.cpp index c297c0b5180a..5cfd8be55714 100644 --- a/port/cpl_vsi_mem.cpp +++ b/port/cpl_vsi_mem.cpp @@ -265,13 +265,21 @@ bool VSIMemFile::SetLength(vsi_l_offset nNewLength) return false; } - const vsi_l_offset nNewAlloc = (nNewLength + nNewLength / 10) + 5000; + // If the first allocation is 1 MB or above, just take that value + // as the one to allocate + // Otherwise slightly reserve more to avoid too frequent reallocations. + const vsi_l_offset nNewAlloc = + (nAllocLength == 0 && nNewLength >= 1024 * 1024) + ? nNewLength + : nNewLength + nNewLength / 10 + 5000; GByte *pabyNewData = nullptr; if (static_cast(static_cast(nNewAlloc)) == nNewAlloc) { pabyNewData = static_cast( - VSIRealloc(pabyData, static_cast(nNewAlloc))); + nAllocLength == 0 + ? VSICalloc(1, static_cast(nNewAlloc)) + : VSIRealloc(pabyData, static_cast(nNewAlloc))); } if (pabyNewData == nullptr) { @@ -282,9 +290,14 @@ bool VSIMemFile::SetLength(vsi_l_offset nNewLength) return false; } - // Clear the new allocated part of the buffer. - memset(pabyNewData + nAllocLength, 0, - static_cast(nNewAlloc - nAllocLength)); + if (nAllocLength > 0) + { + // Clear the new allocated part of the buffer (only needed if + // there was already reserved memory, otherwise VSICalloc() has + // zeroized it already) + memset(pabyNewData + nAllocLength, 0, + static_cast(nNewAlloc - nAllocLength)); + } pabyData = pabyNewData; nAllocLength = nNewAlloc; @@ -560,8 +573,6 @@ int VSIMemHandle::Truncate(vsi_l_offset nNewSize) return -1; } - bExtendFileAtNextWrite = false; - CPL_EXCLUSIVE_LOCK oLock(poFile->m_oMutex); if (poFile->SetLength(nNewSize)) return 0; From bf3592e9df47748ccc7f39737723c25d11c49475 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 12:54:28 +0200 Subject: [PATCH 06/14] OSM: actually reserve memory for /vsimem/ temp files --- ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp index 02c2a8b94ea6..79ee6de286fa 100644 --- a/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp +++ b/ogr/ogrsf_frmts/osm/ogrosmdatasource.cpp @@ -2862,6 +2862,8 @@ int OGROSMDataSource::Open(const char *pszFilename, m_nMaxSizeForInMemoryDBInMB = atoi(CSLFetchNameValueDef( papszOpenOptionsIn, "MAX_TMPFILE_SIZE", CPLGetConfigOption("OSM_MAX_TMPFILE_SIZE", "100"))); + if (m_nMaxSizeForInMemoryDBInMB == 0) + m_nMaxSizeForInMemoryDBInMB = 1; GIntBig nSize = static_cast(m_nMaxSizeForInMemoryDBInMB) * 1024 * 1024; if (nSize < 0 || @@ -2892,14 +2894,13 @@ int OGROSMDataSource::Open(const char *pszFilename, } CPLPushErrorHandler(CPLQuietErrorHandler); - bool bSuccess = - VSIFSeekL(m_fpNodes, static_cast(nSize * 3 / 4), - SEEK_SET) == 0; + const bool bSuccess = + VSIFTruncateL(m_fpNodes, + static_cast(nSize * 3 / 4)) == 0; CPLPopErrorHandler(); if (bSuccess) { - VSIFSeekL(m_fpNodes, 0, SEEK_SET); VSIFTruncateL(m_fpNodes, 0); } else @@ -2978,14 +2979,14 @@ bool OGROSMDataSource::CreateTempDB() VSILFILE *fp = VSIFOpenL(m_osTmpDBName, "wb"); if (fp) { - GIntBig nSize = - static_cast(m_nMaxSizeForInMemoryDBInMB) * 1024 * 1024; + vsi_l_offset nSize = + static_cast(m_nMaxSizeForInMemoryDBInMB) * 1024 * + 1024; if (m_bCustomIndexing && m_bInMemoryNodesFile) nSize = nSize / 4; CPLPushErrorHandler(CPLQuietErrorHandler); - bSuccess = - VSIFSeekL(fp, static_cast(nSize), SEEK_SET) == 0; + bSuccess = VSIFTruncateL(fp, nSize) == 0; CPLPopErrorHandler(); if (bSuccess) From 76f97f7ddb5de8be49e3e000a748c345ac6990a4 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 21 Jul 2024 15:46:05 +0200 Subject: [PATCH 07/14] Arrow/Parquet/generic arrow: add write support for arrow.json extension Read support was added in 3.9.0 per 21c06b9d486. Now that the arrow.json canonical extension has been made official, also implement it on the write side: i.e. in the CreateField() method of the Arrow/Parquet drivers, and in OGRLayer::GetArrowSchema() when generating a Arrow schema from a OGR layer definition. --- autotest/ogr/ogr_mem.py | 42 +++++++++++++++++++ autotest/ogr/ogr_parquet.py | 24 +++++++++++ ogr/ogrsf_frmts/arrow/ogrfeatherlayer.cpp | 2 +- .../arrow_common/ograrrowlayer.hpp | 5 +-- .../arrow_common/ograrrowwriterlayer.hpp | 22 +++++++--- ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp | 18 ++++---- ogr/ogrsf_frmts/parquet/ogrparquetlayer.cpp | 2 +- 7 files changed, 96 insertions(+), 19 deletions(-) diff --git a/autotest/ogr/ogr_mem.py b/autotest/ogr/ogr_mem.py index a308eac2a1c9..0002d2c7487d 100755 --- a/autotest/ogr/ogr_mem.py +++ b/autotest/ogr/ogr_mem.py @@ -1464,6 +1464,18 @@ def test_ogr_mem_write_arrow(): field_def = ogr.FieldDefn("field_stringlist", ogr.OFTStringList) src_lyr.CreateField(field_def) + field_def = ogr.FieldDefn("field_json", ogr.OFTString) + field_def.SetSubType(ogr.OFSTJSON) + src_lyr.CreateField(field_def) + + field_def = ogr.FieldDefn("field_uuid", ogr.OFTString) + field_def.SetSubType(ogr.OFSTUUID) + src_lyr.CreateField(field_def) + + field_def = ogr.FieldDefn("field_with_width", ogr.OFTString) + field_def.SetWidth(10) + src_lyr.CreateField(field_def) + feat_def = src_lyr.GetLayerDefn() src_feature = ogr.Feature(feat_def) src_feature.SetField("field_bool", True) @@ -1485,6 +1497,9 @@ def test_ogr_mem_write_arrow(): src_feature.field_float32list = [1.5, -1.5] src_feature.field_reallist = [123.5, 567.0] src_feature.field_stringlist = ["abc", "def"] + src_feature["field_json"] = '{"foo":"bar"}' + src_feature["field_uuid"] = "INVALID_UUID" + src_feature["field_with_width"] = "foo" src_feature.SetGeometry(ogr.CreateGeometryFromWkt("POINT (1 2)")) src_lyr.CreateFeature(src_feature) @@ -1506,6 +1521,15 @@ def test_ogr_mem_write_arrow(): if schema.GetChild(i).GetName() != "wkb_geometry": dst_lyr.CreateFieldFromArrowSchema(schema.GetChild(i)) + idx = dst_lyr.GetLayerDefn().GetFieldIndex("field_json") + assert dst_lyr.GetLayerDefn().GetFieldDefn(idx).GetSubType() == ogr.OFSTJSON + + idx = dst_lyr.GetLayerDefn().GetFieldIndex("field_uuid") + assert dst_lyr.GetLayerDefn().GetFieldDefn(idx).GetSubType() == ogr.OFSTUUID + + idx = dst_lyr.GetLayerDefn().GetFieldIndex("field_with_width") + assert dst_lyr.GetLayerDefn().GetFieldDefn(idx).GetWidth() == 10 + while True: array = stream.GetNextRecordBatch() if array is None: @@ -2851,6 +2875,24 @@ def test_ogr_mem_write_pyarrow_invalid_dict_index(dict_values): lyr.WritePyArrow(table) +############################################################################### + + +def test_ogr_mem_arrow_json(): + pytest.importorskip("pyarrow") + + ds = ogr.GetDriverByName("Memory").CreateDataSource("") + lyr = ds.CreateLayer("foo") + field_def = ogr.FieldDefn("field_json", ogr.OFTString) + field_def.SetSubType(ogr.OFSTJSON) + lyr.CreateField(field_def) + + stream = lyr.GetArrowStreamAsPyArrow() + md = stream.schema["field_json"].metadata + assert b"ARROW:extension:name" in md + assert md[b"ARROW:extension:name"] == b"arrow.json" + + ############################################################################### # Test Layer.GetDataset() diff --git a/autotest/ogr/ogr_parquet.py b/autotest/ogr/ogr_parquet.py index 8d09e1c861b0..0d3d268cc2a7 100755 --- a/autotest/ogr/ogr_parquet.py +++ b/autotest/ogr/ogr_parquet.py @@ -4044,6 +4044,30 @@ def test_ogr_parquet_read_arrow_json_extension(): assert f["extension_json"] == '{"foo":"bar"}' +############################################################################### +# Test writing a file with the arrow.json extension + + +def test_ogr_parquet_writing_arrow_json_extension(tmp_vsimem): + + outfilename = str(tmp_vsimem / "out.parquet") + with ogr.GetDriverByName("Parquet").CreateDataSource(outfilename) as ds: + lyr = ds.CreateLayer("test") + fld_defn = ogr.FieldDefn("extension_json") + fld_defn.SetSubType(ogr.OFSTJSON) + lyr.CreateField(fld_defn) + f = ogr.Feature(lyr.GetLayerDefn()) + f["extension_json"] = '{"foo":"bar"}' + lyr.CreateFeature(f) + + with gdal.config_option("OGR_PARQUET_READ_GDAL_SCHEMA", "NO"): + ds = ogr.Open(outfilename) + lyr = ds.GetLayer(0) + assert lyr.GetLayerDefn().GetFieldDefn(0).GetSubType() == ogr.OFSTJSON + f = lyr.GetNextFeature() + assert f["extension_json"] == '{"foo":"bar"}' + + ############################################################################### # Test ignored fields with arrow::dataset and bounding box column diff --git a/ogr/ogrsf_frmts/arrow/ogrfeatherlayer.cpp b/ogr/ogrsf_frmts/arrow/ogrfeatherlayer.cpp index a82639d33332..8d867d8bda3c 100644 --- a/ogr/ogrsf_frmts/arrow/ogrfeatherlayer.cpp +++ b/ogr/ogrsf_frmts/arrow/ogrfeatherlayer.cpp @@ -175,7 +175,7 @@ void OGRFeatherLayer::EstablishFeatureDefn() if (field_kv_metadata) { auto extension_name = - field_kv_metadata->Get("ARROW:extension:name"); + field_kv_metadata->Get(ARROW_EXTENSION_NAME_KEY); if (extension_name.ok()) { osExtensionName = *extension_name; diff --git a/ogr/ogrsf_frmts/arrow_common/ograrrowlayer.hpp b/ogr/ogrsf_frmts/arrow_common/ograrrowlayer.hpp index 51b71cf4b332..0ef2b1ec0a05 100644 --- a/ogr/ogrsf_frmts/arrow_common/ograrrowlayer.hpp +++ b/ogr/ogrsf_frmts/arrow_common/ograrrowlayer.hpp @@ -304,16 +304,13 @@ inline bool OGRArrowLayer::MapArrowTypeToOGR( } else if (const auto &field_kv_metadata = field->metadata()) { - auto extension_name = field_kv_metadata->Get("ARROW:extension:name"); + auto extension_name = field_kv_metadata->Get(ARROW_EXTENSION_NAME_KEY); if (extension_name.ok()) { osExtensionName = *extension_name; } } - // Preliminary/in-advance read support for future JSON Canonical Extension - // Cf https://github.com/apache/arrow/pull/41257 and - // https://github.com/apache/arrow/pull/13901 if (!osExtensionName.empty() && osExtensionName != EXTENSION_NAME_ARROW_JSON) { diff --git a/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp b/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp index bb0735fcb1db..20aed6fe967d 100644 --- a/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp +++ b/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp @@ -153,6 +153,7 @@ inline void OGRArrowWriterLayer::CreateSchemaCommon() { const auto poFieldDefn = m_poFeatureDefn->GetFieldDefn(i); std::shared_ptr dt; + const auto eDT = poFieldDefn->GetType(); const auto eSubDT = poFieldDefn->GetSubType(); const auto &osDomainName = poFieldDefn->GetDomainName(); const OGRFieldDomain *poFieldDomain = nullptr; @@ -172,7 +173,7 @@ inline void OGRArrowWriterLayer::CreateSchemaCommon() poFieldDomain = oIter->second.get(); } } - switch (poFieldDefn->GetType()) + switch (eDT) { case OFTInteger: if (eSubDT == OFSTBoolean) @@ -209,7 +210,7 @@ inline void OGRArrowWriterLayer::CreateSchemaCommon() case OFTString: case OFTWideString: - if (eSubDT != OFSTNone || nWidth > 0) + if ((eSubDT != OFSTNone && eSubDT != OFSTJSON) || nWidth > 0) bNeedGDALSchema = true; dt = arrow::utf8(); break; @@ -265,9 +266,18 @@ inline void OGRArrowWriterLayer::CreateSchemaCommon() break; } } - fields.emplace_back(arrow::field(poFieldDefn->GetNameRef(), - std::move(dt), - poFieldDefn->IsNullable())); + + auto field = arrow::field(poFieldDefn->GetNameRef(), std::move(dt), + poFieldDefn->IsNullable()); + if (eDT == OFTString && eSubDT == OFSTJSON) + { + auto kvMetadata = std::make_shared(); + kvMetadata->Append(ARROW_EXTENSION_NAME_KEY, + EXTENSION_NAME_ARROW_JSON); + field = field->WithMetadata(kvMetadata); + } + + fields.emplace_back(std::move(field)); if (poFieldDefn->GetAlternativeNameRef()[0]) bNeedGDALSchema = true; if (!poFieldDefn->GetComment().empty()) @@ -389,7 +399,7 @@ inline void OGRArrowWriterLayer::CreateSchemaCommon() ? field->metadata()->Copy() : std::make_shared(); kvMetadata->Append( - "ARROW:extension:name", + ARROW_EXTENSION_NAME_KEY, GetGeomEncodingAsString(m_aeGeomEncoding[i], false)); field = field->WithMetadata(kvMetadata); } diff --git a/ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp b/ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp index 085610b02356..b6f9a8ba1a61 100644 --- a/ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp +++ b/ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp @@ -419,9 +419,10 @@ int OGRLayer::GetArrowSchema(struct ArrowArrayStream *, psChild->name = CPLStrdup(poFieldDefn->GetNameRef()); if (poFieldDefn->IsNullable()) psChild->flags = ARROW_FLAG_NULLABLE; + const auto eType = poFieldDefn->GetType(); const auto eSubType = poFieldDefn->GetSubType(); const char *item_format = nullptr; - switch (poFieldDefn->GetType()) + switch (eType) { case OFTInteger: { @@ -585,15 +586,18 @@ int OGRLayer::GetArrowSchema(struct ArrowArrayStream *, if (!osComment.empty()) oMetadata.emplace_back(std::pair(MD_GDAL_OGR_COMMENT, osComment)); - if (poFieldDefn->GetSubType() != OFSTNone && - poFieldDefn->GetSubType() != OFSTBoolean && - poFieldDefn->GetSubType() != OFSTFloat32) + if (eType == OFTString && eSubType == OFSTJSON) { oMetadata.emplace_back( - std::pair(MD_GDAL_OGR_SUBTYPE, - OGR_GetFieldSubTypeName(poFieldDefn->GetSubType()))); + std::pair(ARROW_EXTENSION_NAME_KEY, EXTENSION_NAME_ARROW_JSON)); + } + else if (eSubType != OFSTNone && eSubType != OFSTBoolean && + eSubType != OFSTFloat32) + { + oMetadata.emplace_back(std::pair( + MD_GDAL_OGR_SUBTYPE, OGR_GetFieldSubTypeName(eSubType))); } - if (poFieldDefn->GetType() == OFTString && poFieldDefn->GetWidth() > 0) + if (eType == OFTString && poFieldDefn->GetWidth() > 0) { oMetadata.emplace_back(std::pair( MD_GDAL_OGR_WIDTH, CPLSPrintf("%d", poFieldDefn->GetWidth()))); diff --git a/ogr/ogrsf_frmts/parquet/ogrparquetlayer.cpp b/ogr/ogrsf_frmts/parquet/ogrparquetlayer.cpp index e0c33f578788..d001813141e0 100644 --- a/ogr/ogrsf_frmts/parquet/ogrparquetlayer.cpp +++ b/ogr/ogrsf_frmts/parquet/ogrparquetlayer.cpp @@ -212,7 +212,7 @@ bool OGRParquetLayerBase::DealWithGeometryColumn( std::string osExtensionName; if (field_kv_metadata) { - auto extension_name = field_kv_metadata->Get("ARROW:extension:name"); + auto extension_name = field_kv_metadata->Get(ARROW_EXTENSION_NAME_KEY); if (extension_name.ok()) { osExtensionName = *extension_name; From fbecd0bd4fe988bd4db1a4b601e49a96e1f1eca7 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 21 Jul 2024 19:35:46 +0200 Subject: [PATCH 08/14] OGRLineString::SetPoint(): avoid int overflow if passing iPoint = INT_MAX --- autotest/ogr/ogr_geom.py | 40 ++++++++++++++-------------------------- ogr/ogrlinestring.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/autotest/ogr/ogr_geom.py b/autotest/ogr/ogr_geom.py index df26bc55037a..39cd29a1db86 100755 --- a/autotest/ogr/ogr_geom.py +++ b/autotest/ogr/ogr_geom.py @@ -982,50 +982,38 @@ def test_ogr_geom_flattenTo2D_triangle(): ############################################################################### +@gdaltest.enable_exceptions() def test_ogr_geom_linestring_limits(): geom = ogr.CreateGeometryFromWkt("LINESTRING EMPTY") assert geom.Length() == 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.GetPoint(-1) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.GetPoint(0) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.GetPoint_2D(-1) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.GetPoint_2D(0) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.SetPoint(-1, 5, 6, 7) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): + with pytest.raises(Exception): geom.SetPoint_2D(-1, 5, 6) - assert gdal.GetLastErrorType() != 0 - gdal.ErrorReset() - with gdal.quiet_errors(): - geom.SetPoint(2147000000, 5, 6, 7) - assert gdal.GetLastErrorType() != 0 + with pytest.raises(Exception): + geom.SetPoint((1 << 31) - 2, 5, 6, 7) - gdal.ErrorReset() - with gdal.quiet_errors(): - geom.SetPoint_2D(2147000000, 5, 6) - assert gdal.GetLastErrorType() != 0 + with pytest.raises(Exception): + geom.SetPoint_2D((1 << 31) - 2, 5, 6) + + with pytest.raises(Exception): + geom.SetPoint_2D((1 << 31) - 1, 5, 6) geom = ogr.CreateGeometryFromWkt("LINESTRING(0 0)") assert geom.Length() == 0 diff --git a/ogr/ogrlinestring.cpp b/ogr/ogrlinestring.cpp index 9d59218fb9a4..f3e155eac89f 100644 --- a/ogr/ogrlinestring.cpp +++ b/ogr/ogrlinestring.cpp @@ -530,6 +530,20 @@ void OGRSimpleCurve::setPoint(int iPoint, OGRPoint *poPoint) setPoint(iPoint, poPoint->getX(), poPoint->getY()); } +/************************************************************************/ +/* CheckPointCount() */ +/************************************************************************/ + +static inline bool CheckPointCount(int iPoint) +{ + if (iPoint == std::numeric_limits::max()) + { + CPLError(CE_Failure, CPLE_AppDefined, "Too big point count."); + return false; + } + return true; +} + /************************************************************************/ /* setPoint() */ /************************************************************************/ @@ -557,6 +571,8 @@ void OGRSimpleCurve::setPoint(int iPoint, double xIn, double yIn, double zIn) if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1) return; @@ -598,6 +614,8 @@ void OGRSimpleCurve::setPointM(int iPoint, double xIn, double yIn, double mIn) if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1) return; @@ -643,6 +661,8 @@ void OGRSimpleCurve::setPoint(int iPoint, double xIn, double yIn, double zIn, if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1) return; @@ -684,6 +704,8 @@ void OGRSimpleCurve::setPoint(int iPoint, double xIn, double yIn) { if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1 || paoPoints == nullptr) return; @@ -717,6 +739,8 @@ void OGRSimpleCurve::setZ(int iPoint, double zIn) if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1) return; @@ -750,6 +774,8 @@ void OGRSimpleCurve::setM(int iPoint, double mIn) if (iPoint >= nPointCount) { + if (!CheckPointCount(iPoint)) + return; setNumPoints(iPoint + 1); if (nPointCount < iPoint + 1) return; From 5e6bcc9733f3b87cac01e1093faa242e74009cc2 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 21 Jul 2024 19:36:32 +0200 Subject: [PATCH 09/14] OGRCurveCollection::addCurveDirectly(): avoid int overflow if adding to a already huge collection --- ogr/ogrcurvecollection.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/ogr/ogrcurvecollection.cpp b/ogr/ogrcurvecollection.cpp index e17429c45d6a..36d6466dd4da 100644 --- a/ogr/ogrcurvecollection.cpp +++ b/ogr/ogrcurvecollection.cpp @@ -31,6 +31,7 @@ #include #include +#include #include #include "ogr_core.h" @@ -155,6 +156,21 @@ OGRErr OGRCurveCollection::addCurveDirectly(OGRGeometry *poGeom, if (bNeedRealloc) { +#if SIZEOF_VOIDP < 8 + if (nCurveCount == std::numeric_limits::max() / + static_cast(sizeof(OGRCurve *))) + { + CPLError(CE_Failure, CPLE_OutOfMemory, "Too many subgeometries"); + return OGRERR_FAILURE; + } +#else + if (nCurveCount == std::numeric_limits::max()) + { + CPLError(CE_Failure, CPLE_AppDefined, "Too many subgeometries"); + return OGRERR_FAILURE; + } +#endif + OGRCurve **papoNewCurves = static_cast(VSI_REALLOC_VERBOSE( papoCurves, sizeof(OGRCurve *) * (nCurveCount + 1))); if (papoNewCurves == nullptr) From a04dfddd39f415b5d8fa17714fa2f435ff388d97 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 21 Jul 2024 19:37:20 +0200 Subject: [PATCH 10/14] OGRGeometryCollection::addGeometryDirectly(): avoid int overflow if adding to a already huge collection --- ogr/ogrgeometrycollection.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ogr/ogrgeometrycollection.cpp b/ogr/ogrgeometrycollection.cpp index cd0ea336bad5..ab931a04f5eb 100644 --- a/ogr/ogrgeometrycollection.cpp +++ b/ogr/ogrgeometrycollection.cpp @@ -361,6 +361,21 @@ OGRErr OGRGeometryCollection::addGeometryDirectly(OGRGeometry *poNewGeom) if (!isCompatibleSubType(poNewGeom->getGeometryType())) return OGRERR_UNSUPPORTED_GEOMETRY_TYPE; +#if SIZEOF_VOIDP < 8 + if (nGeomCount == std::numeric_limits::max() / + static_cast(sizeof(OGRGeometry *))) + { + CPLError(CE_Failure, CPLE_OutOfMemory, "Too many subgeometries"); + return OGRERR_FAILURE; + } +#else + if (nGeomCount == std::numeric_limits::max()) + { + CPLError(CE_Failure, CPLE_AppDefined, "Too many subgeometries"); + return OGRERR_FAILURE; + } +#endif + HomogenizeDimensionalityWith(poNewGeom); OGRGeometry **papoNewGeoms = static_cast( From 25549d895ff9800f105056259e0863a70c85da5e Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 21 Jul 2024 19:37:40 +0200 Subject: [PATCH 11/14] ogrgeometrycollection.cpp: some cleanups --- ogr/ogrgeometrycollection.cpp | 95 ++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 47 deletions(-) diff --git a/ogr/ogrgeometrycollection.cpp b/ogr/ogrgeometrycollection.cpp index ab931a04f5eb..0b9c3e90d404 100644 --- a/ogr/ogrgeometrycollection.cpp +++ b/ogr/ogrgeometrycollection.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include "cpl_conv.h" @@ -71,7 +72,7 @@ OGRGeometryCollection::OGRGeometryCollection(const OGRGeometryCollection &other) { // Do not use addGeometry() as it is virtual. papoGeoms = static_cast( - VSI_CALLOC_VERBOSE(sizeof(void *), other.nGeomCount)); + VSI_CALLOC_VERBOSE(sizeof(OGRGeometry *), other.nGeomCount)); if (papoGeoms) { nGeomCount = other.nGeomCount; @@ -114,9 +115,9 @@ OGRGeometryCollection::operator=(const OGRGeometryCollection &other) OGRGeometry::operator=(other); - for (int i = 0; i < other.nGeomCount; i++) + for (const auto &poSubGeom : other) { - addGeometry(other.papoGeoms[i]); + addGeometry(poSubGeom); } } return *this; @@ -131,7 +132,7 @@ void OGRGeometryCollection::empty() { if (papoGeoms != nullptr) { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { delete poSubGeom; } @@ -179,7 +180,7 @@ int OGRGeometryCollection::getDimension() const int nDimension = 0; // FIXME? Not sure if it is really appropriate to take the max in case // of geometries of different dimension. - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { int nSubGeomDimension = poSubGeom->getDimension(); if (nSubGeomDimension > nDimension) @@ -199,7 +200,7 @@ int OGRGeometryCollection::getDimension() const void OGRGeometryCollection::flattenTo2D() { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->flattenTo2D(); } @@ -378,8 +379,9 @@ OGRErr OGRGeometryCollection::addGeometryDirectly(OGRGeometry *poNewGeom) HomogenizeDimensionalityWith(poNewGeom); - OGRGeometry **papoNewGeoms = static_cast( - VSI_REALLOC_VERBOSE(papoGeoms, sizeof(void *) * (nGeomCount + 1))); + OGRGeometry **papoNewGeoms = + static_cast(VSI_REALLOC_VERBOSE( + papoGeoms, sizeof(OGRGeometry *) * (nGeomCount + 1))); if (papoNewGeoms == nullptr) return OGRERR_FAILURE; @@ -461,7 +463,7 @@ OGRErr OGRGeometryCollection::removeGeometry(int iGeom, int bDelete) delete papoGeoms[iGeom]; memmove(papoGeoms + iGeom, papoGeoms + iGeom + 1, - sizeof(void *) * (nGeomCount - iGeom - 1)); + sizeof(OGRGeometry *) * (nGeomCount - iGeom - 1)); nGeomCount--; @@ -474,9 +476,9 @@ OGRErr OGRGeometryCollection::removeGeometry(int iGeom, int bDelete) bool OGRGeometryCollection::hasEmptyParts() const { - for (int i = 0; i < nGeomCount; ++i) + for (const auto &poSubGeom : *this) { - if (papoGeoms[i]->IsEmpty() || papoGeoms[i]->hasEmptyParts()) + if (poSubGeom->IsEmpty() || poSubGeom->hasEmptyParts()) return true; } return false; @@ -551,7 +553,7 @@ OGRErr OGRGeometryCollection::importFromWkbInternal( // coverity[tainted_data] papoGeoms = static_cast( - VSI_CALLOC_VERBOSE(sizeof(void *), nGeomCount)); + VSI_CALLOC_VERBOSE(sizeof(OGRGeometry *), nGeomCount)); if (nGeomCount != 0 && papoGeoms == nullptr) { nGeomCount = 0; @@ -901,11 +903,10 @@ std::string OGRGeometryCollection::exportToWktInternal( try { - for (int i = 0; i < nGeomCount; ++i) + for (const auto &poSubGeom : *this) { - OGRGeometry *geom = papoGeoms[i]; OGRErr subgeomErr = OGRERR_NONE; - std::string tempWkt = geom->exportToWkt(opts, &subgeomErr); + std::string tempWkt = poSubGeom->exportToWkt(opts, &subgeomErr); if (subgeomErr != OGRERR_NONE) { if (err) @@ -991,7 +992,7 @@ void OGRGeometryCollection::getEnvelope(OGREnvelope3D *psEnvelope) const bool bExtentSet = false; *psEnvelope = OGREnvelope3D(); - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { if (!poSubGeom->IsEmpty()) { @@ -1052,7 +1053,7 @@ OGRErr OGRGeometryCollection::transform(OGRCoordinateTransformation *poCT) { int iGeom = 0; - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { const OGRErr eErr = poSubGeom->transform(poCT); if (eErr != OGRERR_NONE) @@ -1084,7 +1085,7 @@ OGRErr OGRGeometryCollection::transform(OGRCoordinateTransformation *poCT) void OGRGeometryCollection::closeRings() { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { if (OGR_GT_IsSubClassOf(wkbFlatten(poSubGeom->getGeometryType()), wkbCurvePolygon)) @@ -1102,7 +1103,7 @@ void OGRGeometryCollection::closeRings() void OGRGeometryCollection::setCoordinateDimension(int nNewDimension) { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->setCoordinateDimension(nNewDimension); } @@ -1112,7 +1113,7 @@ void OGRGeometryCollection::setCoordinateDimension(int nNewDimension) void OGRGeometryCollection::set3D(OGRBoolean bIs3D) { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->set3D(bIs3D); } @@ -1122,7 +1123,7 @@ void OGRGeometryCollection::set3D(OGRBoolean bIs3D) void OGRGeometryCollection::setMeasured(OGRBoolean bIsMeasured) { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->setMeasured(bIsMeasured); } @@ -1149,7 +1150,7 @@ void OGRGeometryCollection::setMeasured(OGRBoolean bIsMeasured) double OGRGeometryCollection::get_Length() const { double dfLength = 0.0; - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { const OGRwkbGeometryType eType = wkbFlatten(poSubGeom->getGeometryType()); @@ -1189,7 +1190,7 @@ double OGRGeometryCollection::get_Length() const double OGRGeometryCollection::get_Area() const { double dfArea = 0.0; - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { OGRwkbGeometryType eType = wkbFlatten(poSubGeom->getGeometryType()); if (OGR_GT_IsSurface(eType)) @@ -1249,7 +1250,7 @@ double OGRGeometryCollection::get_GeodesicArea( poSRSOverride = getSpatialReference(); double dfArea = 0.0; - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { OGRwkbGeometryType eType = wkbFlatten(poSubGeom->getGeometryType()); if (OGR_GT_IsSurface(eType)) @@ -1290,7 +1291,7 @@ double OGRGeometryCollection::get_GeodesicArea( OGRBoolean OGRGeometryCollection::IsEmpty() const { - for (auto &&poSubGeom : *this) + for (const auto &poSubGeom : *this) { if (poSubGeom->IsEmpty() == FALSE) return FALSE; @@ -1306,7 +1307,7 @@ void OGRGeometryCollection::assignSpatialReference( const OGRSpatialReference *poSR) { OGRGeometry::assignSpatialReference(poSR); - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->assignSpatialReference(poSR); } @@ -1318,7 +1319,7 @@ void OGRGeometryCollection::assignSpatialReference( void OGRGeometryCollection::segmentize(double dfMaxLength) { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->segmentize(dfMaxLength); } @@ -1330,7 +1331,7 @@ void OGRGeometryCollection::segmentize(double dfMaxLength) void OGRGeometryCollection::swapXY() { - for (auto &&poSubGeom : *this) + for (auto &poSubGeom : *this) { poSubGeom->swapXY(); } @@ -1360,9 +1361,9 @@ OGRBoolean OGRGeometryCollection::isCompatibleSubType( OGRBoolean OGRGeometryCollection::hasCurveGeometry(int bLookForNonLinear) const { - for (int iGeom = 0; iGeom < nGeomCount; iGeom++) + for (const auto &poSubGeom : *this) { - if (papoGeoms[iGeom]->hasCurveGeometry(bLookForNonLinear)) + if (poSubGeom->hasCurveGeometry(bLookForNonLinear)) return TRUE; } return FALSE; @@ -1376,19 +1377,20 @@ OGRGeometry * OGRGeometryCollection::getLinearGeometry(double dfMaxAngleStepSizeDegrees, const char *const *papszOptions) const { - OGRGeometryCollection *poGC = + auto poGC = std::unique_ptr( OGRGeometryFactory::createGeometry(OGR_GT_GetLinear(getGeometryType())) - ->toGeometryCollection(); - if (poGC == nullptr) + ->toGeometryCollection()); + if (!poGC) return nullptr; poGC->assignSpatialReference(getSpatialReference()); - for (int iGeom = 0; iGeom < nGeomCount; iGeom++) + for (const auto &poSubGeom : *this) { - OGRGeometry *poSubGeom = papoGeoms[iGeom]->getLinearGeometry( + OGRGeometry *poSubGeomNew = poSubGeom->getLinearGeometry( dfMaxAngleStepSizeDegrees, papszOptions); - poGC->addGeometryDirectly(poSubGeom); + if (poGC->addGeometryDirectly(poSubGeomNew) != OGRERR_NONE) + return nullptr; } - return poGC; + return poGC.release(); } /************************************************************************/ @@ -1398,27 +1400,26 @@ OGRGeometryCollection::getLinearGeometry(double dfMaxAngleStepSizeDegrees, OGRGeometry * OGRGeometryCollection::getCurveGeometry(const char *const *papszOptions) const { - OGRGeometryCollection *poGC = + auto poGC = std::unique_ptr( OGRGeometryFactory::createGeometry(OGR_GT_GetCurve(getGeometryType())) - ->toGeometryCollection(); - if (poGC == nullptr) + ->toGeometryCollection()); + if (!poGC) return nullptr; poGC->assignSpatialReference(getSpatialReference()); bool bHasCurveGeometry = false; - for (int iGeom = 0; iGeom < nGeomCount; iGeom++) + for (const auto &poSubGeom : *this) { - OGRGeometry *poSubGeom = - papoGeoms[iGeom]->getCurveGeometry(papszOptions); - if (poSubGeom->hasCurveGeometry()) + OGRGeometry *poSubGeomNew = poSubGeom->getCurveGeometry(papszOptions); + if (poSubGeomNew->hasCurveGeometry()) bHasCurveGeometry = true; - poGC->addGeometryDirectly(poSubGeom); + if (poGC->addGeometryDirectly(poSubGeomNew) != OGRERR_NONE) + return nullptr; } if (!bHasCurveGeometry) { - delete poGC; return clone(); } - return poGC; + return poGC.release(); } /************************************************************************/ From 172f0b0a38fd802ab83649971433e2b932e8f4cf Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sat, 20 Jul 2024 18:59:12 +0200 Subject: [PATCH 12/14] GTI: implement GetDataCoverageStatus() --- autotest/gdrivers/gti.py | 62 ++++++++++++ frmts/vrt/gdaltileindexdataset.cpp | 148 +++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) diff --git a/autotest/gdrivers/gti.py b/autotest/gdrivers/gti.py index 13066179ef91..8888103c4b7e 100755 --- a/autotest/gdrivers/gti.py +++ b/autotest/gdrivers/gti.py @@ -34,6 +34,7 @@ import struct import gdaltest +import ogrtest import pytest from osgeo import gdal, ogr @@ -1057,6 +1058,20 @@ def test_gti_rgb_left_right(tmp_vsimem): == "/vsimem/test_gti_rgb_left_right/left.tif" ) + if ogrtest.have_geos(): + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus( + 0, 0, vrt_ds.RasterXSize, vrt_ds.RasterYSize + ) + assert flags == gdal.GDAL_DATA_COVERAGE_STATUS_DATA and pct == 100.0 + + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus(1, 2, 3, 4) + assert flags == gdal.GDAL_DATA_COVERAGE_STATUS_DATA and pct == 100.0 + + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus( + vrt_ds.RasterXSize // 2 - 1, 2, 2, 4 + ) + assert flags == gdal.GDAL_DATA_COVERAGE_STATUS_DATA and pct == 100.0 + def test_gti_overlapping_sources(tmp_vsimem): @@ -1082,6 +1097,12 @@ def test_gti_overlapping_sources(tmp_vsimem): vrt_ds = gdal.Open(index_filename) assert vrt_ds.GetRasterBand(1).Checksum() == 2 + if ogrtest.have_geos(): + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus( + 0, 0, vrt_ds.RasterXSize, vrt_ds.RasterYSize + ) + assert flags == gdal.GDAL_DATA_COVERAGE_STATUS_DATA and pct == 100.0 + # Test unsupported sort_field_type = OFTBinary index_filename = str(tmp_vsimem / "index.gti.gpkg") sort_values = [None, None] @@ -1319,6 +1340,41 @@ def test_gti_overlapping_sources(tmp_vsimem): assert vrt_ds.GetRasterBand(1).Checksum() == 2, sort_values +def test_gti_gap_between_sources(tmp_vsimem): + + filename1 = str(tmp_vsimem / "one.tif") + ds = gdal.GetDriverByName("GTiff").Create(filename1, 1, 1) + ds.SetGeoTransform([2, 1, 0, 49, 0, -1]) + ds.GetRasterBand(1).Fill(1) + del ds + + filename2 = str(tmp_vsimem / "two.tif") + ds = gdal.GetDriverByName("GTiff").Create(filename2, 1, 1) + ds.SetGeoTransform([4, 1, 0, 49, 0, -1]) + ds.GetRasterBand(1).Fill(2) + del ds + + index_filename = str(tmp_vsimem / "index.gti.gpkg") + index_ds, _ = create_basic_tileindex( + index_filename, [gdal.Open(filename1), gdal.Open(filename2)] + ) + del index_ds + + vrt_ds = gdal.Open(index_filename) + assert vrt_ds.GetRasterBand(1).Checksum() == 3 + + if ogrtest.have_geos(): + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus( + 0, 0, vrt_ds.RasterXSize, vrt_ds.RasterYSize + ) + assert ( + flags + == gdal.GDAL_DATA_COVERAGE_STATUS_DATA + | gdal.GDAL_DATA_COVERAGE_STATUS_EMPTY + and pct == pytest.approx(100.0 * 2 / 3) + ) + + def test_gti_no_source(tmp_vsimem): index_filename = str(tmp_vsimem / "index.gti.gpkg") @@ -1359,6 +1415,12 @@ def test_gti_no_source(tmp_vsimem): is None ) + if ogrtest.have_geos(): + (flags, pct) = vrt_ds.GetRasterBand(1).GetDataCoverageStatus( + 0, 0, vrt_ds.RasterXSize, vrt_ds.RasterYSize + ) + assert flags == gdal.GDAL_DATA_COVERAGE_STATUS_EMPTY and pct == 0.0 + def test_gti_invalid_source(tmp_vsimem): diff --git a/frmts/vrt/gdaltileindexdataset.cpp b/frmts/vrt/gdaltileindexdataset.cpp index 122e7f782b98..97e5b9465127 100644 --- a/frmts/vrt/gdaltileindexdataset.cpp +++ b/frmts/vrt/gdaltileindexdataset.cpp @@ -370,6 +370,9 @@ class GDALTileIndexBand final : public GDALPamRasterBand GSpacing nLineSpace, GDALRasterIOExtraArg *psExtraArg) override; + int IGetDataCoverageStatus(int nXOff, int nYOff, int nXSize, int nYSize, + int nMaskFlagStop, double *pdfDataPct) override; + int GetMaskFlags() override { if (m_poDS->m_poMaskBand && m_poDS->m_poMaskBand.get() != this) @@ -2328,6 +2331,151 @@ CPLErr GDALTileIndexBand::IRasterIO(GDALRWFlag eRWFlag, int nXOff, int nYOff, nPixelSpace, nLineSpace, 0, psExtraArg); } +/************************************************************************/ +/* IGetDataCoverageStatus() */ +/************************************************************************/ + +#ifndef HAVE_GEOS +int GDALTileIndexBand::IGetDataCoverageStatus(int /* nXOff */, int /* nYOff */, + int /* nXSize */, + int /* nYSize */, + int /* nMaskFlagStop */, + double *pdfDataPct) +{ + if (pdfDataPct != nullptr) + *pdfDataPct = -1.0; + return GDAL_DATA_COVERAGE_STATUS_UNIMPLEMENTED | + GDAL_DATA_COVERAGE_STATUS_DATA; +} +#else +int GDALTileIndexBand::IGetDataCoverageStatus(int nXOff, int nYOff, int nXSize, + int nYSize, int nMaskFlagStop, + double *pdfDataPct) +{ + if (pdfDataPct != nullptr) + *pdfDataPct = -1.0; + + const double dfMinX = m_poDS->m_adfGeoTransform[GT_TOPLEFT_X] + + nXOff * m_poDS->m_adfGeoTransform[GT_WE_RES]; + const double dfMaxX = + dfMinX + nXSize * m_poDS->m_adfGeoTransform[GT_WE_RES]; + const double dfMaxY = m_poDS->m_adfGeoTransform[GT_TOPLEFT_Y] + + nYOff * m_poDS->m_adfGeoTransform[GT_NS_RES]; + const double dfMinY = + dfMaxY + nYSize * m_poDS->m_adfGeoTransform[GT_NS_RES]; + m_poDS->m_poLayer->SetSpatialFilterRect(dfMinX, dfMinY, dfMaxX, dfMaxY); + m_poDS->m_poLayer->ResetReading(); + + int nStatus = 0; + + auto poPolyNonCoveredBySources = std::make_unique(); + { + auto poLR = std::make_unique(); + poLR->addPoint(nXOff, nYOff); + poLR->addPoint(nXOff, nYOff + nYSize); + poLR->addPoint(nXOff + nXSize, nYOff + nYSize); + poLR->addPoint(nXOff + nXSize, nYOff); + poLR->addPoint(nXOff, nYOff); + poPolyNonCoveredBySources->addRingDirectly(poLR.release()); + } + while (true) + { + auto poFeature = + std::unique_ptr(m_poDS->m_poLayer->GetNextFeature()); + if (!poFeature) + break; + if (!poFeature->IsFieldSetAndNotNull(m_poDS->m_nLocationFieldIndex)) + { + continue; + } + + const auto poGeom = poFeature->GetGeometryRef(); + if (!poGeom || poGeom->IsEmpty()) + continue; + + OGREnvelope sSourceEnvelope; + poGeom->getEnvelope(&sSourceEnvelope); + + const double dfDstXOff = + std::max(nXOff, (sSourceEnvelope.MinX - + m_poDS->m_adfGeoTransform[GT_TOPLEFT_X]) / + m_poDS->m_adfGeoTransform[GT_WE_RES]); + const double dfDstXOff2 = std::min( + nXOff + nXSize, + (sSourceEnvelope.MaxX - m_poDS->m_adfGeoTransform[GT_TOPLEFT_X]) / + m_poDS->m_adfGeoTransform[GT_WE_RES]); + const double dfDstYOff = + std::max(nYOff, (sSourceEnvelope.MaxY - + m_poDS->m_adfGeoTransform[GT_TOPLEFT_Y]) / + m_poDS->m_adfGeoTransform[GT_NS_RES]); + const double dfDstYOff2 = std::min( + nYOff + nYSize, + (sSourceEnvelope.MinY - m_poDS->m_adfGeoTransform[GT_TOPLEFT_Y]) / + m_poDS->m_adfGeoTransform[GT_NS_RES]); + + // CPLDebug("GTI", "dfDstXOff=%f, dfDstXOff2=%f, dfDstYOff=%f, dfDstYOff2=%f", + // dfDstXOff, dfDstXOff2, dfDstYOff, dfDstXOff2); + + // Check if the AOI is fully inside the source + if (nXOff >= dfDstXOff && nYOff >= dfDstYOff && + nXOff + nXSize <= dfDstXOff2 && nYOff + nYSize <= dfDstYOff2) + { + if (pdfDataPct) + *pdfDataPct = 100.0; + return GDAL_DATA_COVERAGE_STATUS_DATA; + } + + // Check intersection of bounding boxes. + if (dfDstXOff2 > nXOff && dfDstYOff2 > nYOff && + dfDstXOff < nXOff + nXSize && dfDstYOff < nYOff + nYSize) + { + nStatus |= GDAL_DATA_COVERAGE_STATUS_DATA; + if (poPolyNonCoveredBySources) + { + OGRPolygon oPolySource; + auto poLR = std::make_unique(); + poLR->addPoint(dfDstXOff, dfDstYOff); + poLR->addPoint(dfDstXOff, dfDstYOff2); + poLR->addPoint(dfDstXOff2, dfDstYOff2); + poLR->addPoint(dfDstXOff2, dfDstYOff); + poLR->addPoint(dfDstXOff, dfDstYOff); + oPolySource.addRingDirectly(poLR.release()); + auto poRes = std::unique_ptr( + poPolyNonCoveredBySources->Difference(&oPolySource)); + if (poRes && poRes->IsEmpty()) + { + if (pdfDataPct) + *pdfDataPct = 100.0; + return GDAL_DATA_COVERAGE_STATUS_DATA; + } + else if (poRes && poRes->getGeometryType() == wkbPolygon) + { + poPolyNonCoveredBySources.reset( + poRes.release()->toPolygon()); + } + else + { + poPolyNonCoveredBySources.reset(); + } + } + } + if (nMaskFlagStop != 0 && (nStatus & nMaskFlagStop) != 0) + { + return nStatus; + } + } + if (poPolyNonCoveredBySources) + { + if (!poPolyNonCoveredBySources->IsEmpty()) + nStatus |= GDAL_DATA_COVERAGE_STATUS_EMPTY; + if (pdfDataPct) + *pdfDataPct = 100.0 * (1.0 - poPolyNonCoveredBySources->get_Area() / + nXSize / nYSize); + } + return nStatus; +} +#endif // HAVE_GEOS + /************************************************************************/ /* GetMetadataDomainList() */ /************************************************************************/ From b1b67c82ec3289f4da7dc0a4f3d109483f3a2f12 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 22 Jul 2024 16:33:19 +0200 Subject: [PATCH 13/14] Python bindings: make GetStatistics() and ComputeStatistics() return None in case of error (#10462) This was the originally intended behavior, but the IF_ERROR_RETURN_NONE typemap that controls this was actually broken. It used to work before 2006, but it was removed per commit 1c6e19f94f64888eadb02ed05d1465c673bb27b6 and later half restored in 86d4f1d47ae13a91e4950ac83d3ae898d499f291 , missing the important 'ret' typemap that was initially present. --- MIGRATION_GUIDE.TXT | 3 +++ autotest/gcore/gdal_stats.py | 12 ++++++------ autotest/gcore/pam.py | 2 +- autotest/gcore/vrt_read.py | 2 +- autotest/gdrivers/ecw.py | 2 +- autotest/gdrivers/ehdr.py | 6 +++--- autotest/gdrivers/netcdf_multidim.py | 14 ++------------ autotest/gdrivers/nitf.py | 3 +-- autotest/gdrivers/vrtprocesseddataset.py | 2 +- swig/include/python/typemaps_python.i | 9 +++++++++ .../gdal-utils/osgeo_utils/samples/gdalinfo.py | 5 ++--- swig/python/modify_cpp_files.cmake | 6 ------ 12 files changed, 30 insertions(+), 36 deletions(-) diff --git a/MIGRATION_GUIDE.TXT b/MIGRATION_GUIDE.TXT index c7d011677e4c..f1155642d493 100644 --- a/MIGRATION_GUIDE.TXT +++ b/MIGRATION_GUIDE.TXT @@ -15,6 +15,9 @@ MIGRATION GUIDE FROM GDAL 3.9 to GDAL 3.10 corresponding optional (but recommended to be implemented to reliably detect reading errors) callbacks "error" and "clear_err". +- Python bindings: Band.GetStatistics() and Band.ComputeStatistics() now + return a None value in case of error (when exceptions are not enabled) + MIGRATION GUIDE FROM GDAL 3.8 to GDAL 3.9 ----------------------------------------- diff --git a/autotest/gcore/gdal_stats.py b/autotest/gcore/gdal_stats.py index 523c6459621f..227d268d64de 100755 --- a/autotest/gcore/gdal_stats.py +++ b/autotest/gcore/gdal_stats.py @@ -99,7 +99,7 @@ def test_stats_dont_force(): gdal.Unlink("data/byte.tif.aux.xml") ds = gdal.Open("data/byte.tif") stats = ds.GetRasterBand(1).GetStatistics(0, 0) - assert stats == [0, 0, 0, -1], "did not get expected stats" + assert stats is None ############################################################################### @@ -762,7 +762,7 @@ def test_stats_approx_stats_flag(dt=gdal.GDT_Byte, struct_frmt="B"): approx_ok = 0 force = 0 stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force) - assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats" + assert stats is None approx_ok = 0 force = 1 @@ -824,15 +824,15 @@ def test_stats_clear(): filename = "/vsimem/out.tif" gdal.Translate(filename, "data/byte.tif") ds = gdal.Open(filename) - assert ds.GetRasterBand(1).GetStatistics(False, False) == [0, 0, 0, -1] - assert ds.GetRasterBand(1).ComputeStatistics(False) != [0, 0, 0, -1] + assert ds.GetRasterBand(1).GetStatistics(False, False) is None + assert ds.GetRasterBand(1).ComputeStatistics(False) is not None ds = gdal.Open(filename) - assert ds.GetRasterBand(1).GetStatistics(False, False) != [0, 0, 0, -1] + assert ds.GetRasterBand(1).GetStatistics(False, False) is not None ds.ClearStatistics() ds = gdal.Open(filename) - assert ds.GetRasterBand(1).GetStatistics(False, False) == [0, 0, 0, -1] + assert ds.GetRasterBand(1).GetStatistics(False, False) is None gdal.GetDriverByName("GTiff").Delete(filename) diff --git a/autotest/gcore/pam.py b/autotest/gcore/pam.py index 2b4403123f04..0af14cd16b38 100755 --- a/autotest/gcore/pam.py +++ b/autotest/gcore/pam.py @@ -382,7 +382,7 @@ def test_pam_11(): # Check that we actually have no saved statistics ds = gdal.Open("tmpdirreadonly/byte.tif") stats = ds.GetRasterBand(1).GetStatistics(False, False) - assert stats[3] == -1, "did not expected to have stats at that point" + assert stats is None ds = None # This must be run as an external process so we can override GDAL_PAM_PROXY_DIR diff --git a/autotest/gcore/vrt_read.py b/autotest/gcore/vrt_read.py index 00c068f165b4..6a67274a971d 100755 --- a/autotest/gcore/vrt_read.py +++ b/autotest/gcore/vrt_read.py @@ -2341,7 +2341,7 @@ def test_vrt_read_compute_statistics_mosaic_optimization_src_with_nodata_all(): with gdal.quiet_errors(): vrt_stats = vrt_ds.GetRasterBand(1).ComputeStatistics(False) - assert vrt_stats == [0, 0, 0, 0] + assert vrt_stats is None assert vrt_ds.GetRasterBand(1).GetMetadataItem("STATISTICS_MINIMUM") is None diff --git a/autotest/gdrivers/ecw.py b/autotest/gdrivers/ecw.py index 008057c1c11e..d0407441e7fd 100755 --- a/autotest/gdrivers/ecw.py +++ b/autotest/gdrivers/ecw.py @@ -1479,7 +1479,7 @@ def test_ecw_41(tmp_path): # Check that no statistics is already included in the file assert ds.GetRasterBand(1).GetMinimum() is None assert ds.GetRasterBand(1).GetMaximum() is None - assert ds.GetRasterBand(1).GetStatistics(1, 0) == [0.0, 0.0, 0.0, -1.0] + assert ds.GetRasterBand(1).GetStatistics(1, 0) is None assert ds.GetRasterBand(1).GetDefaultHistogram(force=0) is None # Now compute the stats diff --git a/autotest/gdrivers/ehdr.py b/autotest/gdrivers/ehdr.py index 8165192b8f55..6fb4b75b747f 100755 --- a/autotest/gdrivers/ehdr.py +++ b/autotest/gdrivers/ehdr.py @@ -379,20 +379,20 @@ def test_ehdr_approx_stats_flag(): approx_ok = 1 force = 1 stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force) - assert stats == [0.0, 0.0, 0.0, 0.0], "did not get expected stats" + assert stats == [0.0, 0.0, 0.0, 0.0] md = ds.GetRasterBand(1).GetMetadata() assert "STATISTICS_APPROXIMATE" in md, "did not get expected metadata" approx_ok = 0 force = 0 stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force) - assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats" + assert stats is None ds = gdal.Open(tmpfile, gdal.GA_Update) approx_ok = 0 force = 0 stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force) - assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats" + assert stats is None approx_ok = 0 force = 1 diff --git a/autotest/gdrivers/netcdf_multidim.py b/autotest/gdrivers/netcdf_multidim.py index bc9ea089c208..71585b3c04f5 100755 --- a/autotest/gdrivers/netcdf_multidim.py +++ b/autotest/gdrivers/netcdf_multidim.py @@ -3979,12 +3979,7 @@ def test(): view = ar.GetView("[0:10,...]") classic_ds = view.AsClassicDataset(1, 0) - assert classic_ds.GetRasterBand(1).GetStatistics(False, False) == [ - 0.0, - 0.0, - 0.0, - -1.0, - ] + assert classic_ds.GetRasterBand(1).GetStatistics(False, False) is None classic_ds.GetRasterBand(1).ComputeStatistics(False) view = ar.GetView("[10:20,...]") @@ -4035,12 +4030,7 @@ def reopen(): ) classic_ds = ar.AsClassicDataset(1, 0) - assert classic_ds.GetRasterBand(1).GetStatistics(False, False) == [ - 0.0, - 0.0, - 0.0, - -1.0, - ] + assert classic_ds.GetRasterBand(1).GetStatistics(False, False) is None rg_subset = rg.SubsetDimensionFromSelection("/x=440750") diff --git a/autotest/gdrivers/nitf.py b/autotest/gdrivers/nitf.py index a24070cbdc97..40909532991e 100755 --- a/autotest/gdrivers/nitf.py +++ b/autotest/gdrivers/nitf.py @@ -1488,8 +1488,7 @@ def test_nitf_36(): ds.GetRasterBand(1).GetMinimum() is None ), "Did not expect to have minimum value at that point." - (_, _, mean, stddev) = ds.GetRasterBand(1).GetStatistics(False, False) - assert stddev < 0, "Did not expect to have statistics at that point." + assert ds.GetRasterBand(1).GetStatistics(False, False) is None (exp_mean, exp_stddev) = (65.4208, 47.254550335) (_, _, mean, stddev) = ds.GetRasterBand(1).GetStatistics(False, True) diff --git a/autotest/gdrivers/vrtprocesseddataset.py b/autotest/gdrivers/vrtprocesseddataset.py index 1c353d170732..11033bf373f7 100755 --- a/autotest/gdrivers/vrtprocesseddataset.py +++ b/autotest/gdrivers/vrtprocesseddataset.py @@ -1181,7 +1181,7 @@ def test_vrtprocesseddataset_serialize(tmp_vsimem): with gdaltest.tempfile(vrt_filename, content): ds = gdal.Open(vrt_filename) np.testing.assert_equal(ds.GetRasterBand(1).ReadAsArray(), np.array([[11, 12]])) - assert ds.GetRasterBand(1).GetStatistics(False, False) == [0.0, 0.0, 0.0, -1.0] + assert ds.GetRasterBand(1).GetStatistics(False, False) is None ds.GetRasterBand(1).ComputeStatistics(False) ds.Close() diff --git a/swig/include/python/typemaps_python.i b/swig/include/python/typemaps_python.i index ef5276f3cc78..1a20779192c1 100644 --- a/swig/include/python/typemaps_python.i +++ b/swig/include/python/typemaps_python.i @@ -225,6 +225,15 @@ TYPEMAP_ARGOUT_ARGOUT_ARRAY_IS_VALID(6) /* %typemap(out) IF_ERROR_RETURN_NONE */ } +%typemap(ret) IF_ERROR_RETURN_NONE +{ + /* %typemap(ret) IF_ERROR_RETURN_NONE */ + if ($1 != CE_None ) { + Py_XDECREF( $result ); + $result = Py_None; + Py_INCREF($result); + } +} %import "ogr_error_map.i" diff --git a/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py b/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py index ed65ac82fb08..326b21ed6d2a 100644 --- a/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py +++ b/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py @@ -402,9 +402,8 @@ def main(argv=None): print(line) stats = hBand.GetStatistics(bApproxStats, bStats) - # Dirty hack to recognize if stats are valid. If invalid, the returned - # stddev is negative - if stats[3] >= 0.0: + # Before GDAL 3.10, a negative value for stddev indicated an error + if stats is not None and stats[3] >= 0.0: print( " Minimum=%.3f, Maximum=%.3f, Mean=%.3f, StdDev=%.3f" % (stats[0], stats[1], stats[2], stats[3]) diff --git a/swig/python/modify_cpp_files.cmake b/swig/python/modify_cpp_files.cmake index d5850c04abc9..af6668b0189e 100644 --- a/swig/python/modify_cpp_files.cmake +++ b/swig/python/modify_cpp_files.cmake @@ -33,12 +33,6 @@ string(REPLACE "obj = PyUnicode_AsUTF8String(obj);" "obj = PyUnicode_AsUTF8String(obj); if (!obj) return SWIG_TypeError;" _CONTENTS "${_CONTENTS}") -if("${FILE}" MATCHES "gdal_wrap.cpp") - string(REGEX REPLACE "result = \\(CPLErr\\)([^;]+)(\\;)" - [[CPL_IGNORE_RET_VAL(result = (CPLErr)\1)\2]] - _CONTENTS "${_CONTENTS}") -endif() - string(REPLACE "PyObject *resultobj = 0;" "PyObject *resultobj = 0; int bLocalUseExceptionsCode = GetUseExceptions();" _CONTENTS "${_CONTENTS}") From c9a07f515db9a12e4c7fe2eb639259e2f19835c8 Mon Sep 17 00:00:00 2001 From: Tom Augspurger Date: Mon, 22 Jul 2024 10:09:36 -0500 Subject: [PATCH 14/14] Update raster_data_model.rst --- doc/source/user/raster_data_model.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/user/raster_data_model.rst b/doc/source/user/raster_data_model.rst index e8e9d5bba92b..4800d7f713c4 100644 --- a/doc/source/user/raster_data_model.rst +++ b/doc/source/user/raster_data_model.rst @@ -136,7 +136,7 @@ The value of the _NAME is the string that can be passed to :cpp:func:`GDALOpen` Drivers which support subdatasets advertise the ``DMD_SUBDATASETS`` capability. This information is reported when the --format and --formats options are passed to the command line utilities. -Currently, drivers which support subdatasets are: ADRG, ECRGTOC, GEORASTER, GTiff, HDF4, HDF5, netCDF, NITF, NTv2, OGDI, PDF, PostGISRaster, Rasterlite, RPFTOC, RS2, TileDB, WCS, and WMS. +Currently, drivers which support subdatasets are: ADRG, ECRGTOC, GEORASTER, GTiff, HDF4, HDF5, netCDF, NITF, NTv2, OGDI, PDF, PostGISRaster, Rasterlite, RPFTOC, RS2, TileDB, WCS, WMS, and Zarr. IMAGE_STRUCTURE Domain ++++++++++++++++++++++