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.

Sign up for GitHub

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

Fixes leakage of GDALDataType::Type in RasterBand. #334

Merged
merged 7 commits into from
Nov 29, 2022
Merged

Fixes leakage of GDALDataType::Type in RasterBand. #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0916638
Fixes leakage of GDALDataType::Type in RasterBand.
metasim Nov 13, 2022
66e87f6
Changelog.
metasim Nov 13, 2022
bb98976
Hold back chrono version until deprecations can be addressed.
metasim Nov 14, 2022
86f90e3
Refined chrono version range.
metasim Nov 15, 2022
739750d
Merge branch 'master' into fix/333
metasim Nov 20, 2022
1c07337
Update CHANGES.md
metasim Nov 28, 2022
a577a93
Reverted bindgen-caused warning suppressions.
metasim Nov 28, 2022
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
4 changes: 4 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

- **Breaking**: `RasterBand::band_type` returns the `GdalDataType` enum instead of `GDALDataType::Type` ordinal. Fixes [#333](https://github.com/georust/gdal/issues/333)

- <https://github.com/georust/gdal/pull/334>

## 0.14

- Added new content to `README.md` and the root docs.
Expand Down
1 change: 1 addition & 0 deletions gdal-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
#![allow(non_snake_case)]
#![allow(clippy::upper_case_acronyms)]
#![allow(rustdoc::bare_urls)]

include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
include!(concat!(env!("OUT_DIR"), "/docs_rs_helper.rs"));
12 changes: 6 additions & 6 deletions src/driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,12 @@ impl Driver {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::DriverManager;
/// use gdal::raster::GdalType;
/// use gdal::raster::GdalDataType;
/// let d = DriverManager::get_driver_by_name("MEM")?;
/// let ds = d.create("in-memory", 64, 64, 3)?;
/// assert_eq!(ds.raster_count(), 3);
/// assert_eq!(ds.raster_size(), (64, 64));
/// assert_eq!(ds.rasterband(1)?.band_type(), u8::gdal_ordinal());
/// assert_eq!(ds.rasterband(1)?.band_type(), GdalDataType::UInt8);
/// # Ok(())
/// # }
/// ```
Expand All @@ -122,12 +122,12 @@ impl Driver {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::DriverManager;
/// use gdal::raster::GdalType;
/// use gdal::raster::GdalDataType;
/// let d = DriverManager::get_driver_by_name("MEM")?;
/// let ds = d.create_with_band_type::<f64, _>("in-memory", 64, 64, 3)?;
/// assert_eq!(ds.raster_count(), 3);
/// assert_eq!(ds.raster_size(), (64, 64));
/// assert_eq!(ds.rasterband(1)?.band_type(), f64::gdal_ordinal());
/// assert_eq!(ds.rasterband(1)?.band_type(), GdalDataType::Float64);
/// # Ok(())
/// # }
/// ```
Expand Down Expand Up @@ -155,7 +155,7 @@ impl Driver {
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::DriverManager;
/// use gdal::raster::RasterCreationOption;
/// use gdal::raster::GdalType;
/// use gdal::raster::GdalDataType;
/// use gdal::spatial_ref::SpatialRef;
/// let d = DriverManager::get_driver_by_name("BMP")?;
/// let options = [
Expand All @@ -168,7 +168,7 @@ impl Driver {
/// ds.set_spatial_ref(&SpatialRef::from_epsg(4326)?)?;
/// assert_eq!(ds.raster_count(), 1);
/// assert_eq!(ds.raster_size(), (64, 64));
/// assert_eq!(ds.rasterband(1)?.band_type(), u8::gdal_ordinal());
/// assert_eq!(ds.rasterband(1)?.band_type(), GdalDataType::UInt8);
metasim marked this conversation as resolved.
Show resolved Hide resolved
/// assert_eq!(ds.spatial_ref()?.auth_code()?, 4326);
/// # Ok(())
/// # }
Expand Down
22 changes: 11 additions & 11 deletions src/raster/rasterband.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use crate::dataset::Dataset;
use crate::gdal_major_object::MajorObject;
use crate::metadata::Metadata;
use crate::raster::GdalType;
use crate::raster::{GdalDataType, GdalType};
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _string};
use gdal_sys::{
self, CPLErr, GDALColorEntry, GDALColorInterp, GDALColorTableH, GDALComputeRasterMinMax,
GDALCreateColorRamp, GDALCreateColorTable, GDALDataType, GDALDestroyColorTable,
GDALGetPaletteInterpretation, GDALGetRasterStatistics, GDALMajorObjectH, GDALPaletteInterp,
GDALRIOResampleAlg, GDALRWFlag, GDALRasterBandH, GDALRasterIOExtraArg, GDALSetColorEntry,
GDALSetRasterColorTable,
GDALCreateColorRamp, GDALCreateColorTable, GDALDestroyColorTable, GDALGetPaletteInterpretation,
GDALGetRasterStatistics, GDALMajorObjectH, GDALPaletteInterp, GDALRIOResampleAlg, GDALRWFlag,
GDALRasterBandH, GDALRasterIOExtraArg, GDALSetColorEntry, GDALSetRasterColorTable,
};
use libc::c_int;
use std::ffi::CString;
Expand Down Expand Up @@ -230,10 +229,10 @@ impl<'a> RasterBand<'a> {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::Dataset;
/// use gdal::raster::{GdalType, ResampleAlg};
/// use gdal::raster::{GdalDataType, ResampleAlg};
/// let dataset = Dataset::open("fixtures/m_3607824_se_17_1_20160620_sub.tif")?;
/// let band1 = dataset.rasterband(1)?;
/// assert_eq!(band1.band_type(), u8::gdal_ordinal());
/// assert_eq!(band1.band_type(), GdalDataType::UInt8);
/// let size = 4;
/// let mut buf = vec![0; size*size];
/// band1.read_into_slice::<u8>((0, 0), band1.size(), (size, size), buf.as_mut_slice(), Some(ResampleAlg::Bilinear))?;
Expand Down Expand Up @@ -300,10 +299,10 @@ impl<'a> RasterBand<'a> {
/// ```rust, no_run
/// # fn main() -> gdal::errors::Result<()> {
/// use gdal::Dataset;
/// use gdal::raster::{GdalType, ResampleAlg};
/// use gdal::raster::{GdalDataType, ResampleAlg};
/// let dataset = Dataset::open("fixtures/m_3607824_se_17_1_20160620_sub.tif")?;
/// let band1 = dataset.rasterband(1)?;
/// assert_eq!(band1.band_type(), u8::gdal_ordinal());
/// assert_eq!(band1.band_type(), GdalDataType::UInt8);
/// let size = 4;
/// let buf = band1.read_as::<u8>((0, 0), band1.size(), (size, size), Some(ResampleAlg::Bilinear))?;
/// assert_eq!(buf.size, (size, size));
Expand Down Expand Up @@ -476,8 +475,9 @@ impl<'a> RasterBand<'a> {
}

/// Returns the pixel datatype of this band.
pub fn band_type(&self) -> GDALDataType::Type {
unsafe { gdal_sys::GDALGetRasterDataType(self.c_rasterband) }
pub fn band_type(&self) -> GdalDataType {
let ordinal = unsafe { gdal_sys::GDALGetRasterDataType(self.c_rasterband) };
ordinal.try_into().unwrap_or(GdalDataType::Unknown)
Copy link
Member

Choose a reason for hiding this comment

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

Just looking at the code in the browser, can we call GdalType::datatype() here instead?

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 Not sure what you mean... GdalType::datatype() would be to get the datatype most closely associated with a static primitive. In this case, we're getting a c_int from C-GDAL and need to map that into the GdalDataType enum. But perhaps I'm missing what you're suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

Basically ordinal.datatype() instead of ordinal.try_into().unwrap_or(GdalDataType::Unknown). Maybe it doesn't work for c_int, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, it doesn't work. GdalType is a (wierd) type-level trait, with no self parameter:

gdal/src/raster/types.rs

Line 330 in 029b95a

fn datatype() -> GdalDataType {

In Scala I'd have called this an "evidence" trait. So <u8>::datatype() exists, but 3u8.datatype() does not.

}

/// Returns the no-data value of this band.
Expand Down
15 changes: 7 additions & 8 deletions src/raster/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ use crate::dataset::Dataset;
use crate::metadata::Metadata;
use crate::raster::rasterband::ResampleAlg;
use crate::raster::{
ByteBuffer, ColorEntry, ColorInterpretation, ColorTable, RasterCreationOption, StatisticsAll,
StatisticsMinMax,
ByteBuffer, ColorEntry, ColorInterpretation, ColorTable, GdalDataType, RasterCreationOption,
StatisticsAll, StatisticsMinMax,
};
use crate::test_utils::TempFixture;
use crate::vsi::unlink_mem_file;
use crate::DriverManager;
use gdal_sys::GDALDataType;
use std::path::Path;

#[cfg(feature = "ndarray")]
Expand Down Expand Up @@ -279,7 +278,7 @@ fn test_create_with_band_type() {
assert_eq!(dataset.raster_count(), 3);
assert_eq!(dataset.driver().short_name(), "MEM");
let rb = dataset.rasterband(1).unwrap();
assert_eq!(rb.band_type(), GDALDataType::GDT_Float32)
assert_eq!(rb.band_type(), GdalDataType::Float32);
}

#[test]
Expand Down Expand Up @@ -407,7 +406,7 @@ fn test_read_raster_as() {
assert_eq!(rv.data, vec!(7, 7, 7, 10, 8, 12));
assert_eq!(rv.size.0, 2);
assert_eq!(rv.size.1, 3);
assert_eq!(rb.band_type(), GDALDataType::GDT_Byte);
assert_eq!(rb.band_type(), GdalDataType::UInt8);
}

#[test]
Expand Down Expand Up @@ -468,7 +467,7 @@ fn test_read_raster_as_array() {
]);

assert_eq!(values, data);
assert_eq!(rb.band_type(), GDALDataType::GDT_Byte);
assert_eq!(rb.band_type(), GdalDataType::UInt8);
}

#[test]
Expand Down Expand Up @@ -525,7 +524,7 @@ fn test_get_band_type() {
let driver = DriverManager::get_driver_by_name("MEM").unwrap();
let dataset = driver.create("", 20, 10, 1).unwrap();
let rb = dataset.rasterband(1).unwrap();
assert_eq!(rb.band_type(), GDALDataType::GDT_Byte);
assert_eq!(rb.band_type(), GdalDataType::UInt8);
}

#[test]
Expand Down Expand Up @@ -817,7 +816,7 @@ fn test_create_color_table() {
// Confirm we have a band without a color table.
assert_eq!(dataset.raster_count(), 1);
let band = dataset.rasterband(1).unwrap();
assert_eq!(band.band_type(), GDALDataType::GDT_Byte);
assert_eq!(band.band_type(), GdalDataType::UInt8);
assert!(band.color_table().is_none());

// Create a new file to put color table in
Expand Down
4 changes: 3 additions & 1 deletion src/raster/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,9 @@ pub trait GdalType {
/// ```
fn datatype() -> GdalDataType {
// We can call `unwrap` because existence is guaranteed in this case.
Self::gdal_ordinal().try_into().expect("GdalDataType")
Self::gdal_ordinal()
.try_into()
.unwrap_or(GdalDataType::Unknown)
}
}

Expand Down