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

Allow reading block-size of Rasters #67

Merged
merged 4 commits into from
May 14, 2019
Merged

Conversation

rmanoka
Copy link
Contributor

@rmanoka rmanoka commented Mar 3, 2019

This PR allows efficient streaming of raster by reading (using already existing read_as) along block boundaries. To get block size, call block_size on a RasterBand.

Additionally, we also add feature flag gdal_2_2 that enables the corresponding feature in gdal-sys crate to pull in features from GDAL >= 2.2. Using this, we provide actual_block_size function behind this feature gate to be used if required.

List of what is implemented:

  1. implemented GetBlockSize
  2. added feature flag "gdal_2_2" to enable recent GDAL functions.
  3. implemented GetActualBlockSize (behind "gdal_2_2" flag)

1. implemented GetBlockSize
2. added feature flag "gdal_2_2" to enable recent GDAL
functions.
3. implemented GetActualBlockSize (behind "gdal_2_2" flag)
@pka
Copy link
Member

pka commented Mar 29, 2019

Thanks for your contribution, @rmanoka! Code looks good to me. Would you mind adding some tests for the new functions and resolve the conflicting files?

@rmanoka
Copy link
Contributor Author

rmanoka commented Mar 29, 2019

@pka Sure, I'll add tests and resolve Cargo.toml conflict over the weekend.

There are a couple of minor issue with #68 that has been merged recently:

  1. The PR introduces block_size in RasterBand, similar to this PR. The code however is identical, so I will fix it in when updating this PR.

  2. However, read_block function added by the PR seems to have a bug. Suppose the raster dimensions are not perfectly divisible by the block-size, then the right, and bottom edge blocks will not have the size as provided by block_size (as it will be smaller). This was the reason why we had to include actual_block_size in this PR. To the best of my understanding, the read_block function would error when reading these edge blocks. The fix is to use actual_block_size in the beginning of read_block implementation; however, the two functions are under different feature gates, so I don't know how to proceed here.

Any ideas?

@rmanoka
Copy link
Contributor Author

rmanoka commented Mar 29, 2019

Actually, the GDAL docs say it already yields an array of block-size dimensions; so the user only needs to use actual_block_size to correctly use the output from read_block. Seems we should be in good shape with present implementation. Sorry about the confusion.

+ added `x_size()`, `y_size()`, and `size()` to `RasterBand`
+ added tests
@@ -42,6 +40,31 @@ impl <'a> RasterBand<'a> {
(size_x as usize, size_y as usize)
}

/// Get x-size of the band
pub fn x_size(&self) -> usize {
let out;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let out = unsafe { gdal_sys::GDALGetRasterBandXSize(self.c_rasterband); }

Copy link
Member

@lnicola lnicola 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 to me, and the Travis failure seems unrelated.

@lnicola
Copy link
Member

lnicola commented May 14, 2019

Ping @pka?

@pka pka merged commit 2c45678 into georust:master May 14, 2019
@pka
Copy link
Member

pka commented May 14, 2019

cargo doc fails on nightly. Will hopefully work again in a few days?

@lnicola
Copy link
Member

lnicola commented May 14, 2019

Works for me. Let's wait for the master build to finish.

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.

3 participants