Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#5176 closed defect (fixed)

HFA driver: re-writing existing histogram in HFA file, after raster editing

Reported by: nflood Owned by: warmerdam
Priority: normal Milestone: 1.11.0
Component: GDAL_Raster Version: 1.10.0
Severity: normal Keywords: hfa histogram
Cc:

Description (last modified by nflood)

When using Imagine to directly edit the pixel values, Imagine fails to recalculate the histogram correctly. GDAL can be used to write the histogram to an HFA file, but only if it is not already present. This patch adds a case to hfaopen.cpp to allow the histogram counts to be re-written after they have changed.

This patch is against the current 1.10.0 release, but was originally trialed against 1.9.0, and has been in use since July 2012 without further problems.

Also added a patch against the current trunk, which is a few lines different than the 1.10.0 file. The trunk patch has not been operationally tested, but the file appears to be the same apart from a slight line number shift, and I would not anticipate any problem.

Attachments (4)

hfaopen.cpp.overwritehisto.patch (2.0 KB) - added by nflood 4 years ago.
hfaopen.cpp.overwritehisto.trunk.patch (2.0 KB) - added by nflood 4 years ago.
patch against current trunk (2013-07-31, revision 26024)
hfaopen.cpp.rewritehistogram.trunk-2130801.patch (3.0 KB) - added by nflood 4 years ago.
autotest.hfa.py.rewritehistogram.trunk-20130801.patch (3.2 KB) - added by nflood 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by nflood

Changed 4 years ago by nflood

patch against current trunk (2013-07-31, revision 26024)

comment:1 Changed 4 years ago by nflood

Description: modified (diff)

comment:2 Changed 4 years ago by Even Rouault

I have looked at the patch and I find something strange. In your patch, you write integer values, whereas in the existing code just above, which look very similar, the values written are doubles

comment:3 in reply to:  2 Changed 4 years ago by nflood

Replying to rouault:

I have looked at the patch and I find something strange. In your patch, you write integer values, whereas in the existing code just above, which look very similar, the values written are doubles

Thanks so much for looking at this so quickly, very kind. I may well have messed up, but I am not sure what you mean. Which values are doubles in the code above? The new code is only re-writing the counts, which are integers, and that seems true also in the existing code just above. The intent was to use the same code, but leaving out all the stuff for creating the histogram in the file, since it is already there. (I confess that the HFA format scares me, so I have been very cautious. :-)

comment:4 Changed 4 years ago by Even Rouault

I was refering to http://trac.osgeo.org/gdal/browser/trunk/gdal/frmts/hfa/hfaopen.cpp#L2840 . But I understand your confusion. Up to a few weeks ago, it was writing integer values. But this has changed in http://trac.osgeo.org/gdal/changeset/26024/trunk/gdal/frmts/hfa/hfaopen.cpp ( ticket #5066 ). I'm myself not a specialst at all of the HFA format, but my feeling is that the patch should check the "dataType" field to check if it is "integer" or "real" and act accordingly.

comment:5 in reply to:  4 Changed 4 years ago by nflood

Replying to rouault: A thousand apologies, I obviously didn't check the trunk version of that closely enough. Well spotted. I think your suggestion is correct, but leave it with me, and I will try to work out some decent tests and confirm that nothing bad happens. Thank you for your diligence. I will get back to you when I have a better patch against the current trunk.

comment:6 Changed 4 years ago by Even Rouault

Suggestion: it would be good if you could enhance the HFA regression tests ( autotest/gcore/hfa_write.py ) to test the new code paths.

comment:7 Changed 4 years ago by nflood

OK, I have tested the approach discussed above, and it seems fine. I have attached a new patch against the trunk, for the file hfaopen.cpp. The resulting HFA files have been successfully read in with Imagine 8.6 and 11.0, with no apparent problems.

I checked the regression tests in autotest/gdrivers/hfa.py. There seems to be a reasonable test there of the writing of a histogram, i.e. a test of the earlier patch for ticket http://trac.osgeo.org/gdal/ticket/5066, but it was failing (before I started tinkering) because it had an unguarded os.remove() of a temporary file which was not necessarily present. I have placed a guard on this in the hfa_histwrite() function, and also in the hfa_createcopy_statistics() function, which had the same problem. The hfa_histwrite() test now passes fine.

I have then added a new test routine, hfa_histrewrite(), which tests the re-writing of a histogram, as made possible with the patch for the current ticket. I have attached a patch to autotest/gdrivers/hfa.py for all these changes to that file.

comment:8 Changed 4 years ago by Even Rouault

Component: defaultGDAL_Raster
Keywords: hfa histogram added
Milestone: 2.0
Resolution: fixed
Status: newclosed

Perfect. Committed in trunk in r26260

comment:9 Changed 4 years ago by Even Rouault

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