[gdal-dev] Suggestion for API addition

[David Strip]

On 11/11/2012 3:53 AM, Even Rouault wrote:
> Le dimanche 11 novembre 2012 03:45:57, David Strip a écrit :
>>   The current GDALRasterBand::RasterIO signature makes it easy to read or
>> write a contiguous subregion of the band. However, if you want to read or
>> write the entire band into/from  a contiguous sub-region of the buffer,
>> it's not nearly as straightforward. You can do this by using the
>> nLineSpace parameter, giving the size of the subregion for buffer size
>> parameters,  and offsetting the address of the buffer. It works, but that
>> makes the code a bit baroque and hard to read.
>>
>>   How about a signature that indicates the entire band will be read, but
>> moves the meaning of the offset to the buffer. Something like
>>
>> CPLErr GDALRasterBand::RasterIO   ( GDALRWFlag eRWFlag,
>>                                     int   nXSize,
>>                                     int   nYSize,
>>                                     void * pData,
>>                                     int   nBufXOff,
>>                                     int   nBufYOff,
>>                                     int   nBufXSize,
>>                                     int   nBufYSize,
>>                                     GDALDataType    eBufType,
>>                                     int   nPixelSpace,
>>                                     int   nLineSpace   )
>>
>>
>>     Obviously I can write a wrapper for this functionality that just calls
>> RasterIO, setting the appropriate values for buffer size, etc, but I was
>> thinking I might not be the only one who wants to crop a buffer while
>> writing to the band, or reading a band into a sub-region of a buffer.
> This is just a personal opinion, but I'm not convinced this is a good idea. I
> think people will be more confused than currently. The meaning of the
> parameters of the existing RasterIO() requires some time to be confortable
> with, but once you've finished learning curve, it is pretty logical. Adding
> another method with the same name, almost same parameters, but subtle
> differences in behaviour is not going to make users life easier.
You're right that the actual signature and function name I provided 
would not be the right way to go. The substance of my question was 
whether there would be value to a function which crops a buffer while 
writing to the band. Rather than overloading RasterIO, the name could be 
writeCroppedBand or something.
>
> Why not adding an example code snippet of your use case in the documentation
> of the current API ? (if I've understood well, your proposal is just a
> convenient way of computing the right value for pData, likely pData =
> pBufferBase + nBufYOff * nLineSpace + nBufXOff * nPixelSpace)
The value of a new function is code clarity. It's not just computing 
pData, it's that the nBufXSize and nBufYSize become the size of the 
cropped region, not the size of the actual buffer, and offsets are not 
visible in the existing signature (there is no nBufXOff, nBufYOff), 
they're buried in the computation of pData, which of course no longer 
points to the actual buffer. When the existing function is used for this 
purpose it becomes very difficult to see what is actually happening. Of 
course one can say that's what comments are for.
>
> Furthermore, if you want to operate on the whole band, you should also remove
> the nXSize and nYSize parameters that are useless.
Yes, those should have been removed
>
>>
>>   This cropping interpretation doesn't allow for decimation/replication. It
>> requires the buffer size be large enough to copy the entire band into the
>> sub-region of the buffer. Allowing for decimation/replication would
>> require yet more parameters.
> Not sure to understand that. The presence of nBufXSize, nBufYsize in your
> proposal allows subsampling/oversampling. Perhaps you meant something else
> with decimation/replication ?
You're right.  nPixelSpace and nLineSpace now become required, no 
default values will work.