Add wrappers for GDALBuildVRT, GDALApplyGeoTransform, GDALInvGeoTransform #239

ayymart · 2022-01-10T01:24:32Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

I did not know where to put apply_geo_transform and inv_geo_transform, so I left them in lib.rs. Please let me know where they should go.

I also did not know what manner of tests should be included. Please let me know what I should contribute in that regard.

src/lib.rs

src/raster_programs.rs

lnicola · 2022-01-10T16:29:36Z

src/raster_programs.rs

+) -> Result<Dataset> {
+    // Convert dest to raw string
+    let dest = match dest {
+        Some(s) => CString::new(s)?.into_raw(),


Same as below, dest.map(|p| p.as_mut_ptr()).

src/raster_programs.rs

ayymart · 2022-01-12T04:48:13Z

Thanks for the feedback. I've summarized my updates below:

invert_geo_transform now returns a result.
Added struct BuildVRTOptions that frees the owned pointer on drop.
build_vrt
- Uses _last_null_pointer_err to populate the error result.
- Employs that thin generic wrapper pattern. (is there a name for this?)
- Takes dest: Option<&Path> instead of String, though it gets converted using Path::to_string_lossy

I didn't take the suggestions re: raw strings verbatim, as I didn't see how they added terminating null bytes. I did look at GDALBuildVRTOptionsNew and determine that it doesn't actually mutate the strings, so I left them owned by the CStrings, which hopefully addresses part of the concern.

lnicola · 2022-01-12T09:25:51Z

src/raster_programs.rs

+
+fn _build_vrt(dest: Option<&Path>, datasets: &[&Dataset], args: Vec<String>) -> Result<Dataset> {
+    // Convert dest to CString
+    let dest = match dest {


It's not great, but let's use utils::_path_to_c_string here.

(untested)

let dest = dest.map(crate::_path_to_c_string).transpose()?; let c_dest = dest.map(CString::as_ptr).unwrap_or(ptr::null());

lnicola · 2022-01-12T09:34:01Z

src/raster_programs.rs

+    /// [GDALBuildVRTOptionsNew]: https://gdal.org/api/gdal_utils.html#_CPPv422GDALBuildVRTOptionsNewPPcP28GDALBuildVRTOptionsForBinary
+    fn new(args: Vec<String>) -> Result<Self> {
+        // Convert args to CStrings to add terminating null bytes
+        let mut cstr_args = Vec::<CString>::new();


Untested, but:

let cstr_args = args.into_iter() .map(|arg| CString::new(arg)) .collect::<Result<Vec<_>>>()?;

This looks nicer and (as a minor point) pre-allocates the destination Vec.

ayymart · 2022-01-12T19:46:33Z

Incorporated latest feedback.

lnicola · 2022-01-12T19:55:05Z

src/raster_programs.rs

+/// [GDALBuildVRT]: https://gdal.org/api/gdal_utils.html#gdal__utils_8h_1a057aaea8b0ed0476809a781ffa377ea4
+/// [program docs]: https://gdal.org/programs/gdalbuildvrt.html
+pub fn build_vrt<D: Borrow<Dataset>>(
+    dest: Option<&Path>,


Sorry for missing this. I guess dest: Option<P> ... where P: AsRef<Path> would be nicer, so you can call it with strings.

lnicola · 2022-01-12T20:07:54Z

r? @jdroenner

ayymart · 2022-01-12T22:07:04Z

@lnicola Taking dest: Option<P> ... where P: AsRef<Path> has made it so a type annotation is needed when passing None:

gdal::raster_programs::build_vrt(None, datasets, args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type for type parameter `P` declared on the function `build_vrt`

A default type cannot be specified because that is not allowed for functions:

error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions
pub fn build_vrt<D: Borrow<Dataset>, P: AsRef<Path> = &'static str>(
                                     ^

Is there a cleaner way to do this?

lnicola · 2022-01-12T22:47:20Z

Ah, good point. You can still specify the type, but it's an ugly API. Let's revert this change .

This reverts commit 41f8359.

rmanoka

@amartin96 Nice PR! The overall code looks great; my suggestions are mostly cosmetic.

rmanoka · 2022-01-14T16:11:50Z

src/lib.rs

@@ -42,6 +43,43 @@ pub use dataset::{
 pub use driver::Driver;
 pub use metadata::Metadata;

+/// Apply GeoTransform to x/y coordinate.


The GeoTransform relevant methods might be better placed inside the dataset or the rasters module. It is anyway only relevant to rasters IIUC.

rmanoka · 2022-01-14T16:13:58Z

src/lib.rs

+/// Wraps [GDALApplyGeoTransform].
+///
+/// [GDALApplyGeoTransform]: https://gdal.org/api/raster_c_api.html#_CPPv421GDALApplyGeoTransformPdddPdPd
+pub fn apply_geo_transform(geo_transform: &GeoTransform, pixel: f64, line: f64) -> (f64, f64) {


Would it be more ergonomic to throw these inside a trait that is impl by GeoTransform so we get the nicer:

let (wx, wy) = geo_t.apply(pix, line); let inv_t = geo_t.invert()?;

Optional suggestion.

rmanoka · 2022-01-14T16:15:33Z

src/raster_programs.rs

@@ -0,0 +1,107 @@
+use crate::{


Would be nice to move these into a sub-module raster_programs::vrt or similar. Not a major concern, but this mod. might get cluttered if we bring in more of the programs like Warp, etc.

rmanoka · 2022-01-14T16:17:23Z

src/raster_programs.rs

+///
+/// [GDALBuildVRT]: https://gdal.org/api/gdal_utils.html#gdal__utils_8h_1a057aaea8b0ed0476809a781ffa377ea4
+/// [program docs]: https://gdal.org/programs/gdalbuildvrt.html
+pub fn build_vrt<D: Borrow<Dataset>>(


Could we expose the BuildVRTOptions struct, and let this method take an Option<BuildVrtOptions> instead of a Vec<String> ? Seems more similar to the C api, and also a bit more natural.

rmanoka · 2022-01-14T16:25:42Z

src/raster_programs.rs

+    /// See [GDALBuildVRTOptionsNew].
+    ///
+    /// [GDALBuildVRTOptionsNew]: https://gdal.org/api/gdal_utils.html#_CPPv422GDALBuildVRTOptionsNewPPcP28GDALBuildVRTOptionsForBinary
+    fn new(args: Vec<String>) -> Result<Self> {


Does args have to be a Vec<String>? Wouldn't any IntoIterator<Item = ...> work? This would avoid allocations if it's a fixed sized array.

Going one more step, does the inner type have to be String or anything that impls TryInto<CString> (like Vec<U8> or Vec<NonZeroU8>, etc.)?

We can leave it as-is if this makes the API too complicated.

I attempted to implement this as follows:

pub fn new<S: TryInto<CString>, I: IntoIterator<Item = S>>(args: I) -> Result<Self> { let cstr_args = args.into_iter().map(|x| x.try_into()).collect::<std::result::Result<Vec<CString>, _>>()?;

However, I do not know how to convert TryInto<CString>::Error to GdalError::FfiNulError.

Can you show me how its done? Is the rest of it as you intended?

Hmm, good pt.; we can't give a good error for arbitrary conversion failures, and the user could anyway map as req. before passing it. How about:

fn new<S: Into<Vec<u8>>, I: IntoIterator<Item = S>>(args: I) -> Result<Self> { let c_args = args .into_iter() .map(|x| CString::new(x)) .collect::<std::result::Result<Vec<CString>, _>>()?; .... }

This is inline with the rest of the lib, where we expect &str to pass CString to the FFI. The above seems to accept both &str, and String, so that should be good.

ayymart · 2022-01-14T16:37:49Z

I will look into applying the latest feedback this weekend.

rmanoka · 2022-01-27T15:43:49Z

Changes look good to me; thanks again @amartin96 . Pinging @lnicola @jdroenner : could you pls. allow running CI and merge if all looks good?

lnicola · 2022-01-27T15:46:37Z

I took a hard page fault on this one 😄.

CHANGES.md

rmanoka · 2022-01-27T15:51:55Z

@amartin96 One final nitpick: it would be good to rename GeoTransformTrait as GeoTransformEx. That's the typical rust naming convention for traits to add functionalities.

lnicola · 2022-01-27T15:53:30Z

src/dataset.rs

+    ///
+    /// [GDALInvGeoTransform]: https://gdal.org/api/raster_c_api.html#_CPPv419GDALInvGeoTransformPdPd
+    fn invert(&self) -> Result<GeoTransform> {
+        let mut gt_out: GeoTransform = Default::default();


We could use MaybeUninit here, but it's fine to leave it like this.

Minor nit: I prefer keeping as much code as possible outside unsafe blocks, as in let rv = unsafe { ... }; if rv == 0 { ... }.

lnicola · 2022-01-27T16:02:19Z

src/programs/raster/vrt.rs

+        .map(|x| x.c_options as *const GDALBuildVRTOptions)
+        .unwrap_or(null());
+
+    let result = unsafe {


Same nit about large unsafe blocks.

Co-authored-by: Laurențiu Nicola <[email protected]>

…ild-vrt

lnicola · 2022-01-28T11:46:58Z

CHANGES.md

@@ -2,6 +2,13 @@

 ## Unreleased

+- Add `programs::raster::build_vrt`
+- Add `GeoTransformTrait` extension trait with `apply` and `invert`
+- Add `apply_geo_transform`


These two lines need to be removed

lnicola · 2022-01-28T11:47:11Z

bors d+

bors · 2022-01-28T11:47:12Z

✌️ amartin96 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

ayymart · 2022-01-28T21:31:38Z

bors r+

bors · 2022-01-28T21:37:07Z

Build succeeded:

321: Convert `byte_no_cf` and `cf_nasa_4326` to Zarr r=metasim,jdroenner a=lnicola - [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md). - [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Closes #239 I left `alldatatypes.nc` untouched because it's string arrays are not supported. Unfortunately, I had to disable these tests on pre-3.4 GDAL, where Zarr is not supported. Co-authored-by: Laurențiu Nicola <[email protected]>

ayymart added 5 commits January 10, 2022 01:12

Add build_vrt

453c536

Add apply_geo_transform

869d9d0

Add inv_geo_transform

7f69212

Update CHANGES.md

a95d519

Cargo fmt all

23dcedc