[Gdal-dev] C# Bindings

[Tamas Szekeres]

> > I've added the base implementation with the following signature into
> > the CVS:
> >
> > public int ReadRasterInternal(int xOff, int yOff, int xSize, int
> > ySize, IntPtr buffer, int buf_xSize, int buf_ySize, int buf_type, int
> > pixelSpace, int lineSpace);
> >
> > public int WriteRasterInternal(int xOff, int yOff, int xSize, int
> > ySize, IntPtr buffer, int buf_xSize, int buf_ySize, int buf_type, int
> > pixelSpace, int lineSpace);
>
> That seems fine. Though I guess it feels slightly odd to me for a public
> function to be called "Internal". How about just calling these functions
> ReadRaster() and WriteRaster(), same as the more type safe functions,
> but just using a different set of arguments?

I see your option but to achieve this i shall have to change the common Band.i
interface file a bit for hiding the original ReadRaster and
WriteRaster implementation from the C# interface.
The current approach uses %ignore GDALRasterBandShadow::ReadRaster;
might prevent from
creating another implementation for these items.
But I will take care of this issue, since I've finally left these
functions public.

BTW, how about Dataset.WriteRaster? Is it necessary to implement?

>
> And it worked? Hmm, I notice that you use Format24bppRgb when creating
> the Bitmap, and then Format32bppRgb when locking it. I'm surprised that
> didn't throw an exception. Also the pixelspace of 4 is appropriate for
> 32bpp, but not for 24bpp. Perhaps windows uses 4 bytes per pixel even
> when you specify 24bpp?
>

Indeed. But it works. And the resulting image differs in bit depth
depending on the
pixelformat selection when creating the Bitmap. ;-)

> >
> > At this point I would avoid creating /unsafe code into the interface
> > so I will leave the user alone with the pointers and the fixed
> > statements.
> > However I would add the specializations depending on the array types
> > using Marshal.Copy
> > between the managed and the unmanaged memory.
>
> So how does this work? i.e. where does the unmanaged buffer that
> GDALRasterIO initially reads into get allocated? In C code somewhere? Or
> is there some way of getting an IntPtr pointing to an allocated buffer
> in C#? And then you copy it into a C# array in some CS code? And then
> switch back to C to free the unmanaged memory? And you can do all this
> without using the unsafe keyword? Guess it'll be easier to visualize it
> when you've written it.
>

With this solution the unmanaged memory would be allocated using
Marshal.AllocHGlobal at the marshaling
code and released after the call as for example (untested):

IntPtr ptr = Marshal.AllocHGlobal(buffer.length);
Mashal.Copy(buffer, 0, ptr, buffer.length);
WriteRasterInternal(xOff, yOff, xSize, ySize, ptr, pixelSpace, lineSpace);
Marshal.FreeHGlobal(ptr);

or

IntPtr ptr = Marshal.AllocHGlobal(buffer.length);
ReadRasterInternal(xOff, yOff, xSize, ySize, ptr, pixelSpace, lineSpace);
Mashal.Copy(ptr, buffer, 0, buffer.length);
Marshal.FreeHGlobal(ptr);


> FWIW, my personal feeling is that writing code like this that doesn't
> use the unsafe keyword makes you feel better, but when you look more
> closely it's a pretty dubious benefit. At the end of the day, you're
> passing pointers to buffers into unmanaged C code, and so really, that
> SHOULD be declared as unsafe. That C code could do all kinds of nasty
> things to your C# data structures, outside of your control. The fact
> that you can get around it by using IntPtr seems to me to be more a
> failing of the language, than a genuinely "safe" way to do things.
> Unless I'm missing something.
>

I consider your motion is conceptionally different, since it implies
the usage the garbage collected
'managed' memory at the unmanaged side requiring to prevent the memory
from unexpected
relocations and GC collections by using the fixed statement. In this
regard I really
feel better passing the memory to ReadRaster/WriteRaster allocated by
CPLMalloc for example.
The default behaviour of the SWIG generated wappers is that allocating
and freeing
the unmanaged memory takes place at the unmanaged side.
Since I cannot see the actual implementation how the several MS.NET FW
variations and MONO on
different platforms implement this issue and what potential problems
it may involve. It might
perform pretty well but not the 'safest' way from my aspect. It might
also require to compile the
interface assemblies using the /unsafe option.

Although we can do almost anything with SWIG I would not keen to go
too far with theese
variations. However I would not discourage the folks to use the C#
interface that way.
I think we would also require to make a documentation describing this
kind 'advanced' usage of the
interface and some sample apps would also be appreciated.

> Anyway, I'm still not especially keen on any extra copying of buffers.
> As long as the direct interface using IntPtr is available, I guess users
> can always choose to write their own more efficient methods to read GDAL
> data into arrays, using fixed() { }. But still, seems a shame to build
> in inefficiency just to avoid using the unsafe keyword, when in my
> opinion, the code should not be considered "safe" anyway.
>
>

Since I am not an expert in the usage of the GDAL part of the
interface it's much difficult for me
to think over the most efficient use cases. As you have mentioned
copying the buffers will produce
much of overhead during the processing of the image, I would consider
it was most efficient
to prolong the lifetime of the "processing" buffer by introducing a
new class (GDALBuffer for example)
This class could safely handle an internal unmanaged memory (being
allocated and freed at the unmanaged
side by using CPLMalloc and CPLFree). In addition we could use this
class passing the internal buffer to
WriteRasterInternal / ReadRasterInternal respectively. This class
would also contain the GDALDataType that makes up the
buffer and could reduce the number of overloads mentioned before in this thread.
We could access the elements of the array using an indexer with a GDALDataType
compliant C# type.
How do you like this idea?

>
> > How about supporting ReadBlock/WriteBlock? Would it be required?
>
> I think this is a pretty low priority. Most C++ apps use RasterIO()
> directly, which in turn makes calls to ReadBlock() and WriteBlock()
> (actually IReadBlock() and IWriteBlock()), which are implemented by the
> author of the particular GDAL driver being used. If people really want
> to read and write blocks, then can always get the block width and height
> using GDALGetBlockXSize(), and so forth, and then use RasterIO() with
> windows of that size.
>

OK. But the documentation notes that using RasterIO is less efficient, isn't it?


>
> Thinking about it some more, I think even for the IntPtr overload of
> ReadRaster / WriteRaster, it would make sense to add the bufferOffset
> argument, whenever we use the pixelSpace and lineSpace arguments. This
> avoids having to use that "new IntPtr(buf.ToInt32() + 1)" stuff. All
> three args are only really used when dealing with packed pixel format
> buffers.
>
> For convenience, it would be nice to have overloads that assume default
> values for the last three parameters (bufferOffset, pixelSpace and
> lineSpace) for the common case where we're not dealing with packed
> buffers. In these cases, bufferOffset = 0, pixelSpace =
> sizeof(datatype), and lineSpace = bufXSize * pixelSpace.
>

Would it be a requirement to access the buffer as a 2D array and specify
the offset accordingly?


Best Regards,

Tamas