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

[meta-ticket] GDAL 4.0 potential changes related to API or behaviour breaks #8440

Open
16 tasks
rouault opened this issue Sep 20, 2023 · 29 comments
Open
16 tasks
Milestone

Comments

@rouault
Copy link
Member

rouault commented Sep 20, 2023

List of tickets tagged with 4.0 milestone: https://github.com/OSGeo/gdal/milestone/33

Below are just proposed ideas. Nothing decided

API breakage:

  • C and C++ API:
  • C++ API (mostly):
  • OGRLayer::GetNextFeature() and GetFeature() to return a std::unique_ptr<OGRFeature>: involves lots of internal changes, affect C++ users
  • or change OGRLayer::GetNextFeature() and GetFeature() to accept a OGRFeature& that they update. May help a bit performance in bulk reading operations, but probably not spectacular. EDIT: might not be doable, as some drivers return an instance of a subclass of OGRFeature
  • Remove reference count mechanism from OGRSpatialReference class itself and use std::shared_ptr<OGRSpatialReference> (or std::shared_ptr<const OGRSpatialReference> ?) instead : involves lots of internal changes, affect C++ users of the geometry and layer classes, although we might have to propagate it everywhere since OGRSpatialReferenceH would be an opaque type for std::shared_ptr<OGRSpatialReference>
  • Remove reference count mechanism from OGRFeatureDefn class itself and use std::shared_ptr<OGRFeatureDefn> instead: involves lots of internal changes, affect C++ users, may impact slightly the C API (OGR_FD_GetReferenceCount)
  • Remove reference count mechanism from GDALDataset class itself & remove OF_SHARED? GDALOpenShared used to be mostly used in the VRT driver but a more sophisticated mechanism has replaced it when opening from XML. But when building a VRT dynamically shared/ref_counting is used. And there are GDALOpenShared() users in external code though
  • C ABI:
  • Remove CPL_STDCALL #8158 : caution on existing external Windows code implementing the GDALProgressFunc typedef that would use manual __stdcall. That said __stdcall is a no-op on x64 and arm64, so the impact nowadays would be only for legacy x86 code

API behaviour changes:

Driver changes:

Driver removals?

Others :

@rouault rouault added this to the 4.0 milestone Sep 20, 2023
@tbonfort
Copy link
Member

  • GTIFF: used TILED=YES by default
  • GTIFF: use COMPRESS=LZW by default

@elpaso
Copy link
Collaborator

elpaso commented Sep 21, 2023

@jjimenezshaw
Copy link
Contributor

GTIFF: used TILED=YES by default

About this default without overviews, it can be really frustrating.
It happened to me: create a big tiff (one GB is enough) with tiles and no overviews and try to open it in QGIS. It may take minutes to show something. In that time you could think that the file or the program are broken.
Now I know it and I prevent that (either with overviews/cog or not opening it). I know the benefits of tiles. However I don't know how many frustrated users will blame us.

@tbonfort
Copy link
Member

About this default without overviews, it can be really frustrating.

Is this any different than with a stripped tif also without overviews ?

@jratike80
Copy link
Collaborator

When answering to users questions I have been thinking often that if we had no defaults at all then users should first think what they want. But I do not really want to write every time things like BLOCKXSIZE=256 BLOCKYSIZE=256. With the data that I play with I would say that TILED=YES could be a better default but I am not sure about COMPRESS=LZW. But LZW does make files smaller and maybe it does not make any harm in common use cases. What makes it better than DEFLATE?

@rouault
Copy link
Member Author

rouault commented Sep 22, 2023

Is this any different than with a stripped tif also without overviews ?

yes, scanline organized TIFFs are faster to downscale (when using nearest neighbour), as GDAL will only try to access 1 line over N if downscaling by a factor of N. For tiled datasets, unless N is at least twice the block height, you need to read all tiles.

@rouault
Copy link
Member Author

rouault commented Sep 22, 2023

I have been thinking often that if we had no defaults at all then users should first think what they want

There are tons of settings that should be set then: for a multiband dataset, INTERLEAVE ; for a non-Byte dataset, endianness; for compressed method, a number of compression options, etc. Think of JPEG2000 which has like 20 independent settings. For small enough datasets, in general you don't care about the settings. They become more relevant for big datasets where they have visible effects.

But LZW does make files smaller and maybe it does not make any harm in common use cases. What makes it better than DEFLATE?

Thought to be faster to compress/decompress. But just trying on one dataset, I find deflate to be actually faster (using libdeflate)

@jjimenezshaw
Copy link
Contributor

About this default without overviews, it can be really frustrating.

Is this any different than with a stripped tif also without overviews ?

Definitely.
Let's say that to view the full image you have a zoom factor of 100 (So you are showing 0.01% of the total pixels), and your tiles are 256x256.
With stripped it will read 1 every 100 lines. For a tiled tiff it will read every tile (just to read a few pixels on each).
I/O will be 100 times more.

@tbonfort
Copy link
Member

What makes it better than DEFLATE?

I don't know of any software that does not support LZW. I do know at least one (IDL) that does not support deflate. Aside from that I don't have any strong preference over one or another, especially since deflate has become so much faster with Even's libdeflate integration.

@jratike80
Copy link
Collaborator

I have been thinking often that if we had no defaults at all then users should first think what they want

There are tons of settings that should be set then:

Yes, I was not serious. Maybe the compression is the default that astonish users most and by changing the default we could save gigantic amout of bits in the world because so many users run with the defaults. There are loads of questions like this https://gis.stackexchange.com/questions/310149/gdal-nearblack-increases-file-size-is-this-expected-behaviour.

One could say that we should just use reasonable defaults but what is reasonable for one may not be that for the others. A lossless compression that does not alter data feels like a good default for GeoTIFF, but JPEG2000 drivers use lossy compression by default and I think that it is reasonable, too. Now thinking, maybe a bit questionable because someone may have lost the smallest details from their data unintentionally. On the other hand, 30% reduction in file size with one of the best compression methods that exist could feel disappointing for the users.

When it comes to this ticket as a whole, the GeoTIFF defaults may not be the most important items on the list. But because they are so concrete it is natural that they gather many comments.

@runette
Copy link
Contributor

runette commented Oct 5, 2023

There is one I would like to propose - although I would almost certainly have to implement it:

API Behaviour change

  • Make c# API compatible with MONO AOT compilers e.g. Xamarin or IL2CPP

Discussion

There is an incompatibility between SWIG generated C# code and Mono AOT compilers that cause failures when there are certain types of callbacks.

See swig/swig#1262 and related.

The particular usage scenario that I have come across is

2 When targetting the C# code to run in Mono with AOT (publishing WASM requires AOT)

Basically if the only way to do a callback from C to C# is via static methods marked with
[MonoPInvokeCallback(typeof(NativeBinding.callback))], and in the case of class methods take a GCHandle to this as > the first parameter.

This affects any AOT in Mono. The main consumer of this is probably IL2CPP in Unity but it is also a problem in Xamarin on iOS.

The fixes - as discussed here https://forum.htc.com/topic/9139-unity-il2cpp-and-callbacks/ are not complicated and actually are a matter of metadata as opposed to functional changes.

I have been implementing the changes manually for the UPM Package for GDAL that I support - without any problems so far. Doing this manually is less than ideal but I have been reluctant to port this change into the main repo because of the potential for breaking something that is not being tested (i.e. the "you don't know what you don't know" problem).

I would say that 4.0 would be the time to implement the change.

@wildintellect
Copy link

What makes it better than DEFLATE?

I don't know of any software that does not support LZW. I do know at least one (IDL) that does not support deflate. Aside from that I don't have any strong preference over one or another, especially since deflate has become so much faster with Even's libdeflate integration.

Are you sure this is still true, looks like reference docs indicate IDL can now write Deflate (would be weird to not read) https://www.nv5geospatialsoftware.com/docs/WRITE_TIFF.html

@tbonfort
Copy link
Member

@wildintellect support for deflate seems to have been added in version 8.8.2, released in march 2022, which can be considered very recent given their release velocity and adoption rate.

@plimkilde
Copy link
Contributor

I'd like to suggest ditching .py from the canonical way of calling the Python-implemented utilities (e.g. gdal_calc.py would become gdal_calc). This appears to make for easier implementation of Python package entry points, see conda-forge/gdal-feedstock#834, and is also more consistent with the remaining utilies.

@rouault
Copy link
Member Author

rouault commented Nov 15, 2023

I'd like to suggest ditching .py from the canonical way of calling the Python-implemented utilities (e.g. gdal_calc.py would become gdal_calc). This appears to make for easier implementation of Python package entry points, see conda-forge/gdal-feedstock#834, and is also more consistent with the remaining utilies.

attempt at fixing that in #8718 . Apparently on Windows setuptools create .exe launchers

@rouault
Copy link
Member Author

rouault commented Nov 23, 2023

Adding another potential topic:

  • Remove GDALRasterIOExtraArg structure. This was introduced as a backwards compatible way to express sub-pixel windows and non-nearest resampling, but it turns out to be error prone within the code base as it is easy to forget updating values in that structure (cf RasterIO: fix subpixel shift when reading from overviews with non-nearest resampling #8797). We could potentially modify IRasterIO() signature to take floating point values for the coordinates of the window of interest, the resampling algorithm and the progress function.

@jjimenezshaw
Copy link
Contributor

About this default without overviews, it can be really frustrating.

Is this any different than with a stripped tif also without overviews ?

Today the default TILED=YES came up again in the mailing list.

Based on his answer, I have the impression that @tbonfort was not aware of the consequences of using tiles without overview. And probably many users as well.

However it is really frustrating, even something not huge like 10k * 10k pixels. I have seen that several times at work.
IMO the default should stay as it is now (it works good enough for a significant number of users). If a user creates overviews should know that it will perform better with tiles. Or even better, they can just create a COG, that has tiles without any option. But I don't think it is a good idea bothering many users just to avoid setting TILED=YES when we create a geotiff ... with overviews.
Adding overviews artificially is over-complicating it just to change the default option; and very slow, that is the main problem.

I want to think there are two types of users: those who really go into the options, and configure the driver a lot; and those who just create a geotiff using the minimal configuration... because it is complicated. (at the beginning I was the latter, I want to think that now I am the former). For the first type we have all those options. It is really powerful. For the second type they rely on the defaults. It is simpler.

@tbonfort
Copy link
Member

tbonfort commented Nov 27, 2023

IMO the default should stay as it is now (it works good enough for a significant number of users)

This is not a hill I am willing to die on. Large tiffs that should have overviews but are missing them suck, and in that case stripped tiffs suck a bit less. In all other cases tiled tiffs are a better option, but my fingers have sufficient muscle memory to type -co TILED=YES every time I type a gdal command for me not to consider ditching gdal as a whole because of that :)

@jratike80
Copy link
Collaborator

When it comes to potential GDAL 4.0 changes I think that the TILED=YES or TILED=NO is not the most critical topic.

Defaults can never be optimal for all users. My opinion is that TILED=YES would be better for more users than TILED=NO, but I do not know all users. My typical use case for smaller or bigger TIFFs is to look at them at the resolution that is close to the native resolution, either on the screen or by using them as source data for WMS.

It happened to me: create a big tiff (one GB is enough) with tiles and no overviews and try to open it in QGIS. It may take minutes to show something. In that time you could think that the file or the program are broken.
Now I know it and I prevent that (either with overviews/cog or not opening it). I know the benefits of tiles. However I don't know how many frustrated users will blame us.

I tried to re-produce with an image of "Size is 48000, 24000", file size on disk 3.5 GB. It takes 5 seconds to open with QGIS on Windows, (Intel64 Family 6 Model 142 Stepping 10 GenuineIntel ~1910 Mhz, 32 GB of memory, SSD drive). It does feel a bit slow but acceptable. Panning when zoomed close to the native resolution is naturally fast. Maybe a part of your frustration is caused by the hardware.

I know from gis.stackexchange that it is not uncommon to have severe troubles when trying to warp or otherwise process big striped TIFF files. In that case the only solution is to re-write the image as tiled because adding overviews does not fix the processing issue even it helps with viewing.

The biggest problem with changing the default of TILED= is the change itself. Users will continue to use versions 3.x and 4.x side by side for several years and during that time those users who rely on the defaults would not know what will happen because a) users do not usually know what GDAL version they are using and b) users should be aware about the change of the default value between v3 and v4. That would certainly frustrate some users. Maybe the majority of users would not notice anything.

@rouault
Copy link
Member Author

rouault commented Nov 27, 2023

to type -co TILED=YES every time I type a gdal command for me not to consider ditching gdal as a whole because of that :)

making me wondering if a (non-breaking) improvement could not be to extend the GDAL configuration file to have a new section where could could add:

[default-creation-options-for-cli]
GTiff=TILED=YES
GTiff=COMPRESS=DEFLATE

that would be used for command line utilities. I believe they should display a message in the console in non quiet mode to recall those options

$ gdal_translate in.tif out.tif
Adding default creation options from /home/even/.gdal/gdalrc: -co TILED=YES -co COMPRESS=DEFLATE
0...10....20....30...40...50...60...70...80...90...100

That said it might perhaps only be desirable to use them for a command directly started from an interactive shell, and not from a script, and looking a bit, it seems tricky/impossible to detect reliably in which case we are, and even if we can, it might be too confusing.

@tbonfort
Copy link
Member

Thanks Even for looking into it, but I think that the issues arising from the variability between environments with different configuration files would be more problematic than having to repeatedly type a few creation options.

@rouault
Copy link
Member Author

rouault commented Nov 29, 2023

Other ideas that have been floated around in the past:

  • Make port/ CPL library a standalone shared object that other projects could link against, without linking GDAL itself. The main benefit for them would be to get all the /vsi file systems. Although there are some GDAL specific hacks leaking into CPL, but it should still remain largely usable outside of it. Or perhaps just the /vsi/ part of CPL ? (as nobody really needs a CPLMalloc() or a CPLString nowadays :-))

  • Move GDAL installed headers to a gdal/ subdirectory . (and potentially cpl's one into a dedicated one too)

@lnicola
Copy link
Contributor

lnicola commented Nov 30, 2023

Can we drop some of the deprecated functions like GDALGetDefaultHistogram? I just saw someone using it in new code.

@rouault
Copy link
Member Author

rouault commented Dec 8, 2023

Other candidate: removing support for direct calls to python bindings's setup.py script, and rather use "pip install" or other "modern"/recommended way of packaging Python ? (expert with 20 years of Python packaging experience required). Cf #8926

@sgillies
Copy link
Contributor

sgillies commented Dec 16, 2023

Here's a sorta breaking change that shouldn't be too controversial: make /vsicurl/ the default HTTP handler. This is the case now for Rasterio, and users appreciate it.

My opinion is that the option to fully download the resource at a URI and open it from a temporary location should exist at a level above GDALOpenEx, not within GDALOpenEx or driver code.

@tbonfort
Copy link
Member

gdaladdo / GDALBuildOverviews does not take creation options, but instead needs to pass these as configuration options. This leads to a de-synchronization between what options are available for main datasets vs. what is available for overview levels, and a clunky api/documentation. Last example to date: #8976
Breaking change for 4.0: make GDALBuildOverviews take creation options instead of relying on configuration options.

@rouault
Copy link
Member Author

rouault commented Jan 22, 2024

Another potential change is get rid of the wkbPoint25D, wkbLineString25D, etc. constants of the wkbGeometryType enumeration that pre-date ISO WKB, and that have "funny" values that are wkbPoint, wkbLineString or'ed with 0x8000000, which leads to unusual enumeration values not fitting into a signed int (apparently C23 makes it legal... cf #2322 (comment)). It could be best to have wkbPointZ = wkbPoint + 1000 to be ISO WKB compliant. That would impact the C & C++ API & ABI.

@rouault
Copy link
Member Author

rouault commented Jan 31, 2024

Remove unused OFTWideString and OFTWideStringList from OGRFieldType enumeration

@rouault
Copy link
Member Author

rouault commented Feb 21, 2024

Installing headers in ${CMAKE_INSTALL_INCLUDEDIR}/gdal : #9276

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

No branches or pull requests

10 participants