[WIP] Add new OGR driver for OpenDRIVE (XODR) #9504

michikommader · 2024-03-19T13:57:15Z

What does this PR do?

This adds a read-only vector driver for the road description format OpenDRIVE. OpenDRIVE is an open industry standard in the automotive domain of driving simulation, maintained by ASAM e.V. It is increasingly intersecting with the public and GIS domains which raises the need for better interoperability with open-source spatial tools.

This XODR driver is developed by German Aerospace Center (DLR) and licenced under MIT.
This XODR driver implementation uses libOpenDRIVE which is licenced under Apache 2.0.
This XODR driver is supposed to be used as plug-in.

I am am open to a fruitful discussion in order to make OpenDRIVE directly usable through GDAL. The context for this development has been introduced on FOSSGIS conference 2024: OpenDRIVE-HD-Karten mittels GDAL ins GIS bringen. Additional talks are planned for Geospatial World Forum and FOSS4G Europe 2024.

What are related issues/pull requests?

Tasklist

Make sure newly added files are correctly formatted (cf pre-commit configuration)
Add test case(s)
Add documentation
~~Updated Python API documentation (swig/include/python/docs/)~~ I see no necessity there for adding a new driver
Clarify handling of custom docker/ubuntu-small/DockerfileXODR with GDAL maintainers: Which file location is better suited for future maintenance – the driver's source code directory or a separate repository?
Review
Adjust for comments
All CI builds and checks have passed

Environment

Provide environment details, if relevant:

OS: tested with Ubuntu 24.04, Windows 10
Compiler: GCC on Linux, GCC 13.1.0 x86_64 (MCF threads) MinGW-w64 MSVCRT on Windows

rouault

Nice work generally. See my comments.

I would also like this to be triggered more by our CI.
Can you add a libopendrive build in .github/workflows/ubuntu_20.04/Dockerfile.ci and .github/workflows/ubuntu_22.04/Dockerfile.ci

Do you know how your driver (and libopendrive) is robust to corrupted / hostile datasets? At the very minimum, I'd like to see a test with a "random file" that has xodr extension, but isn't valid XODR file. Ideally we'd want to stress-test it using OSSFuzz. Cf https://github.com/OSGeo/gdal/blob/master/fuzzers/README.TXT , https://github.com/OSGeo/gdal/blob/master/fuzzers/build.sh

autotest/ogr/ogr_xodr.py

doc/source/drivers/vector/xodr.rst

ogr/ogrsf_frmts/xodr/ogrxodrlayerroadmark.cpp

ogr/ogrsf_frmts/xodr/ogrxodrlayerroadobject.cpp

ogr/ogrsf_frmts/xodr/ogrxodrlayerroadsignal.cpp

rouault · 2024-03-19T15:47:51Z

Is the some packaging of libopendrive in Debian, conda-forge, etc ?

rouault · 2024-04-18T10:18:10Z

@michikommader Do you plan any further work on this any time soon ? I'm asking as I'm trying to keep the list of opened PRs low, and if something is not going to progress for some time, it is better to close the PR, and re-open it later when work restart

michikommader · 2024-04-18T13:31:47Z

@michikommader Do you plan any further work on this any time soon ?

Apologies for my inactivity. Yes, we already went through some of your valuable remarks internally but did not review and push changes yet. I plan to supply all the missing information in the first week of May.

michikommader · 2024-05-06T12:51:11Z

Is the some packaging of libopendrive in Debian, conda-forge, etc ?

Not yet. I myself am not maintainer of libOpenDRIVE and would rather see packaging responsibility on the maintainer side. We can separately discuss if our institute could take over this task. Also, ASAM e. V. as standardisation organisation behind OpenDRIVE could have an interest in that.

michikommader · 2024-05-15T20:56:10Z

Do you know how your driver (and libopendrive) is robust to corrupted / hostile datasets? At the very minimum, I'd like to see a test with a "random file" that has xodr extension, but isn't valid XODR file.

libOpenDRIVE forwards XML data loading directly to pugixml. According to pugixml's exception guarantees "most pugixml functions have a no-throw exception guarantee". For handling parsing errors the xml_parse_result would have to be inspected for its xml_parse_status. But, from GDAL driver side we do not have access to that. Unfortunately, libOpenDRIVE itself only uses simple printf() statements to report possible problems.

At least, libOpenDRIVE gives access to the parsed pugi::xml_document. Checking for the availability of "expected OpenDRIVE XML nodes" would allow basic validation if the parsing was successful. But, I don't see this as a very practicable way. If the dataset was corrupted "somewhere in the middle", pugixml would still have successfully parsed all previous content and made this accessible to the caller. For our use case of ensuring robustness in GDAL, I see this as a major deficit. The best solution would be to extend libOpenDRIVE to allow at least access to pugixml's xml_parse_status with which we could implement a somewhat meaningful error handling. I sent a pull request to the maintainer.

michikommader · 2024-05-30T16:02:49Z

I'll focus on satisfying cppcheck_2004 next days.

doc/source/development/building_from_source.rst

.github/workflows/ubuntu_20.04/Dockerfile.ci

.github/workflows/ubuntu_22.04/Dockerfile.ci

michikommader

@rouault In the current master branch you use ARROW_VERSION=16.0.0-1 but when testing, I have to set it to 16.1.0-1 in order to successfully build the Ubuntu 24.04 Docker image.

coveralls · 2024-05-31T12:59:35Z

coverage: 69.17% (+0.02%) from 69.148%
when pulling 9ffd4e8 on DLR-TS:libopendrive-pr
into 0890073 on OSGeo:master.

rouault · 2024-05-31T12:59:37Z

I have to set it to 16.1.0-1 in order to successfully build the Ubuntu 24.04 Docker image.

yes, you've well done. The version has to be bumped regularly

rouault · 2024-05-31T15:26:43Z

@michikommader Do you mind rebasing on top of latest master? I've cherry-picked the change for ARROW_VERSION. And you'll have to do a few changes in your CMakeLists.txt file as well as the ogrxdrdrivercore.h due to the changes of #10068 that has just been merged. See 1cd120f as an example of the changes you'll have to do: adding NO_SHARED_SYMBOL_WITH_CORE, using #define FOO PLUGIN_SYMBOL_NAME(FOO) and removing CPL_DLL.

- Refactor file opening - Ensure m_ prefix for member variables - Simplify for loops - Use default member initializers - Simplify and optimise manual memory management using std::unique_ptr - Pass by const and reference where possible - Use SetGeometryDirectly on features to avoid memory leaks - Implement deferred driver loading capability as plugin, as per RFC 96 - Implement OGRGetNextFeatureThroughRaw - Move XODR driver config to official Dockerfiles - Add "test_ogrsf" compliance checking to Python tests - Improve documentation

- Pass std::string by const reference - Switch to postfix operators for non-primitive types - Avoid virtual function calling from subclass constructors

rouault · 2024-05-31T22:01:53Z

ogr/ogrsf_frmts/xodr/ogrxodrdrivercore.cpp

+    CPLString extension = CPLString(CPLGetExtension(poOpenInfo->pszFilename));
+    extension.tolower();
+    return EQUAL(extension, "xodr");


EQUAL() is case insensitive

Suggested change

CPLString extension = CPLString(CPLGetExtension(poOpenInfo->pszFilename));

extension.tolower();

return EQUAL(extension, "xodr");

return EQUAL(CPLGetExtension(poOpenInfo->pszFilename), "xodr");

rouault · 2024-05-31T22:05:20Z

ogr/ogrsf_frmts/xodr/ogrxodrdatasource.cpp

+    VSILFILE *file = nullptr;
+    file = VSIFOpenL(pszFilename, "r");
+    if (file == nullptr)
+        return FALSE;
+


This leaks the VSILFILE*, and is not necessary due OGRXODRDriverOpen() testing that poOpenInfo->fpL != nullptr

Suggested change

VSILFILE *file = nullptr;

file = VSIFOpenL(pszFilename, "r");

if (file == nullptr)

return FALSE;

rouault reviewed Mar 19, 2024

View reviewed changes

rouault reviewed May 31, 2024

View reviewed changes

doc/source/development/building_from_source.rst Show resolved Hide resolved

rouault reviewed May 31, 2024

View reviewed changes

.github/workflows/ubuntu_20.04/Dockerfile.ci Outdated Show resolved Hide resolved

rouault reviewed May 31, 2024

View reviewed changes

.github/workflows/ubuntu_20.04/Dockerfile.ci Outdated Show resolved Hide resolved

rouault reviewed May 31, 2024

View reviewed changes

.github/workflows/ubuntu_22.04/Dockerfile.ci Outdated Show resolved Hide resolved

rouault reviewed May 31, 2024

View reviewed changes

.github/workflows/ubuntu_22.04/Dockerfile.ci Outdated Show resolved Hide resolved

michikommader commented May 31, 2024

View reviewed changes

michikommader added 11 commits May 31, 2024 21:14

Add new OGR driver for OpenDRIVE (XODR)

c07aca3

Switch to MIT license

f32a69f

Increment versionadded to 3.10

59cb14e

Move libOpenDRIVE before libQB3 for correct order

2c8ab28

Apply review suggestions for CI Dockerfiles

7f2fd92

Ensure cppcheck passes without errors

296e12c

- Pass std::string by const reference - Switch to postfix operators for non-primitive types - Avoid virtual function calling from subclass constructors

Avoid calling ResetReading from constructor

90bfdf8

Add libOpenDRIVE config to Ubuntu 24.04 Dockerfile.ci

9ff0c1c

Implement new changes related to RFC96

e184ba7

Increment Ubuntu version in doc

710a023

michikommader force-pushed the libopendrive-pr branch from 9ffd4e8 to 710a023 Compare May 31, 2024 19:35

rouault reviewed May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add new OGR driver for OpenDRIVE (XODR) #9504

[WIP] Add new OGR driver for OpenDRIVE (XODR) #9504

michikommader commented Mar 19, 2024 •

edited

rouault left a comment

rouault commented Mar 19, 2024

rouault commented Apr 18, 2024

michikommader commented Apr 18, 2024

michikommader commented May 6, 2024

michikommader commented May 15, 2024 •

edited

michikommader commented May 30, 2024

michikommader left a comment •

edited

coveralls commented May 31, 2024

rouault commented May 31, 2024

rouault commented May 31, 2024

rouault May 31, 2024

rouault May 31, 2024

[WIP] Add new OGR driver for OpenDRIVE (XODR) #9504

Are you sure you want to change the base?

[WIP] Add new OGR driver for OpenDRIVE (XODR) #9504

Conversation

michikommader commented Mar 19, 2024 • edited

What does this PR do?

What are related issues/pull requests?

Tasklist

Environment

rouault left a comment

Choose a reason for hiding this comment

rouault commented Mar 19, 2024

rouault commented Apr 18, 2024

michikommader commented Apr 18, 2024

michikommader commented May 6, 2024

michikommader commented May 15, 2024 • edited

michikommader commented May 30, 2024

michikommader left a comment • edited

Choose a reason for hiding this comment

coveralls commented May 31, 2024

rouault commented May 31, 2024

rouault commented May 31, 2024

rouault May 31, 2024

Choose a reason for hiding this comment

rouault May 31, 2024

Choose a reason for hiding this comment

michikommader commented Mar 19, 2024 •

edited

michikommader commented May 15, 2024 •

edited

michikommader left a comment •

edited