Opened 17 years ago

Closed 17 years ago

#1732 closed defect (fixed)

Possible error with Imagine Palettized images (rounding vs truncating)

Reported by: mrosen Owned by: warmerdam
Priority: normal Milestone: 1.4.3
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: hfa
Cc:

Description (last modified by warmerdam)

I was chasing down an issue with GDAL's support for palettized Imagine files and think I found a defect in HFARasterBand. In the construtor we convert from a 0..1 double to a 0..255 short. But we do so by truncation:

            sEntry.c1 = (short) (padfRed[iColor]   * 255);
            sEntry.c2 = (short) (padfGreen[iColor] * 255); 
            sEntry.c3 = (short) (padfBlue[iColor]  * 255);
            sEntry.c4 = (short) (padfAlpha[iColor]  * 255);
            poCT->SetColorEntry( iColor, &sEntry );

Currently, this code suffers from two difficulties: (a) it gives different answers on different platforms (b) rounding rather than truncation would seem to be the natural way to map from [0..1] to [0..255].

The first problem is probably unavoidable since floating point arithmetic is involved. Solving the second problem can be solved by something like "(short)((padfRed[iColor]*255+0.5))".

I tried this and found that the input table did seem more correct (true grey palette in repro below). However, then a subsquent call (from the application layer) to the band's GetMetadataItem("STATISTICS_MEAN") returns an empty string (where it did not prior to the change ... spurious?)

REPRO

I'm attaching a small repro image "grey.img." Despite the name, it contains an RGB lookup table. Under linux/gcc3.2 the lookup table is not quite grey; under win32/vc8 it creates slightly different not-quite grey lookup table. I've confirmed the differences are due to FP rounding. Looking at the values read from the table and the name of the file, the obvious intent is that the table be an RGB palette that implements gray scale. Unfortunately, I used application level printf() calls to dump the entries in the table.

A short demo of the FP rounding issue based on the second entry in the CLUT:

	double df = 0.0039215686274509803; // taken from CLUT, entry #2.
	short s;
	s = (short)((df  * 255)+0.5);
	printf ("df = %lf, s = %d\n", df, s);  // linux/gcc32, s==0; win32/vc8 s==1
	return 0;

I verified that ArcMap 9.2 will read this image and indicates a true greyscale palette!

Attachments (1)

grey.img (16.3 KB ) - added by mrosen 17 years ago.
repro image for 1732

Download all attachments as: .zip

Change History (3)

by mrosen, 17 years ago

Attachment: grey.img added

repro image for 1732

comment:1 by warmerdam, 17 years ago

Component: defaultGDAL_Raster
Description: modified (diff)
Keywords: hfa added
Milestone: 1.5.0
Status: newassigned

comment:2 by warmerdam, 17 years ago

Milestone: 1.5.01.4.3
Resolution: fixed
Status: assignedclosed

Michael,

I have chosen to use the following conversion which gives a similar result for "normal" values, but also gives reasonably continuous values for various other circumstances.

Index: hfadataset.cpp
===================================================================
--- hfadataset.cpp      (revision 11816)
+++ hfadataset.cpp      (working copy)
@@ -499,10 +499,14 @@
         {
             GDALColorEntry   sEntry;
 
-            sEntry.c1 = (short) (padfRed[iColor]   * 255);
-            sEntry.c2 = (short) (padfGreen[iColor] * 255);
-            sEntry.c3 = (short) (padfBlue[iColor]  * 255);
-            sEntry.c4 = (short) (padfAlpha[iColor]  * 255);
+            // The following mapping assigns "equal sized" section of 
+            // the [0...1] range to each possible output value and avoid
+            // rounding issues for the "normal" values generated using n/255.
+            // See bug #1732 for some discussion.
+            sEntry.c1 = MIN(255,(short) (padfRed[iColor]   * 256));
+            sEntry.c2 = MIN(255,(short) (padfGreen[iColor] * 256));
+            sEntry.c3 = MIN(255,(short) (padfBlue[iColor]  * 256));
+            sEntry.c4 = MIN(255,(short) (padfAlpha[iColor] * 256));
             poCT->SetColorEntry( iColor, &sEntry );
         }
     }

Let me know if you see any problems with this. I am going to apply this patch in trunk, 1.4 branch and 1.4-esri branch.

Note: See TracTickets for help on using tickets.