gdaladdo -r mode completely broken?

[sprice]

If I take a generic tiff file (or a GeoTIFF) and run "gdaladdo -r mode ..." on it, the generated overviews are invalid. For example (input/output attached):

$ gdaladdo -r mode N036E108_x16_GLC.tiff 2 4
0...10...20...30...40...50...60...70...80...90...100 - done.
$ tiffsplit N036E108_x16_GLC.tiff
_TIFFVSetField: xaaa.tif: Bad value 65578 for "SampleFormat" tag.
_TIFFVSetField: xaab.tif: Bad value 65537 for "SampleFormat" tag.
_TIFFVSetField: xaac.tif: Bad value 65537 for "SampleFormat" tag.

[sprice]

input

[sprice]

output

[sprice]

Things I should also mention:

• By "overviews are invalid" I mean they are a singe solid color with some optional static in them.
• This these output files are verified by ImageMagick, so it most likely isn't a bug in tiffsplit.
• From what I can tell, this bug has been around for a while. I have a file from November, generated on a different computer, which shows the same symptoms.

[warmerdam]

It is my understanding that "mode" was never implemented for overview building (despite an early intention to do so). So I think the action item for this ticket is to improve parameter validation, error reporting and the documentation.

Mateusz, could you look into this as time permits?

[sprice]

mode implementation

[sprice]

Sounds like you need a patch like this. I'm using basically the same algorithm as the large patch that I attached here: http://trac.osgeo.org/gdal/ticket/2327

[sprice]

Also note that the patch is incomplete in two places. See comments.

[sprice]

Quick bugfix

[sprice]

How is my patch? Does it help?

[sprice]

Has there been any work done with this patch/bug since the last post?

[sprice]

Ping?

[Even Rouault]

Sorry, since Mateusz' unavailability, we lack a bit resources to properly review submitted patches.

I looked a bit at mode1.2.diff. Here are my comments :

• First remark (ok, I'm going to sounds like a fool, but who cares lol), I have no idea of what the 'mode' algorithm was supposed to do in the mind of the first person who thought about implementing it. Is it a standard name in signal processing/statistics for what I've understood by reading your patch that is to say count the number of time each pixel value is found in the source window and select the value that was found the most frequently ? I guess I might not be the only one to wonder about the meaning of that option, so that would maybe worth adding a few lines in gdal_utilities.dox in the gdaladdo section
• I understand that your patch only works for GDT_Byte input data. There should be some validation mechanism in to ensure that we meet these conditions otherwise, as such, we risk buffer overflow, crashes, etc if it is fed with other data types.
• What's the purpose of the "+ .4" at line 28 of the patch ? If it's for rounding to the nearest integer, I'd have thought about + .5.
• The patch doesn't apply on recent trunk. Would be great if you could update it to apply above trunk and test it as there has been some work in that area in the last few months (gauss overview implementation)
• The indentation is not really consistent. Advised indentation is to be consistent with surrounding code, which generally means spaces only (no tabs), 4 spaces for each level

[sprice]

To respond to your first point, the mode implementation is supposed to choose the value which appears most often of all the sampled points. This really only makes sense in terms of classification raster data, and could also be called a majority filter.

Thus my algorithm works on data where sizeof() = 1 byte, but that's also the most common type of raster classification dataset.

I'll try to address your other points soon. I think I was hoping that someone familiar with GDAL internals could do the validation the Right Way (tm).

[sprice]

same patch but w/o white space changes

[sprice]

A new patch is attached. I fixed some of the new gauss's white space, so the patch is a bit bigger than it would otherwise be. Note that the patch ending in "w.diff" doesn't contain the whitespace changes, for clarity.

Also note that I've installed the latest stable TIFF library, but I'm still getting these errors:

_TIFFVSetField: xaaa.tif: Bad value 65544 for "SampleFormat".
_TIFFVSetField: xaab.tif: Bad value 65537 for "SampleFormat".
_TIFFVSetField: xaac.tif: Bad value 65537 for "SampleFormat".

The current patch makes no changes to the GDALDownsampleChunkC32R() method. I'm not exactly sure when it's invoked, and I don't have a file to test the code anyway. I can apply the code to that method also, but you'll have to send me a testing file.

I just noticed that there is no proper classification TIFF attached to this, so I'm attaching the one I've been testing with.

[sprice]

class TIFF

[sprice]

PS: If the patch looks good, I'll take a quick look at optimizing the code. Creating overviews seems to already be pretty fast, so I don't expect to do anything major this time. ;)

[Even Rouault]

Ok, I commited in r15606 (test added in r15607) a modified version of your mode2.w.diff patch. The main difference is that I dont use the dynamic stack array allocation ( 'int foo[bar]' where bar is a variable) because it probably lacks of portability. I made another change so that we go the fastest case when the data type is GDT_Byte and there's no color table. Please check that it still works for you.