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

Very minor changes to allow multiband GeoTiff reading #214

Closed
tromper opened this issue Sep 7, 2023 · 5 comments · Fixed by #224
Closed

Very minor changes to allow multiband GeoTiff reading #214

tromper opened this issue Sep 7, 2023 · 5 comments · Fixed by #224

Comments

@tromper
Copy link
Contributor

tromper commented Sep 7, 2023

I realize that this has been proposed to various degrees before. In the GIS space, geotiffs with multiple bands are relatively common. It would be nice to be able to just read all of the band data into a vector. The coordinate projections can easily be handled manually since the relevant tags are already exposed. There are only a couple of minor things blocking the read functionality:

  • The GeoTiff is usually coming in as a Photometric interpretation of BlackIsZero (WhiteIsZero also is ok). In image.rs, we'd need to allow more than 1 bits_per_sample to avoid the unsupported error.
  • Need to allow sample depth other than 1,3,4
  • The decoding buffer size limit of 256mb is too small for many geotiffs, the unlimited settings work fine.

One way to implement the above would be to detect if a tiff is a geotiff (e.g. it has the coordinate project tag info present probably works) and then just bypass the above restrictions just for geotiffs w/o impacting any of the current functionality for non-geotiffs I was able to do this quickly and the existing ColorType::Gray ends up working fine for the different geotiff band data types. This isn't going work for all possible geotiffs (e.g bands with different data types won't work currently), but that's a less common use case (and commented as a todo in the code already, so maybe it happens someday). There are a couple of existing geotiff crates, but they were built for a single use case and appear to be largely unused/unsupported. I am happy to code up the changes (all 5 lines) and submit, but wanted to check first if a line had already been drawn on this topic and it isn't going to happen.

@fintelia
Copy link
Contributor

fintelia commented Sep 9, 2023

I'm in favor improving the crate to support a broader range of TIFF files. No need to gate changes on the file specifically being a geotiff. The only point of returning "unsupported" errors is because the rest of the code doesn't handle them properly. The goal is to either return an error or correctly decode the image. If any checks rejects TIFFs that we'd otherwise decode fine, then we should fix that.

The GeoTiff is usually coming in as a Photometric interpretation of BlackIsZero (WhiteIsZero also is ok). In image.rs, we'd need to allow more than 1 bits_per_sample to avoid the unsupported error.

I'm not quite sure what you mean here, but it might be easiest just to see the PR you have in mind.

Need to allow sample depth other than 1,3,4

Sounds reasonable.

The decoding buffer size limit of 256mb is too small for many geotiffs, the unlimited settings work fine.

Yep, this is why we have the option to disable decoding limits! Doesn't sound like further changes are required here.

tromper added a commit to tromper/image-tiff that referenced this issue Sep 11, 2023
…lowing BlackIsZero and WhiteIsZero to have up to 64 bits per sample. Also includes decode_geotiff_images unit test.
@tromper
Copy link
Contributor Author

tromper commented Sep 11, 2023

I submitted the PR that I described (minus the decoding limit changed, which you pointed out isn't neeeded).

  • Allows up to 255 samples per pixel
  • Allows BlackIsZero or WhiteIsZero to have up to 64 bits per sample
  • Added a unit test to read a simple 5 band geotiff into a vector

More than happy to do more on this if needed, so pls push back on anything that needs work here.

@weiji14
Copy link
Contributor

weiji14 commented Mar 17, 2024

Hi, small ping here to say that I've drafted a Pull Request to enable multi-band reads at #224, building on @tromper's work. Have tested the multi-band patch in my downstream library at weiji14/cog3pio#13, and it seems to be working nicely for a sample file, but would appreciate someone having a look to review it properly if possible!

@fintelia
Copy link
Contributor

Sorry for the delay! The PR is on my list to review, but I've been rather busy with the image 0.25.0 release and some other responsibilities. Hoping to catch up with my PR backlog in the next week or so

@weiji14
Copy link
Contributor

weiji14 commented Mar 24, 2024

Thanks @fintelia!

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 a pull request may close this issue.

3 participants