Opened 12 years ago

Closed 11 years ago

#2347 closed defect (fixed)

gdaladdo -r mode completely broken?

Reported by: sprice Owned by:
Priority: normal Milestone: 1.6.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: overviews mode
Cc: warmerdam, Even Rouault

Description

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.

Attachments (7)

N036E108_x16_GLC.tiff.zip (436.5 KB) - added by sprice 12 years ago.
input
xaab.tif.zip (2.6 KB) - added by sprice 12 years ago.
output
mode1.diff (1.5 KB) - added by sprice 12 years ago.
mode implementation
mode1.2.diff (1.5 KB) - added by sprice 12 years ago.
Quick bugfix
mode2.diff (13.5 KB) - added by sprice 11 years ago.
mode2.w.diff (6.3 KB) - added by sprice 11 years ago.
same patch but w/o white space changes
N084W180_x32_GLC.class.tif.zip (152.0 KB) - added by sprice 11 years ago.
class TIFF

Download all attachments as: .zip

Change History (20)

Changed 12 years ago by sprice

Attachment: N036E108_x16_GLC.tiff.zip added

input

Changed 12 years ago by sprice

Attachment: xaab.tif.zip added

output

comment:1 Changed 12 years ago by 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.

comment:2 Changed 12 years ago by warmerdam

Cc: warmerdam added
Component: defaultGDAL_Raster
Keywords: overviews added
Owner: changed from warmerdam to Mateusz Łoskot

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?

Changed 12 years ago by sprice

Attachment: mode1.diff added

mode implementation

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

comment:4 Changed 12 years ago by sprice

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

Changed 12 years ago by sprice

Attachment: mode1.2.diff added

Quick bugfix

comment:5 Changed 12 years ago by sprice

How is my patch? Does it help?

comment:6 Changed 11 years ago by sprice

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

comment:7 Changed 11 years ago by Mateusz Łoskot

Owner: Mateusz Łoskot deleted

comment:8 Changed 11 years ago by sprice

Ping?

comment:9 Changed 11 years ago by Even Rouault

Cc: Even Rouault added

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

comment:10 Changed 11 years ago by 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).

Changed 11 years ago by sprice

Attachment: mode2.diff added

Changed 11 years ago by sprice

Attachment: mode2.w.diff added

same patch but w/o white space changes

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

Changed 11 years ago by sprice

class TIFF

comment:12 Changed 11 years ago by 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. ;)

comment:13 Changed 11 years ago by Even Rouault

Keywords: mode added
Milestone: 1.6.0
Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.