Proposal: GDALRasterBlock::Get[XY]Size() should return actual size

[danmitchell@…]

The documentation of GDALRasterBand::GetBlockSize() states the following:

"GDAL contains a concept of the natural block size of rasters so that applications can organized data access efficiently for some file formats. The natural block size is the block size that is most efficient for accessing the format. [...] Note that the X and Y block sizes don't have to divide the image size evenly, meaning that right and bottom edge blocks may be incomplete. See ReadBlock() for an example of code dealing with these issues."

The problem is that the actual X size (as opposed to the natural X size) is needed to compute offsets into the block's data array. The ReadBlock() example deals with the issue by testing whether or not a block is on the right or bottom edge so that the proper block width is used in calculating array offsets. The code works, but it is -- with all due respect -- a little clumsy. Worse, these tests (or variations on them) appear in a number of files throughout the library, increasing the chances for a bug.

The GDALRasterBlock class has methods GetXSize() and GetYSize(), and one reasonably expects these methods to return the actual X and Y size of the block, but they don't. Instead they return the natural block size of the raster, even though that information is already available via GetBand().

I propose to (A) add methods to GDALRasterBlock that will return the actual sizes, or (B) modify GDALRasterBlock so that GetXSize() and GetYSize() return the actual sizes. With either of these changes it will no longer be necessary to test whether or not a block is on the right or bottom edge to determine its actual size, instead one simply calls the appropriate method.

In case (A) changes are only needed in GDALRasterBlock; in case (B) I expect it will be necessary to make about 50 changes (total) in about 10 files. I think (B) is probably the Right Thing, but (A) is more practical. I guess it will also be necessary in case (A) to add a small amount of overhead to GDALRasterBlock in the form of a pair of ints to hold the actual X and Y sizes.

D.

[Jukka Rahkonen]

I have honestly no idea if this suggestion is good or not but I would like to see some comment from someone who understands the topic.

[Even Rouault]

The (A) approach would be the less disruptive, but I'm not even sure that the new methods should be in GDALRasterBlock. I'd rather see it at the GDALRasterBand level since those truncated block sizes are sometimes useful without needing a GDALRasterBlock instance.

void GDALRasterBand::GetActualBlockSize(int nBlockXOff, int nBlockYOff, int* pnActualBlockXSize, int* pnActualBlockYSize)

[jamesramm]

Tackling this would be interesting for me as it would take away this bit of common boilerplate:

void GDALRasterBand::GetActualBlockSize(int nBlockXOff, int nBlockYOff, int* pnActualBlockXSize, int* pnActualBlockYSize)
{
  if( (nBlockXOff+1) * nXBlockSize > nRasterXSize)
     *pnActualBlockXSize = nRasterXSize - iXBlock * nXBlockSize;
  else
     *pnActualBlockXSize = nXBlockSize;
  if( (nBlockYOff+1) * nYBlockSize > nRasterYSize )
     *pnActualBlockYSize = nRasterYSize - iYBlock * nYBlockSize;
  else
     *pnActualBlockYSize = nYBlockSize;
}

I think GetActualBlockSize should return CPLErr in case of requesting an offset that is out of bounds.

What would be very interesting for us would be to also override the above in the geotiff driver in order to handle sparse blocks. Would it be reasonable to return NULL in the case that the block is sparse? I'm suggesting this in the hope (ive not delved into it yet), that there is an efficient way to check whether a block is sparse. Clearly this function should return as instantaneously as possible...

[JamesRamm]

my first attempt at adding something useful for this

[Even Rouault]

Regarding the patch, sounds reasonable with the following observations :

• missing doxygen description of the input parameter
• naming conventions: nXValid/nYValid should be *pnXValid / *pnYValid
• did you actually compile the code :-) ? GDALGetActualBlockSize() is lacking a hBand parameter
• GDALGetActualBlockSize is declared to return void, but the implementation returs CPLErr

[jamesramm]

Corrects errors and exposes new function to swig

[jamesramm]

addressed those problems and also added corresponding function in Band.i

[Even Rouault]

Sorry, I wasn't clear on the naming convention. If it is a int, it should be prefixed by n. If it is a pointer to a int, it should be prefixed by pn. See ​https://trac.osgeo.org/gdal/wiki/rfc8_devguide

You can drop the changes in bridge/: this is abandon-ware likely to be removed soon

And a @since GDAL 2.2 in the doxygen comments would be useful.

[JamesRamm]

Added the @since and found 2 instances where the pointer was incorrectly named (nXSize instead of pnXSize) and have corrected them.

[Even Rouault]

In 35453:

Add GDALRasterBand::GetActualBlockSize() (based on patch by James Ramm + python bindings and tests by myself. Fixes #1233)