-
Notifications
You must be signed in to change notification settings - Fork 201
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
Conversation
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.
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 = {}) { |
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.
Nits:
- Since the function is called
fromBlob
can the type be the more generalBlob
instead ofFile
? - I assume we could also accept
ArrayBuffer
and just callnew Blob([arrayBuffer])
(or newFile(...)
)?
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.
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.
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.
I am thinking about it. FWIW:
- loaders.gl is currently standardized on
parse(ArrayBuffer)
andparseInBatches(AsyncIterator<ArrayBuffer>)
. - loaders.gl/core automatically converts from other types (File, Blob, Response, ...) to those types before calling the loaders.
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.
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.
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.
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
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.
- loaders.gl is currently standardized on
parse(ArrayBuffer)
andparseInBatches(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.
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.
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.
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.
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
).
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.
I don't think it fits very well in the loaders.gl paradigm, but allowing for
loadGeoTIFF
to take anyGeoTIFF
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.
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.
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
@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? |
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. |
Yes my thinking is that zarr and geotiff will be a focus of loaders.gl@3.1.
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. |
No description provided.