Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

- Added fixes for the OGC API Tiles driver #7656

Merged
merged 8 commits into from
May 2, 2023

Conversation

doublebyte1
Copy link
Contributor

The OGC API Tiles dataset was not working as expected.

A complete document was being passed to TileMatrixSet::parse, which was causing a series of issues:

  • the comparisons with the well-known uris were never verified
  • all the other ways of retrieving a TMS were failing and it was falling back to reading TMS from a file (which had the entire document in the filename causing buffer overflow)

The well-known URI was being compared to a word/name, rather than an uri. I added support to two of the most popular ones, but we could add more.

Submitted changes:

  • Passed uri to TileMatrixSet::parse
  • updated uris of well-known tile matrix sets

With these changes, I can read raster and vector tiles from these urls:

gdalinfo -oo API=TILES --config CPL_DEBUG ON "OGCAPI:https://maps.gnosis.earth/ogcapi/collections/blueMarble"
ogrinfo -oo API=TILES --config CPL_DEBUG ON "OGCAPI:https://maps.gnosis.earth/ogcapi/collections/Daraa"

 - Passed uri to TileMatrixSet::parse
 - updated uris of well-known tile matrix sets
@rouault
Copy link
Member

rouault commented Apr 29, 2023

More fixes are needed to support OGC 2D Tile Matrix Set v2: cf #6882
And we must still support v1 because of other uses in GDAL

@doublebyte1
Copy link
Contributor Author

doublebyte1 commented Apr 30, 2023

"InspireCRS84Quad")

Yes. I would like to address #6882 on a different PR.

@rouault
Copy link
Member

rouault commented Apr 30, 2023

I would like to address #6882 on a different PR.

sounds good. Make sure to have commit hooks installed to have correct code formatting: cf https://gdal.org/development/dev_practices.html#commit-hooks

@doublebyte1
Copy link
Contributor Author

@rouault Thank you for the feedback! 🙏🏽

With 13360c5, I was able to run successfully all the tests in the test suite:

100% tests passed, 0 tests failed out of 28

Label Time Summary:
quicktest    = 156.70 sec*proc (19 tests)

Total Test time (real) = 156.91 sec
Process finished with exit code 0


if (uri.empty())
{
CPLError(CE_Failure, CPLE_AppDefined, "Cannot parse TMS uri");
Copy link
Contributor

@jerstlouis jerstlouis Apr 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doublebyte1
Not sure what the logic is, but it is totally OK for a 2D TMS to not have a URI, so this should probably not fail here?
Only registered 2DTMS have a uri.

Also, I would also expect the URI-matching-based identification (for registered 2DTMSs) to have happened before using the tileMatrixSetURI property of the tileset instead. If the tileMatrixSetURI was a match for a registered 2DTMS, the client would not need to download the 2DTMS definition at all (the DownloadJSon() call above I think).

Copy link
Contributor Author

@doublebyte1 doublebyte1 Apr 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerstlouis you are right. There are other ways of parsing the TMS information.

I will need to refactor the code, to send the entire document (as before) and to parse the uri at the beginning of the parse function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doublebyte1 Yes essentially it could use the URI already checked at:

https://github.com/OSGeo/gdal/blob/master/frmts/ogcapi/gdalogcapidataset.cpp#L1489

const auto oTileMatrixSetURI = oTileset.GetString("tileMatrixSetURI");

and if this URI is predefined (e.g., WebMercatorQuad or WorldCRS84Quad), then it does not need to call DownloadJSon(osTilingSchemeURL.c_str(), ....

This InitWithTilesAPI() method is very long though, so it would benefit from being split into different methods each performing one specific high level task.

@doublebyte1 doublebyte1 marked this pull request as draft April 30, 2023 14:11
… If that fails, instead of throwing an error it does what it was doing before (e.g.: sends an entire document to the parser and lets it find the needed information).
// Attempts to find the uri for a well-known TMS; if it does not work, it will send the entire document
const auto uri = oDoc.GetRoot().GetString("uri");

auto tms = gdal::TileMatrixSet::parse(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerstlouis for the time being, I am just checking if it can get an uri, and if not it defaults to what was there before (e.g.: sending the entire document). At least this is now working for well-known uris.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doublebyte1 Yes it seems that this method accepts either an identifier or a full document, which is quite confusing.
Ideally, there would be a diferent method gdal::TileMatrixSet::fromIdentifier() and parse() would always expect a TileMatrixSet definition.

Then, parse() itself could be the one first looking for the uri inside of the definition and then calling gdal::TileMatrixSet::fromIdentifier() when it is found.

But what I was saying earlier, is that when parsing the tileset, it is oTileMatrixSetURI (in the tileset metadata, not the tile matrix set definition) that should be used with gdal::TileMatrixSet::fromIdentifier() (or parse()) before downloading the TileMatrixSet definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerstlouis I got that, but I don't want to address this refactoring in this PR.

@doublebyte1 doublebyte1 marked this pull request as ready for review May 1, 2023 14:43
doublebyte1 and others added 2 commits May 2, 2023 12:42
@rouault rouault merged commit 20da904 into OSGeo:master May 2, 2023
@rouault rouault added this to the 3.7.0 milestone May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants