azavea / loam

Javascript wrapper for GDAL in the browser
Apache License 2.0
218 stars 16 forks source link

Raster band wrappers #112

Closed ddohler closed 1 year ago

ddohler commented 1 year ago

Overview

Add wrappers for some GDALRasterBand methods, specifically

These methods require the band number to be passed when called, e.g. ds.bandMaximum(1).

Demo

Added band statistics at the bottom:

Screenshot from 2022-12-02 10-26-58

Notes

I'm interested to hear thoughts on the interface here. I had initially planned to go with the interface discussed in https://github.com/azavea/loam/issues/81#issuecomment-915206788, but as I got into implementation, that approach would have added complexity and I became less and less confident that I would prefer it as a library user. It would make multiple calls to the same band smoother because it gives you a band object you can hold onto for repeat access to the same band, but it would make calling the same method on multiple bands clunkier because each different band you access requires two steps rather than one (getBand(num).then(band => band.getWhatever()) as compared to ds.bandGetWhatever(bandNum)).

I went with the approach here because it was pretty simple to implement, and we can build on it to implement the other interface later if we want to. For example, a hypothetical getBand() function could be implemented inside gdalDataset.js as something like the below (though I've ignored promises):

getBand(bandNum) {
  return {
    getMax: () => accessFromDataset('GDALGetRasterMaximum', this, bandNum);
    // etc.
  }
}

Testing Instructions

Checklist

Resolves #81

mstone121 commented 1 year ago

Taking a look now

ddohler commented 1 year ago

@mstone121 Thanks for the review! I've made the changes you suggested.

About the repetitions you noticed in the code, totally--the pattern of 1) Check for error status 2) Either throw or return a result depending on error status is something I've wanted to simplify for a long time. Historically, the problem has been that GDAL's API functions don't operate particularly consistently and so there are small but important differences in the process across the various wrappers in terms of how they detect errors and recover from them. The last time that I looked into trying to create a helper function for this process I ended up giving up because I couldn't figure out an elegant way to do it without the helper function being pretty complex to be able to handle all the various corner cases. That's why I ended up going with the "verbose but easily modifiable" approach that you see here. It is definitely worth another look though, especially now that I've added a bunch of additional wrappers; I'll give it some additional thought for the next round of development.

For the GDALGetRasterBand call, I missed this because after spending enough time in Emscripten's bizarro Javascript-that-looks-an-awful-lot-like-C-code world, I had gotten used to Module.ccall() just being the way that you call functions such that I didn't think to wrap it in anything. I'm planning to add RasterIO next so I'll see whether it's feasible to do this when I do that work (I think it should be but there are some complexities to setting up the wasm execution environment that may make it trickier than it would appear at first glance).

mstone121 commented 1 year ago

... an elegant way to do it without the helper function being pretty complex to be able to handle all the various corner cases

Yeah, this makes sense. After going back to take a more careful look I realize they are subtly different.

... used to Module.ccall() just being the way that you call functions such that I didn't think to wrap it in anything

This also makes sense. There are cases when being more explicit outweighs the organization/efficiency benefits of creating helper functions and this might be one of those cases. It might be helpful to have the "C" code (or at least the ccall exposed at this layer of code.