Raster band wrappers

[ddohler]

Overview

Add wrappers for some GDALRasterBand methods, specifically

• GDALGetRasterDataType
• GDALGetRasterMaximum
• GDALGetRasterMinimum
• GDALGetRasterNoDataValue
• GDALGetRasterStatistics

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

Demo

Added band statistics at the bottom:

Notes

I'm interested to hear thoughts on the interface here. I had initially planned to go with the interface discussed in #81 (comment), 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

• Run tests
• Check some rasters in the demo site (yarn run demo and then go to localhost:8080) and confirm the stats match the information provided by gdalinfo -stats on the command line.

Checklist

• Add entry to CHANGELOG.md
• Update the README with any function signature changes

Resolves #81

[mstone121]

Taking a look now

[mstone121]

Code looks good and tests are passing. I have just a few optional suggestions here and in the comments.

This is probably outside the scope of these changes, but I'm see that the following snippets of code are repeated in many of the wrappers:

        const errorType = errorHandling.CPLGetLastErrorType();

        // Check for errors; clean up and throw if error is detected
        if (
            errorType === errorHandling.CPLErr.CEFailure ||
            errorType === errorHandling.CPLErr.CEFatal
        ) {
            let message = errorHandling.CPLGetLastErrorMsg();

            throw new Error('Error in GDALGetRasterMaximum: ' + message);
        } else {
            return result;
        }

and

const bandPtr = Module.ccall(
            'GDALGetRasterBand',
            'number',
            ['number', 'number'],
            [datasetPtr, bandNum]
        );

Could these be extracted to helper functions?

Regarding the choice of interface: Without knowing much about how the implementation would work, I would say that the two-step (get band, then get band stats) version makes more sense. The dataset object should manage the dataset and a separate band object should manage a band.

That being said, I don't think it's entirely necessary to have separation of concerns at this point since it's only a few functions. Given that implementation of the two step interface would require a lot of extra work, I think we should stick to this for now and consider implementing the additional functionality if more specific band interactivity is required.

[mstone121]

It might be helpful to add a comment here to show that this array must be kept in sync with the GDAL enum mentioned here: https://github.com/azavea/loam/pull/112/files#diff-58204db7a8ef0d088be0d859d0fbb107d5fa2fa237581949e9a31f0e3ae0bd0bR12.

[ddohler]

Good catch! I've done that for other enums, dunno why I forgot here. I'll move over one of those comments.

[ddohler]

@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]

... 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.