MaxRev-Dev / gdal.netcore

GDAL 3.x C#/F# bindings for .NET apps
MIT License
161 stars 36 forks source link

GDAL band data types missing from ReadRaster() overloads #111

Closed twest820 closed 1 year ago

twest820 commented 1 year ago

Is your feature request related to a problem? Please describe. Band and Dataset currently expose overloads for byte, double, float, int, and short. Overloads for byte, ushort and uint are missing, along with less commonly used GDAL datatypes such as GDT_Int8, GDT_Int64, GDT_UInt64, GDT_CFloat32, GDT_CFloat64, GDT_CInt16, and GDT_CInt32. Some avoidable hoop jumping is therefore needed to read rasters with these datatypes.

Describe the solution you'd like or your ideas here Since these are all just differently typed ways of moving bytes around, adding overloads for several of the remaining GDAL datatypes appears straightforward.

ReadRaster(..., sbyte buffer, ...)
ReadRaster(..., Int64 buffer, ...)
ReadRaster(..., UInt16 buffer, ...)
ReadRaster(..., UInt32 buffer, ...)
ReadRaster(..., UInt64 buffer, ...)
ReadRaster(..., Complex buffer, ...)

I'd suggest also

ReadRaster<TBand>(..., TBand buffer, ...) where TBand : INumber<TBand>

to compose with .NET 7, though some minor checking is needed to exclude types implementing INumber which lack corresponding GDAL support.

Not seeing System.Numerics has equivalents of GDT_CFloat32, GDT_CInt16, or GDT_CInt32—a Complex<T> appears to be absent.

Additional context My guess is UInt16 support is probably most valuable as it's frequently used with images. GDAL's GTiff driver doesn't currently support Int8 (sbyte), Int64, or UInt64 so those seem low priority.

A workaround I've had success with is to use Unsafe to bend the type to one of the same size. E.g.

                case DataType.GDT_UInt16:
                    rasterDataset.ReadRaster(xOff: 0, yOff: 0, xSize: this.XSize, ySize: this.YSize, Unsafe.As<Int16[]>(this.Data), buf_xSize: this.XSize, buf_ySize: this.YSize, this.Bands.Length, bandMap, pixelSpace: 0, lineSpace: 0, bandSpace: 0);
MaxRev-Dev commented 1 year ago

I have no control over this. Introducing some workarounds can be dangerous and "unsafe". This library does not extend or change GDAL's behaviour. As this issue is related directly to GDAL's bindings, please consider creating an issue in GDAL's repo. I found a similar PR for Java bindings here https://github.com/OSGeo/gdal/pull/7893. I believe this can be implemented in a similar manner.

MaxRev-Dev commented 1 year ago

The exact location of the change to be suggested https://github.com/OSGeo/gdal/blob/dfb76755c62061a3ea02f219e65dcc7a5c09075c/swig/include/csharp/gdal_csharp.i#L138-L151

MaxRev-Dev commented 1 year ago

The issue from the GDAL repo can be linked to this one. After it completes in GDAL's release it will be pulled up here. As for now, I'm closing this.

twest820 commented 9 months ago

Hmm, any sense of how long a fix might take to flow? The Java PR merged last August but gdal_csharp.i still hasn't updated in master or the 3.8 release branch.

Workaround above continues to work fine for read but this is rather a blocker for write (type bending fails as the .tif gets marked, reasonably, as having that datatype).

MaxRev-Dev commented 9 months ago

@twest820 That Java PR was merged and you can see it in the history of the master branch https://github.com/OSGeo/gdal/commits/master/swig/include/java/gdal_java.i

The gdal_csharp.i only refers to C# and must be updated in a separate PR. Java/Python/Csharp interfaces are not the same. They only have a common base interface.

twest820 commented 9 months ago

Yes, hence the question when (if?) such code motion might occur. Which follows from the above in this issue

After it completes in GDAL's release it will be pulled up here.

MaxRev-Dev commented 9 months ago

Around a few days/weeks after the GDAL release and could be per request, as I can miss the date. I'm running the pipelines with self-hosted machines and have no profit from it.

twest820 commented 9 months ago

Cool. Is there a way to find out when that GDAL release will happen? Didn't see anything tracking corresponding C# changes in the OSGeo repo when I searched a few days ago, though I might not have hit on the right keywords.

It's not that there aren't some workarounds—such as writing 32 bit signed integer and converting to 16 bit unsigned with tools which do have access to that binding—but testing's finding they're pretty cumbersome.

MaxRev-Dev commented 9 months ago

@twest820 https://github.com/OSGeo/gdal/milestones

twest820 commented 9 months ago

Yes. Since there doesn't appear to be a tracking work item it's unclear when the missing C# read/write raster bindings will be added—nothing obviously relevant is linked in 3.8.4, 3.9.0, or 4.0 that I could see. Hence the question about the timing.

MaxRev-Dev commented 9 months ago

You need it - you add it. PRs are welcome and you can help to modify the interface. https://github.com/OSGeo/gdal/pulls

Again. You're in the wrong place. This repo doesn't change the GDAL and there are official bindings that require GDAL installation on a target system. Once GDAL is updated I can build corresponding bindings with native libraries here.

twest820 commented 9 months ago

You need it - you add it.

Thank you for (finally) confirming. Policy between MaxRev and OSGeo over GDALdataset.RasterIO() p/invoke signatures aside, can't say it seems in I'm in the wrong place or GDAL changes are needed (desirable, aye, required, no).

I'm surprised Dataset's public buf_type overloads for data type flexible deserialization similar to

    TBand[] data; // where TBand : INumber<TBand>, alternatively Span<TBand> with GetPinnableReference(), fixed (TBand*) { cast to nint }, or a concrete array
    Band band = rasterDataset.GetRasterBand(1);
    int[] bandMap = [ 1, 2, 3, 4, ... ];
    GCHandle dataPin = GCHandle.Alloc(data, GCHandleType.Pinned);
    try
    {
        CPLErr gdalErrorCode = rasterDataset.ReadRaster(xOff: 0, yOff: 0, xSize: rasterDataset.RasterXSize, ySize: rasterDataset.RasterYSize, buffer: dataPin.AddrOfPinnedObject(), buf_xSize: rasterDataset.RasterXSize, buf_ySize: rasterDataset.RasterYSize, buf_type: band.DataType, bandCount: rasterDataset.RasterCount, bandMap, pixelSpace: 0, lineSpace: 0, bandSpace: 0);
        GdalException.ThrowIfError(gdalErrorCode, nameof(rasterDataset.WriteRaster));
    }
    finally
    {
        dataPin.Free();
    }

weren't indicated—didn't take long to check the implementation after guessing what to look for. Change ReadRaster() to WriteRaster() and this also works for serialization.

Given RasterIO(..., void *pData, ...) (nint in C#) it doesn't appear there's side effect concerns. I'm unsure what to propose on the GDAL side for GDT_CInt16, GDT_CInt32, and GDT_CFloat32 though as .NET lacks these types.