From 686c1ff8a6631a319a5aa933a1b0a0ef2614e3f9 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 8 Dec 2023 18:45:58 +0100 Subject: [PATCH] GPX: fix crash with GPX_ELE_AS_25D=YES config option (fixes qgis/QGIS#55558, master only); add open options --- autotest/ogr/ogr_gpx.py | 46 ++++++++++++++++++++++++ doc/source/drivers/vector/gpx.rst | 34 ++++++++++++++++++ ogr/ogrsf_frmts/gpx/ogr_gpx.h | 4 +-- ogr/ogrsf_frmts/gpx/ogrgpxdatasource.cpp | 22 +++++++----- ogr/ogrsf_frmts/gpx/ogrgpxdriver.cpp | 36 +++++++++++++++---- ogr/ogrsf_frmts/gpx/ogrgpxlayer.cpp | 18 ++++++---- 6 files changed, 138 insertions(+), 22 deletions(-) diff --git a/autotest/ogr/ogr_gpx.py b/autotest/ogr/ogr_gpx.py index 18d324411382..f9bff84c1dc7 100755 --- a/autotest/ogr/ogr_gpx.py +++ b/autotest/ogr/ogr_gpx.py @@ -526,3 +526,49 @@ def test_ogr_gpx_creator(tmp_vsimem, options, expected): data = gdal.VSIFReadL(1, gdal.VSIStatL(filename).size, f) gdal.VSIFCloseL(f) assert expected in data + + +############################################################################### +# Test ELE_AS_25D open option + + +def test_ogr_gpx_ELE_AS_25D(): + + ds = gdal.OpenEx( + "data/gpx/test.gpx", gdal.OF_VECTOR, open_options=["ELE_AS_25D=YES"] + ) + lyr = ds.GetLayerByName("waypoints") + f = lyr.GetNextFeature() + assert f.GetGeometryRef().ExportToIsoWkt() == "POINT Z (1 0 2)" + + lyr = ds.GetLayerByName("routes") + f = lyr.GetNextFeature() + assert f.GetGeometryRef().ExportToIsoWkt() == "LINESTRING Z (6 5 7,9 8 10,12 11 13)" + + +############################################################################### +# Test SHORT_NAMES open option + + +def test_ogr_gpx_SHORT_NAMES(): + + ds = gdal.OpenEx( + "data/gpx/test.gpx", gdal.OF_VECTOR, open_options=["SHORT_NAMES=YES"] + ) + lyr = ds.GetLayerByName("track_points") + f = lyr.GetNextFeature() + assert f["trksegid"] == 0 + + +############################################################################### +# Test N_MAX_LINKS open option + + +def test_ogr_gpx_N_MAX_LINKS(): + + ds = gdal.OpenEx( + "data/gpx/test.gpx", gdal.OF_VECTOR, open_options=["N_MAX_LINKS=3"] + ) + lyr = ds.GetLayerByName("track_points") + f = lyr.GetNextFeature() + assert f["link3_href"] is None diff --git a/doc/source/drivers/vector/gpx.rst b/doc/source/drivers/vector/gpx.rst index f32a22d26b85..2f51bb185be6 100644 --- a/doc/source/drivers/vector/gpx.rst +++ b/doc/source/drivers/vector/gpx.rst @@ -85,6 +85,40 @@ available: Determines the number of ** elements can be taken into account by feature. +Open options +------------ + +.. versionadded:: 3.9 + +The following open options are available: + +- .. oo:: SHORT_NAMES + :choices: YES, NO + :default: FALSE + + If ``YES``, the following fields will be renamed: + + - "track_seg_id" to "trksegid" + - "track_seg_point_id" to "trksegptid" + - "route_point_id" to "rteptid" + + But note that + no particular processing will be done for any extension field names. + +- .. oo:: ELE_AS_25D + :choices: YES, NO + :default: NO + + If ``YES``, the elevation + element will be used to set the Z coordinates of waypoints, route points + and track points. + +- .. oo:: N_MAX_LINKS + :default: 2 + + Determines the number of ** elements can be taken into account by + feature. + Encoding issues --------------- diff --git a/ogr/ogrsf_frmts/gpx/ogr_gpx.h b/ogr/ogrsf_frmts/gpx/ogr_gpx.h index 3b286be443fa..5089027a9be7 100644 --- a/ogr/ogrsf_frmts/gpx/ogr_gpx.h +++ b/ogr/ogrsf_frmts/gpx/ogr_gpx.h @@ -137,7 +137,7 @@ class OGRGPXLayer final : public OGRLayer public: OGRGPXLayer(const char *pszFilename, const char *layerName, GPXGeometryType gpxGeomType, OGRGPXDataSource *poDS, - bool bWriteMode); + bool bWriteMode, CSLConstList papszOpenOptions); ~OGRGPXLayer(); void ResetReading() override; @@ -223,7 +223,7 @@ class OGRGPXDataSource final : public GDALDataset int m_nLastTrkId = -1; int m_nLastTrkSegId = -1; - int Open(const char *pszFilename, int bUpdate); + int Open(GDALOpenInfo *poOpenInfo); int Create(const char *pszFilename, char **papszOptions); diff --git a/ogr/ogrsf_frmts/gpx/ogrgpxdatasource.cpp b/ogr/ogrsf_frmts/gpx/ogrgpxdatasource.cpp index 70e264c1f352..c756d123bd63 100644 --- a/ogr/ogrsf_frmts/gpx/ogrgpxdatasource.cpp +++ b/ogr/ogrsf_frmts/gpx/ogrgpxdatasource.cpp @@ -170,7 +170,7 @@ OGRGPXDataSource::ICreateLayer(const char *pszLayerName, return nullptr; } m_apoLayers.emplace_back(std::make_unique( - GetDescription(), pszLayerName, gpxGeomType, this, true)); + GetDescription(), pszLayerName, gpxGeomType, this, true, nullptr)); return m_apoLayers.back().get(); } @@ -447,10 +447,11 @@ static void XMLCALL dataHandlerValidateCbk(void *pUserData, const char *data, /* Open() */ /************************************************************************/ -int OGRGPXDataSource::Open(const char *pszFilename, int bUpdateIn) +int OGRGPXDataSource::Open(GDALOpenInfo *poOpenInfo) { - if (bUpdateIn) + const char *pszFilename = poOpenInfo->pszFilename; + if (poOpenInfo->eAccess == GA_Update) { CPLError(CE_Failure, CPLE_NotSupported, "OGR/GPX driver does not support opening a file in " @@ -567,15 +568,20 @@ int OGRGPXDataSource::Open(const char *pszFilename, int bUpdateIn) } m_apoLayers.emplace_back(std::make_unique( - GetDescription(), "waypoints", GPX_WPT, this, false)); + GetDescription(), "waypoints", GPX_WPT, this, false, + poOpenInfo->papszOpenOptions)); m_apoLayers.emplace_back(std::make_unique( - GetDescription(), "routes", GPX_ROUTE, this, false)); + GetDescription(), "routes", GPX_ROUTE, this, false, + poOpenInfo->papszOpenOptions)); m_apoLayers.emplace_back(std::make_unique( - GetDescription(), "tracks", GPX_TRACK, this, false)); + GetDescription(), "tracks", GPX_TRACK, this, false, + poOpenInfo->papszOpenOptions)); m_apoLayers.emplace_back(std::make_unique( - GetDescription(), "route_points", GPX_ROUTE_POINT, this, false)); + GetDescription(), "route_points", GPX_ROUTE_POINT, this, false, + poOpenInfo->papszOpenOptions)); m_apoLayers.emplace_back(std::make_unique( - GetDescription(), "track_points", GPX_TRACK_POINT, this, false)); + GetDescription(), "track_points", GPX_TRACK_POINT, this, false, + poOpenInfo->papszOpenOptions)); } return m_validity == GPX_VALIDITY_VALID; diff --git a/ogr/ogrsf_frmts/gpx/ogrgpxdriver.cpp b/ogr/ogrsf_frmts/gpx/ogrgpxdriver.cpp index d26830ddd73f..c47ef7e289f4 100644 --- a/ogr/ogrsf_frmts/gpx/ogrgpxdriver.cpp +++ b/ogr/ogrsf_frmts/gpx/ogrgpxdriver.cpp @@ -39,22 +39,32 @@ #include "ogr_core.h" /************************************************************************/ -/* Open() */ +/* Identify() */ /************************************************************************/ -static GDALDataset *OGRGPXDriverOpen(GDALOpenInfo *poOpenInfo) +static int OGRGPXDriverIdentify(GDALOpenInfo *poOpenInfo) { if (poOpenInfo->eAccess == GA_Update || poOpenInfo->fpL == nullptr) - return nullptr; + return false; + + return strstr(reinterpret_cast(poOpenInfo->pabyHeader), + "(poOpenInfo->pabyHeader), - "eAccess == GA_Update || !OGRGPXDriverIdentify(poOpenInfo)) return nullptr; OGRGPXDataSource *poDS = new OGRGPXDataSource(); - if (!poDS->Open(poOpenInfo->pszFilename, FALSE)) + if (!poDS->Open(poOpenInfo)) { delete poDS; poDS = nullptr; @@ -122,6 +132,19 @@ void RegisterOGRGPX() poDriver->SetMetadataItem(GDAL_DMD_HELPTOPIC, "drivers/vector/gpx.html"); poDriver->SetMetadataItem(GDAL_DMD_SUPPORTED_SQL_DIALECTS, "OGRSQL SQLITE"); + poDriver->SetMetadataItem( + GDAL_DMD_OPENOPTIONLIST, + "" + " "); + poDriver->SetMetadataItem( GDAL_DMD_CREATIONOPTIONLIST, "" @@ -174,6 +197,7 @@ void RegisterOGRGPX() poDriver->SetMetadataItem(GDAL_DCAP_VIRTUALIO, "YES"); poDriver->SetMetadataItem(GDAL_DCAP_MULTIPLE_VECTOR_LAYERS, "YES"); + poDriver->pfnIdentify = OGRGPXDriverIdentify; poDriver->pfnOpen = OGRGPXDriverOpen; poDriver->pfnCreate = OGRGPXDriverCreate; poDriver->pfnDelete = OGRGPXDriverDelete; diff --git a/ogr/ogrsf_frmts/gpx/ogrgpxlayer.cpp b/ogr/ogrsf_frmts/gpx/ogrgpxlayer.cpp index 498c16a003cb..2f00403b0ea6 100644 --- a/ogr/ogrsf_frmts/gpx/ogrgpxlayer.cpp +++ b/ogr/ogrsf_frmts/gpx/ogrgpxlayer.cpp @@ -70,23 +70,29 @@ constexpr int FLD_ROUTE_NAME = 2; OGRGPXLayer::OGRGPXLayer(const char *pszFilename, const char *pszLayerName, GPXGeometryType gpxGeomTypeIn, - OGRGPXDataSource *poDSIn, bool bWriteModeIn) + OGRGPXDataSource *poDSIn, bool bWriteModeIn, + CSLConstList papszOpenOptions) : m_poDS(poDSIn), m_gpxGeomType(gpxGeomTypeIn), m_bWriteMode(bWriteModeIn) { #ifdef HAVE_EXPAT const char *gpxVersion = m_poDS->GetVersion(); #endif - m_nMaxLinks = atoi(CPLGetConfigOption("GPX_N_MAX_LINKS", "2")); + m_nMaxLinks = + atoi(CSLFetchNameValueDef(papszOpenOptions, "N_MAX_LINKS", + CPLGetConfigOption("GPX_N_MAX_LINKS", "2"))); if (m_nMaxLinks < 0) m_nMaxLinks = 2; if (m_nMaxLinks > 100) m_nMaxLinks = 100; - m_bEleAs25D = CPLTestBool(CPLGetConfigOption("GPX_ELE_AS_25D", "NO")); + m_bEleAs25D = CPLTestBool( + CSLFetchNameValueDef(papszOpenOptions, "ELE_AS_25D", + CPLGetConfigOption("GPX_ELE_AS_25D", "NO"))); - const bool bShortNames = - CPLTestBool(CPLGetConfigOption("GPX_SHORT_NAMES", "NO")); + const bool bShortNames = CPLTestBool( + CSLFetchNameValueDef(papszOpenOptions, "SHORT_NAMES", + CPLGetConfigOption("GPX_SHORT_NAMES", "NO"))); m_poFeatureDefn = new OGRFeatureDefn(pszLayerName); SetDescription(m_poFeatureDefn->GetName()); @@ -859,7 +865,7 @@ void OGRGPXLayer::endElementCbk(const char *pszName) (m_gpxGeomType == GPX_TRACK && m_depthLevel == m_interestingDepthLevel + 3))) { - m_poFeature->GetGeometryRef()->setCoordinateDimension(3); + m_lineString->setCoordinateDimension(3); if (!m_osSubElementValue.empty()) {