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

[gdal] Don't try to download whole COG locally in order to determine open options #51982

Merged
merged 3 commits into from
Feb 23, 2023

Conversation

nyalldawson
Copy link
Collaborator

Don't try to download whole COG locally in order to determine open options in Add Raster Layer dialog, if a "http" URL is entered without the /vsicurl/ prefix

If a user directly enters a HTTP URL into the "Raster datasets" option in the Data Source Manager, QGIS will otherwise hang while the whole remote file is retrieved.

Instead automatically update the entered URI to a proper /vsicurl/ one so that GDAL's optimisations for remote data sources can take effect.

@github-actions github-actions bot added this to the 3.30.0 milestone Feb 22, 2023
@nyalldawson
Copy link
Collaborator Author

Can I grab your thoughts @rouault ?

@rouault
Copy link
Contributor

rouault commented Feb 22, 2023

That's always a bit tricky. That will work with COGs or other raster that are served by a HTTP range friendly server, but that will fail for unfriendly servers where only the ingest-the-whole-file strategy would work. But as noted the later is only practical for smallish files.
An "ideal" strategy would probably start with the /vsicurl/ one, and if that fails, fallback to the GDAL HTTP driver (as currently), but only for files "not too big" (which might not be easy to figure out quickly for non-cooperative servers). That said, that would seriously complicate the logic.
I guess your PR is a reasonable option that will be an improvement w.r.t current situation.

@nyalldawson
Copy link
Collaborator Author

@rouault

Hmm.... so the other option I thought of was that we just entirely skip the creation option population for http urls. Do you think that's a better solution?

@rouault
Copy link
Contributor

rouault commented Feb 22, 2023

skip the creation option population for http urls

open options you mean ? That would be reasonable too, at least for the GeoTIFF driver they are quite esoteric for QGIS use cases. But would the /vsicurl/ prefix be added when adding the file ?

@nyalldawson
Copy link
Collaborator Author

But would the /vsicurl/ prefix be added when adding the file ?

Ahh, I misunderstood your original concern -- it was with always adding the prefix, right?

So I propose:

  • revert the auto-add prefix
  • if a http(s) url without vsicurl prefix is entered, then always just clear the open options

@rouault
Copy link
Contributor

rouault commented Feb 23, 2023

it was with always adding the prefix, right?

yes, but actually trying (my previous comments were done without trying) with a relatively small file of 2 MB, adding without /vsicurl/ is barely usable

So I propose:
revert the auto-add prefix
if a http(s) url without vsicurl prefix is entered, then always just clear the open options

sounds good.

This can be very slow to process, as it may involve a complete
download of a huge remote file (hanging QGIS in the process)
Raster Layer dialog, prompt the user to promote these to
vsicurl URIs via a user-friendly warning message
@nyalldawson
Copy link
Collaborator Author

@rouault
how about the latest iteration -- now we show a warning to users if a non-vsicurl http(s) uri is entered:

image

@rouault
Copy link
Contributor

rouault commented Feb 23, 2023

how about the latest iteration -- now we show a warning to users if a non-vsicurl http(s) uri is entered:

Great !

@nyalldawson nyalldawson enabled auto-merge (rebase) February 23, 2023 05:55
@nyalldawson nyalldawson merged commit 65afa1c into qgis:master Feb 23, 2023
@nyalldawson nyalldawson deleted the load_cog branch February 23, 2023 05:56
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.

2 participants