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

feat(geotiff): Consolidate helpers for GeoTIFF images. #1469

Merged
merged 3 commits into from
Jun 10, 2021
Merged

Conversation

manzt
Copy link
Collaborator

@manzt manzt commented Jun 8, 2021

No description provided.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Looks good, just added suggestions.

@kylebarron We should add some actual GeoTIFF tests.

@@ -26,24 +25,26 @@ interface TiffOptions {
* multi-threaded pool of image decoders should be used to decode tiles (default = true).
* @return {Promise<{ data: TiffPixelSource[], metadata: ImageMeta }>} data source and associated OME-Zarr metadata.
*/
export async function loadGeoTiff(source: string | File, opts: TiffOptions = {}) {
export async function loadGeoTiff(source: string | File | GeoTIFF, opts: TiffOptions = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nits:

  • Since the function is called fromBlob can the type be the more general Blob instead of File?
  • I assume we could also accept ArrayBuffer and just call new Blob([arrayBuffer]) (or new File(...))?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you are correct, and it can be more general Blob. There is a fromArrayBuffer constructor as well: https://geotiffjs.github.io/geotiff.js/global.html

I'm not sure if you have an opinion on how to include these different global constructors in loaders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking about it. FWIW:

  • loaders.gl is currently standardized on parse(ArrayBuffer) and parseInBatches(AsyncIterator<ArrayBuffer>).
  • loaders.gl/core automatically converts from other types (File, Blob, Response, ...) to those types before calling the loaders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense. However, I think the fromArrayBuffer method is for TIFF images that fit into memory (e.g. fromArrayBuffer(fetch('https://data.ome.tiff').then(res => res.arrayBuffer()))). The other sources are intended for large, multichannel/tiled TIFFs, where different byte-ranges are read dynamically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps, similar to Zarr, there would be a way to create a GeoTIFF source using loader.gl utils/modules: https://github.com/geotiffjs/geotiff.js/tree/master/src/source

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • loaders.gl is currently standardized on parse(ArrayBuffer) and parseInBatches(AsyncIterator<ArrayBuffer>).

What do we do for large tilesets like i3s and 3dtiles? It seems a similar issue here where we don't want to bring the entire tileset into memory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note there are even some instances where you want to pass multiple URLs to fromURLs to create a single file, for example with a Landsat or Sentinel scene where there are 10 or 11 different files and you might want to load 3 of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note there are even some instances where you want to pass multiple URLs to fromURLs to create a single file, for example with a Landsat or Sentinel scene where there are 10 or 11 different files and you might want to load 3 of them.

I don't think it fits very well in the loaders.gl paradigm, but allowing for loadGeoTIFF to take any GeoTIFF object (regardless of source), enables the flexibility for tiff storage/layout (similar to allowing for a zarr.Store).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it fits very well in the loaders.gl paradigm, but allowing for loadGeoTIFF to take any GeoTIFF object (regardless of source)

Note this matches my suggestion here to avoid unnecessary fetches of the metadata. Essentially we'd have two loaders, a GeoTIFFMetadataLoader and a GeoTIFFLoader. The GeoTIFFMetadataLoader would fetch only the metadata of the GeoTIFF: here calling fromUrl, fromBlob, etc and returning a GeoTIFF object. The GeoTIFFLoader would either use the metadata/GeoTIFF object passed in or call the GeoTIFFMetadataLoader on the input.

This approach makes it possible for advanced users to load multiple tiles in succession while only loading the metadata once. Of course a similar approach is possible for Zarr as well.

Copy link
Collaborator Author

@manzt manzt Jun 8, 2021

Choose a reason for hiding this comment

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

Definitely something Analogous for Zarr as well. Given the .zarray metadata (JSON) and a valid store, you can initialize ZarrArray object(s) directly. hms-dbmi/vizarr#75

modules/geotiff/src/lib/load-geotiff.ts Outdated Show resolved Hide resolved
@manzt manzt merged commit f62d2a2 into master Jun 10, 2021
@manzt manzt deleted the manzt/tiff-loader branch June 10, 2021 13:17
@zakjan
Copy link

zakjan commented Jun 16, 2021

@manzt Looks great as a progress towards supporting generic GeoTIFF files! I tested it with a sample file generated with GDAL: 2021060212.tif.zip (GFS wind u,v speed). I'm not sure if you continue working on it? If not, I could implement loading such files.

@kylebarron Do you aim for COG support from the beginning, or would it make sense to start with non-COG files first, having COG in mind?

@kylebarron
Copy link
Collaborator

@kylebarron Do you aim for COG support from the beginning, or would it make sense to start with non-COG files first, having COG in mind?

I believe @ibgreen is thinking of a 3.0.0 release of loaders soon, where the Geotiff and Zarr loaders are included but undocumented, so that we can continue working on them until the 3.1.0 release.

@ibgreen
Copy link
Collaborator

ibgreen commented Jun 16, 2021

Yes my thinking is that zarr and geotiff will be a focus of [email protected].

Do you aim for COG support from the beginning, or would it make sense to start with non-COG files first, having COG in mind?

I would like us to support both, but getting non-COG support solid first sounds like a good approach.

The COG support will require more API consideration.

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.

4 participants