Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#5653 closed defect (fixed)

Stride calculation error on GDALRegenerateOverviewsMultiBand

Reported by: Lucian Plesea Owned by: warmerdam
Priority: high Milestone: 2.0.0
Component: GDAL_Raster Version: svn-trunk
Severity: major Keywords:
Cc: antonio

Description (last modified by Lucian Plesea)

GDALRegenerateOverviewsMultiBand is used when paged and compressed TIF overviews are built. The source relative stride calculation is sometimes wrong, leading to inefficient code execution and partial tiles being generated multiple tiles, particularly for large rasters, where the errors accumulate. Sometimes there are visible artifacts in the output, but not always.

This is due to the following two lines in gcore/overview.cpp, in GDALRegenerateOverviewsMultiBand(), lines 1993 and 1994 currently:

int nFullResXChunk = (nDstBlockXSize * nSrcWidth) / nDstWidth;

int nFullResYChunk = (nDstBlockYSize * nSrcHeight) / nDstHeight;

Most of the time the result is correct, nSrcWidth/nDstWidth is commonly 2/1. When nSrcWidth is odd, nDstWidth is rounded up, thus the ratio is a bit under 2. Since all variables are integers, the end result is rounded down, the result ends up being off by one. For example (512*1023)/512 results in 1023 instead of the correct 1024. The brackets makes this error less likely. It escaped detection because it is relatively infrequent and the effect is noticeable only when the sizes are large, the stepping error having time to accumulate.

The proposed fix is to use floating point and round up to an integer, which also takes care of the potential integer overflow in the original formula. This calculation is not in an inner loop, so efficiency is not an issue.

int nFullResXChunk = int( double(nDstBlockXSize) * nSrcWidth / nDstWidth + 0.5);

int nFullResYChunk = int( double(nDstBlockYSize) * nSrcHeight / nDstHeight + 0.5;

Change History (5)

comment:1 Changed 7 years ago by Lucian Plesea

Description: modified (diff)

comment:2 Changed 7 years ago by antonio

Cc: antonio added

comment:3 Changed 7 years ago by Even Rouault

Milestone: 2.0
Resolution: fixed
Status: newclosed

I've managed to reproduce the issue. See test in r27788. "GDALRegenerateOverviewsMultiBand(): fix stride calculation error with certain raster dimensions (#5653)"

My fix is different. I reworked the way we iterate over blocks in GDALRegenerateOverviewsMultiBand() and factored some code that was common in resampling methods. This is also in preparation for further fixes, in particular in cubic resampling.

comment:4 Changed 7 years ago by Lucian Plesea

Indeed, that is a better, long term fix. What I proposed is still useful as a bug fix for existing versions of GDAL.

comment:5 Changed 6 years ago by Even Rouault

Milestone: 2.02.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.