Opened 11 years ago

Closed 14 months ago

Last modified 14 months ago

#1233 closed enhancement (fixed)

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

Reported by: danmitchell@… Owned by: warmerdam
Priority: normal Milestone: 2.2.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords:
Cc:

Description (last modified by warmerdam)

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.

Attachments (3)

getactualblocksize.patch (5.3 KB) - added by JamesRamm 15 months ago.
actual_blocksize_2.patch (7.1 KB) - added by jamesramm 15 months ago.
Corrects errors and exposes new function to swig
get_actual_blocksize_3.patch (5.6 KB) - added by JamesRamm 14 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by warmerdam

Component: defaultGDAL_Raster
Description: modified (diff)
Priority: highestnormal
Severity: minornormal

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by 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)

comment:4 Changed 18 months ago by 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...

Changed 15 months ago by JamesRamm

Attachment: getactualblocksize.patch added

comment:5 Changed 15 months ago by JamesRamm

my first attempt at adding something useful for this

comment:6 Changed 15 months ago by 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

Changed 15 months ago by jamesramm

Attachment: actual_blocksize_2.patch added

Corrects errors and exposes new function to swig

comment:7 Changed 15 months ago by jamesramm

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

comment:8 Changed 15 months ago by 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.

Changed 14 months ago by JamesRamm

comment:9 Changed 14 months ago by JamesRamm

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

comment:10 Changed 14 months ago by Even Rouault

Resolution: fixed
Status: newclosed

In 35453:

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

comment:11 Changed 14 months ago by Even Rouault

Milestone: 2.2.0
Note: See TracTickets for help on using tickets.