-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add LIBERTIFF driver: native thread-safe read-only GeoTIFF reader #11507
base: master
Are you sure you want to change the base?
Conversation
4916920
to
6d8454a
Compare
I'm curious to know a bit more the thoughts of the persons having watched this PR. This driver is mostly my reaction to a recurring complaint: "why aren't GDAL drivers thread-safe for read-only scenarios ?", and also partly because relying on libtiff, knowing what is inside, makes me somewhat nervous (although it happens to work). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not finished yet to read it (it is big ;)
Two comments:
I like to use non square images for tests. To not confuse width and height. The same way I do not use 45 deg to test angles (cosine and sine are the same)
I have the impression there is much code copied from gtiff. Would it make sense to make an intermediate class with the common code (like interpolateatpoint) and Libertiff and GTiff inherit from it?
|
||
#ifdef __clang__ | ||
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? you replaced some 0 with NULL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, at the beginning I've removed just the warnings needed to pass on gcc, but clang raises much more during CI runs and they weren't easy/awkward to fix in libtiff upstream being a C project (don't want to impose my hacks too much...), so suppression of those warnings seem the less invasive solution.
This driver is based on the libertiff library for IFD and tag parsing. It uses in a very controlled (and isolated) way internal libtiff LZW, PackBits and LERC codecs (even if using external libtiff for the GTiff driver). For Deflate/GZip, LZMA, ZSTD, it defers to GDAL cpl_compressor.h API. For JPEG, WEBP, JPEGXL, it uses the corresponding GDAL drivers to open strip/tile blobs as temporary datasets. Note that at current time the driver isn't necessarily optimized to minimize the number of network requests for COG layout. The driver supports most TIFF formulations, but not necessarily the most exotic ones that the GTiff libtiff based might support. In particular it does not support non-power-of-two BitsPerSample value (could be added with extra coding). On the plus side, JPEGXL is supported as soon as the JPEGXL driver is available at runtime (contrary to the GTiff driver which requires using internal libtiff since the tif_jxl.c codec hasn't been upstreamed to libtiff yet). As noted in the documentation, no side-car file at all are currently used by this driver.
I've tested a few inversions in the code to check if they broke tests, and there were indeed a few cases where the tests where not selective enough. Thanks for the suggestion!
there were indeed a few copy&paste. I've created a common header to factor them. |
This driver is based on the libertiff library for IFD and tag parsing.
It uses in a very controlled (and isolated) way internal libtiff LZW,
PackBits and LERC codecs (even if using external libtiff for the GTiff
driver). For Deflate/GZip, LZMA, ZSTD, it defers to GDAL
cpl_compressor.h API. For JPEG, WEBP, JPEGXL, it uses the corresponding
GDAL drivers to open strip/tile blobs as temporary datasets.
Note that at current time the driver isn't necessarily optimized to
minimize the number of network requests for COG layout.
The driver supports most TIFF formulations, but not necessarily the most
exotic ones that the GTiff libtiff based might support. In particular it
does not support non-power-of-two BitsPerSample value (could be added
with extra coding). On the plus side, JPEGXL is supported as soon as the
JPEGXL driver is available at runtime (contrary to the GTiff driver which requires
using internal libtiff since the tif_jxl.c codec hasn't been upstreamed
to libtiff yet).
As noted in the documentation, no side-car file at all are currently
used by this driver.
Consult https://github.com/rouault/gdal/blob/libertiff/doc/source/drivers/raster/libertiff.rst