Add histogram calculation for RasterBand

[spadarian]

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

---

This PR exposes 2 GDAL methods to calculate band histograms (default and user-defined).

[metasim]

Consider naming buckets or counts? With this name, you'll end up with things like this:

let rb: Rasterband<'_> = ...;
let histogram = rb.get_histogram(...);

for count in histogram.histogram() { // <--- awkward.
...
}

[spadarian]

I also found it weird but I was just being consistent with GDAL naming. Happy to change it if we are not aiming for that.

[metasim]

Why isn't this usize? Sometimes GDAL uses signed values when it doesn't make semantic sense, and there's precedence for rectifying this in Rust.

[spadarian]

I haven't contributed for a while and from memory we were using isize. I do agree that we could rectify that.

How far do we want to take it? I could also use a NonZeroUsize, which could be also useful when selecting a band from a dataset... (@lnicola)

[metasim]

Lol... yeah, I've argued for NonZeroUsize, but creating them is really annoying/verbose.

When I see isize or i32 I immediately ask myself "What does a negative value mean?". If it has meaning, we should document it. If it doesn't, then I'd at least try in the bindings to coerce it into an unsigned type.

[lnicola]

Yeah, I wish constructing a NonZeroUsize from a constant/literal was possible without unsafe or bloating the binary. 😔

[metasim]

Is this storing the counts twice? Who owns histogram?

It would really help to document why this is neccessary. :)

[spadarian]

Yes, sorry. I should document that. I used that double storage mechanisms because with GDALGetDefaultHistogram C owns histogram and with GDALGetRasterHistogram Rust does.

[metasim]

Here's a restating of my idea from before. Does this capture the semantics? I should probably just compile and step through the tests, but I'm admittedly confused if I've gotten the borrowed vs owned cases switched...

pub struct Histogram {
    min: f64,
    max: f64,
    n_buckets: i32,
    counts: HistCounts
}

enum HistCounts {
   Borrowed(*mut libc::c_void),
   Owned(Vec<i32>)
}

...

impl Drop for HistCounts {
    fn drop(&mut self) {
        match self {
            Self::Borrowed(p) => ???;
            Self::Owned(v) => ???;
        }
    }
}

[spadarian]

No worries mate. I'm pushing those changes now.

[metasim]

For coherence with other methods/functions, I'd consider collapsing these two methods into a single one accepting a new HistogramOptions with a Default implementation setting the GDALGetDefaultHistogram settings.

@lnicola Do you agree with me? Even though there are separate GDALGetDefaultHistogram and GDALGetRasterHistogram binding functions, we could be more idiomatic with the HistogramOptions approach.

[spadarian]

I do prefer thin wrappers to avoid learning a new API but I'm happy to make it more idiomatic if you guys want.

[metasim]

Fair point. Let's hear what some of the other contributors think. I'm on the extreme of "let's not leak C-language workarounds" and "let's leverage Rust to improve ergonomics". Others, I think, are more in your camp, so let's let someone else make the call. We've had related debates here.

[metasim]

I personally think the counts should be usize or u32, regardless of what GDAL C returns, but not sure that's a consensus opinion. @lnicola @jdroenner your opinion?

[spadarian]

I saw that @lnicola did something similar with raster index, so probably it makes sense to do it like that moving forward.

[spadarian]

I've incorporated some of the changes suggested by @metasim, including:

• Double storage mechanism for Borrowed and Owned histogram counts (makes sense and it is hidden from the user)
• Changing isize to usize since we recently changed that for raster indexing

I did not merge the calls for GDALGetDefaultHistogram and GDALGetRasterHistogram since I think having a API more or less consistent with the C code is desirable. Open to change it if that is where the project is going @lnicola @jdroenner

[metasim]

@spadarian Take a look at this PR against your repo for an alternative naming and associated documentation, which I think clarifies things for a future contributor.

The other option (assuming VSIFree is required instead of using drop), is to have default_histogram copy the allocated array into the same Vec as used by histogram, then VSIFree the array. I personally like this approach, but I would expect some to balk at the extra copy. Makes no difference to the user tho.

[lnicola]

We should also add set_default_histogram, but it's fine to do it in another PR.

[spadarian]

I addressed all the comments... but CI failed on a unrelated test ( raster::processing::dem::slope::tests::test_slope)

[lnicola]

Error: CplError { class: 3, number: 1, msg: "/__w/gdal/gdal/target/dem-hills-slope.tiff, band 1: IReadBlock failed at X offset 0, Y offset 0: TIFFReadEncodedStrip() failed." }

❗

[lnicola]

Ah, there's a race between tpi and slope. I can try to fix it tomorrow.

Should we use /vsimem/ here?

[metasim]

@spadarian Will you accept this? spadarian#12

Thanks for merging! :)

[lnicola]

@metasim isn't that already merged?

[metasim]

@metasim isn't that already merged?

Oops! Yes. Sorry. Was looking at a non-refreshed page.

[rouault]

little robustness fix in spadarian#13

[spadarian]

Rebased @lnicola

[lnicola]

@metasim this seems to use u64 consistently for the counts, did you have some concerns about that?

[metasim]

No, sounds fine. I just had comments about it to make sure we used the .*Ex variants in boths cases.