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

A Consistent and Principled Approach to GDAL C Pointers #445

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .cargo/config.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[alias]
# Run doctests, displaying compiler output
# doc-test-output: Run doctests, displaying compiler output
dto = "test --doc -- --show-output"
# Run clippy, raising warnings to errors
nowarn = "clippy --all-targets -- -D warnings"
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ ndarray = { version = "0.15", optional = true }
chrono = { version = "0.4.26", default-features = false }
bitflags = "2.4"
once_cell = "1.18"
foreign-types = "0.5.0"

[build-dependencies]
semver = "1.0"
Expand Down
3 changes: 1 addition & 2 deletions examples/rasterband.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use gdal::raster::RasterBand;
use gdal::{Dataset, Metadata};
use std::path::Path;

Expand All @@ -7,7 +6,7 @@ fn main() {
let dataset = Dataset::open(path).unwrap();
println!("dataset description: {:?}", dataset.description());

let rasterband: RasterBand = dataset.rasterband(1).unwrap();
let rasterband = dataset.rasterband(1).unwrap();
println!("rasterband description: {:?}", rasterband.description());
println!("rasterband no_data_value: {:?}", rasterband.no_data_value());
println!("rasterband type: {:?}", rasterband.band_type());
Expand Down
2 changes: 1 addition & 1 deletion examples/read_write_ogr_datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ fn run() -> gdal::errors::Result<()> {
for feature_a in layer_a.features() {
let mut ft = Feature::new(&defn)?;
if let Some(geom) = feature_a.geometry() {
ft.set_geometry(geom.clone())?;
ft.set_geometry(geom.to_owned())?;
}
// copy each field value of the feature:
for field in defn.fields() {
Expand Down
9 changes: 9 additions & 0 deletions fixtures/tinymarble.tif.aux.xml
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
<PAMDataset>
<SRS dataAxisToSRSAxisMapping="2,1">GEOGCS["WGS 84",DATUM["WGS_1984",SPHEROID["WGS 84",6378137,298.257223563,AUTHORITY["EPSG","7030"]],AUTHORITY["EPSG","6326"]],PRIMEM["Greenwich",0,AUTHORITY["EPSG","8901"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AXIS["Latitude",NORTH],AXIS["Longitude",EAST],AUTHORITY["EPSG","4326"]]</SRS>
<PAMRasterBand band="1">
<Metadata>
<MDI key="STATISTICS_MAXIMUM">255</MDI>
<MDI key="STATISTICS_MEAN">68.4716</MDI>
<MDI key="STATISTICS_MINIMUM">0</MDI>
<MDI key="STATISTICS_STDDEV">83.68444773935</MDI>
<MDI key="STATISTICS_VALID_PERCENT">100</MDI>
</Metadata>
</PAMRasterBand>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert this.

</PAMDataset>
128 changes: 60 additions & 68 deletions src/dataset.rs
Original file line number Diff line number Diff line change
@@ -1,29 +1,34 @@
use foreign_types::{foreign_type, ForeignType, ForeignTypeRef};
use std::fmt::{Debug, Formatter};
use std::mem::ManuallyDrop;
use std::{ffi::CString, ffi::NulError, path::Path, ptr};

use gdal_sys::{self, CPLErr, GDALDatasetH, GDALMajorObjectH};
use gdal_sys::{self, CPLErr, GDALMajorObjectH};

use crate::cpl::CslStringList;
use crate::errors::*;
use crate::options::DatasetOptions;
use crate::raster::RasterCreationOption;
use crate::spatial_ref::SpatialRefRef;
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};
use crate::{
gdal_major_object::MajorObject, spatial_ref::SpatialRef, Driver, GeoTransform, Metadata,
};

/// Wrapper around a [`GDALDataset`][GDALDataset] object.
///
/// Represents both a [vector dataset][vector-data-model]
/// containing a collection of layers; and a
/// [raster dataset][raster-data-model] containing a collection of raster-bands.
///
/// [vector-data-model]: https://gdal.org/user/vector_data_model.html
/// [raster-data-model]: https://gdal.org/user/raster_data_model.html
/// [GDALDataset]: https://gdal.org/api/gdaldataset_cpp.html#_CPPv411GDALDataset
#[derive(Debug)]
pub struct Dataset {
c_dataset: GDALDatasetH,
closed: bool,
foreign_type! {
/// Wrapper around a [`GDALDataset`][GDALDataset] object.
///
/// Represents both a [vector dataset][vector-data-model]
/// containing a collection of layers; and a
/// [raster dataset][raster-data-model] containing a collection of raster-bands.
///
/// [vector-data-model]: https://gdal.org/user/vector_data_model.html
/// [raster-data-model]: https://gdal.org/user/raster_data_model.html
/// [GDALDataset]: https://gdal.org/api/gdaldataset_cpp.html#_CPPv411GDALDataset
pub unsafe type Dataset {
type CType = libc::c_void;
fn drop = gdal_sys::GDALClose;
}
}

// GDAL Docs state: The returned dataset should only be accessed by one thread at a time.
Expand All @@ -35,26 +40,6 @@ unsafe impl Send for Dataset {}

/// Core dataset methods
impl Dataset {
/// Returns the wrapped C pointer
///
/// # Safety
/// This method returns a raw C pointer
pub fn c_dataset(&self) -> GDALDatasetH {
self.c_dataset
}

/// Creates a new Dataset by wrapping a C pointer
///
/// # Safety
/// This method operates on a raw C pointer
/// The dataset must not have been closed (using [`GDALClose`]) before.
pub unsafe fn from_c_dataset(c_dataset: GDALDatasetH) -> Dataset {
Dataset {
c_dataset,
closed: false,
}
}

/// Open a dataset at the given `path` with default
/// options.
pub fn open<P: AsRef<Path>>(path: P) -> Result<Dataset> {
Expand Down Expand Up @@ -156,10 +141,7 @@ impl Dataset {
if c_dataset.is_null() {
return Err(_last_null_pointer_err("GDALOpenEx"));
}
Ok(Dataset {
c_dataset,
closed: false,
})
Ok(unsafe { Dataset::from_ptr(c_dataset) })
}

/// Flush all write cached data to disk.
Expand All @@ -170,67 +152,76 @@ impl Dataset {
pub fn flush_cache(&mut self) -> Result<()> {
#[cfg(any(all(major_ge_3, minor_ge_7), major_ge_4))]
{
let rv = unsafe { gdal_sys::GDALFlushCache(self.c_dataset) };
let rv = unsafe { gdal_sys::GDALFlushCache(self.as_ptr()) };
if rv != CPLErr::CE_None {
return Err(_last_cpl_err(rv));
}
}
#[cfg(not(any(all(major_is_3, minor_ge_7), major_ge_4)))]
{
unsafe {
gdal_sys::GDALFlushCache(self.c_dataset);
gdal_sys::GDALFlushCache(self.as_ptr());
}
}
Ok(())
}

/// Close the dataset.
/// Close the dataset, consuming it.
///
/// See [`GDALClose`].
/// See [`GDALClose`](https://gdal.org/api/raster_c_api.html#_CPPv49GDALClose12GDALDatasetH).
///
/// Note: on GDAL versions older than 3.7.0, this function always succeeds.
pub fn close(mut self) -> Result<()> {
self.closed = true;

/// # Notes
/// * `Drop`ing automatically closes the Dataset, so this method is usually not needed.
/// * On GDAL versions older than 3.7.0, this function always succeeds.
pub fn close(self) -> Result<()> {
// According to the C documentation, GDALClose will release all resources
// using the C++ `delete` operator.
// According to https://doc.rust-lang.org/std/mem/fn.forget.html#relationship-with-manuallydrop
// `ManuallyDrop` is the suggested means of transferring memory management while
// robustly preventing a double-free.
let ds = ManuallyDrop::new(self);
#[cfg(any(all(major_ge_3, minor_ge_7), major_ge_4))]
{
let rv = unsafe { gdal_sys::GDALClose(self.c_dataset) };
let rv = unsafe { gdal_sys::GDALClose(ds.as_ptr()) };
if rv != CPLErr::CE_None {
return Err(_last_cpl_err(rv));
}
}
#[cfg(not(any(all(major_is_3, minor_ge_7), major_ge_4)))]
{
unsafe {
gdal_sys::GDALClose(self.c_dataset);
gdal_sys::GDALClose(ds.as_ptr());
}
}
Ok(())
}

/// Fetch the projection definition string for this dataset.
pub fn projection(&self) -> String {
let rv = unsafe { gdal_sys::GDALGetProjectionRef(self.c_dataset) };
let rv = unsafe { gdal_sys::GDALGetProjectionRef(self.as_ptr()) };
_string(rv)
}

/// Set the projection reference string for this dataset.
pub fn set_projection(&mut self, projection: &str) -> Result<()> {
let c_projection = CString::new(projection)?;
unsafe { gdal_sys::GDALSetProjection(self.c_dataset, c_projection.as_ptr()) };
unsafe { gdal_sys::GDALSetProjection(self.as_ptr(), c_projection.as_ptr()) };
Ok(())
}

#[cfg(major_ge_3)]
/// Get the spatial reference system for this dataset.
pub fn spatial_ref(&self) -> Result<SpatialRef> {
unsafe { SpatialRef::from_c_obj(gdal_sys::GDALGetSpatialRef(self.c_dataset)) }
Ok(
unsafe { SpatialRefRef::from_ptr(gdal_sys::GDALGetSpatialRef(self.as_ptr())) }
.to_owned(),
)
}

#[cfg(major_ge_3)]
/// Set the spatial reference system for this dataset.
pub fn set_spatial_ref(&mut self, spatial_ref: &SpatialRef) -> Result<()> {
let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.c_dataset, spatial_ref.to_c_hsrs()) };
let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.as_ptr(), spatial_ref.as_ptr()) };
if rv != CPLErr::CE_None {
return Err(_last_cpl_err(rv));
}
Expand Down Expand Up @@ -260,7 +251,7 @@ impl Dataset {
gdal_sys::GDALCreateCopy(
driver.c_driver(),
c_filename.as_ptr(),
ds.c_dataset,
ds.as_ptr(),
0,
c_options.as_ptr(),
None,
Expand All @@ -270,15 +261,15 @@ impl Dataset {
if c_dataset.is_null() {
return Err(_last_null_pointer_err("GDALCreateCopy"));
}
Ok(unsafe { Dataset::from_c_dataset(c_dataset) })
Ok(unsafe { Dataset::from_ptr(c_dataset) })
}
_create_copy(self, driver, filename.as_ref(), options)
}

/// Fetch the driver to which this dataset relates.
pub fn driver(&self) -> Driver {
unsafe {
let c_driver = gdal_sys::GDALGetDatasetDriver(self.c_dataset);
let c_driver = gdal_sys::GDALGetDatasetDriver(self.as_ptr());
Driver::from_c_driver(c_driver)
}
}
Expand All @@ -299,7 +290,7 @@ impl Dataset {
pub fn set_geo_transform(&mut self, transformation: &GeoTransform) -> Result<()> {
assert_eq!(transformation.len(), 6);
let rv = unsafe {
gdal_sys::GDALSetGeoTransform(self.c_dataset, transformation.as_ptr() as *mut f64)
gdal_sys::GDALSetGeoTransform(self.as_ptr(), transformation.as_ptr() as *mut f64)
};
if rv != CPLErr::CE_None {
return Err(_last_cpl_err(rv));
Expand All @@ -319,7 +310,7 @@ impl Dataset {
pub fn geo_transform(&self) -> Result<GeoTransform> {
let mut transformation = GeoTransform::default();
let rv =
unsafe { gdal_sys::GDALGetGeoTransform(self.c_dataset, transformation.as_mut_ptr()) };
unsafe { gdal_sys::GDALGetGeoTransform(self.as_ptr(), transformation.as_mut_ptr()) };

// check if the dataset has a GeoTransform
if rv != CPLErr::CE_None {
Expand All @@ -329,24 +320,25 @@ impl Dataset {
}
}

impl Debug for Dataset {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Dataset")
.field(
"description",
&self.description().unwrap_or_else(|_| Default::default()),
)
.finish()
}
}

impl MajorObject for Dataset {
fn gdal_object_ptr(&self) -> GDALMajorObjectH {
self.c_dataset
self.as_ptr()
}
}

impl Metadata for Dataset {}

impl Drop for Dataset {
fn drop(&mut self) {
if !self.closed {
unsafe {
gdal_sys::GDALClose(self.c_dataset);
}
}
}
}

#[cfg(test)]
mod tests {
use gdal_sys::GDALAccess;
Expand Down
3 changes: 2 additions & 1 deletion src/driver.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use foreign_types::ForeignType;
use std::ffi::CString;
use std::path::Path;
use std::sync::Once;
Expand Down Expand Up @@ -223,7 +224,7 @@ impl Driver {
return Err(_last_null_pointer_err("GDALCreate"));
};

Ok(unsafe { Dataset::from_c_dataset(c_dataset) })
Ok(unsafe { Dataset::from_ptr(c_dataset) })
}

/// Convenience for creating a vector-only dataset from a compatible driver.
Expand Down
20 changes: 11 additions & 9 deletions src/gcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ use std::marker::PhantomData;
use gdal_sys::CPLErr;

use crate::errors::Result;
use crate::spatial_ref::SpatialRef;
use crate::spatial_ref::{SpatialRef, SpatialRefRef};
use crate::utils::{_last_cpl_err, _string};
use crate::Dataset;
use foreign_types::{ForeignType, ForeignTypeRef};

/// An owned Ground Control Point.
#[derive(Clone, Debug, PartialEq, PartialOrd)]
Expand Down Expand Up @@ -115,13 +116,14 @@ impl Dataset {
///
/// See: [`GDALGetGCPSpatialRef`](https://gdal.org/api/raster_c_api.html#_CPPv420GDALGetGCPSpatialRef12GDALDatasetH)
pub fn gcp_spatial_ref(&self) -> Option<SpatialRef> {
let c_ptr = unsafe { gdal_sys::GDALGetGCPSpatialRef(self.c_dataset()) };
let c_ptr = unsafe { gdal_sys::GDALGetGCPSpatialRef(self.as_ptr()) };

if c_ptr.is_null() {
return None;
}

unsafe { SpatialRef::from_c_obj(c_ptr) }.ok()
// NB: Creating a `SpatialRefRef` from a pointer acknowledges that GDAL is giving us
// a shared reference. We then call `to_owned` to appropriately get a clone.
Some(unsafe { SpatialRefRef::from_ptr(c_ptr) }.to_owned())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lnicola said in 7db2868#r128939164

We could return a SpatialRefRef here, right?
One more reason to rename these to SpatialReference.

}

/// Get the projection definition string for the GCPs in this dataset.
Expand All @@ -132,7 +134,7 @@ impl Dataset {
///
/// See: [`GDALGetGCPProjection`](https://gdal.org/api/raster_c_api.html#gdal_8h_1a85ffa184d3ecb7c0a59a66096b22b2ec)
pub fn gcp_projection(&self) -> Option<String> {
let cc_ptr = unsafe { gdal_sys::GDALGetGCPProjection(self.c_dataset()) };
let cc_ptr = unsafe { gdal_sys::GDALGetGCPProjection(self.as_ptr()) };
if cc_ptr.is_null() {
return None;
}
Expand All @@ -143,8 +145,8 @@ impl Dataset {
///
/// See: [`GDALDataset::GetGCPs`](https://gdal.org/api/gdaldataset_cpp.html#_CPPv4N11GDALDataset7GetGCPsEv)
pub fn gcps(&self) -> &[GcpRef] {
let len = unsafe { gdal_sys::GDALGetGCPCount(self.c_dataset()) };
let data = unsafe { gdal_sys::GDALGetGCPs(self.c_dataset()) };
let len = unsafe { gdal_sys::GDALGetGCPCount(self.as_ptr()) };
let data = unsafe { gdal_sys::GDALGetGCPs(self.as_ptr()) };
unsafe { std::slice::from_raw_parts(data as *const GcpRef, len as usize) }
}

Expand Down Expand Up @@ -204,10 +206,10 @@ impl Dataset {

let rv = unsafe {
gdal_sys::GDALSetGCPs2(
self.c_dataset(),
self.as_ptr(),
len,
gdal_gcps.as_ptr(),
spatial_ref.to_c_hsrs() as *mut _,
spatial_ref.as_ptr() as *mut _,
)
};

Expand Down
4 changes: 2 additions & 2 deletions src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ mod tests {
#[test]
fn test_set_description() {
let driver = DriverManager::get_driver_by_name("MEM").unwrap();
let dataset = driver.create("", 1, 1, 1).unwrap();
let mut band = dataset.rasterband(1).unwrap();
let mut dataset = driver.create("", 1, 1, 1).unwrap();
let band = dataset.rasterband_mut(1).unwrap();

let description = "A merry and cheerful band description";
assert_eq!(band.description().unwrap(), "");
Expand Down
Loading