[PATCH] Support for writing RasterAttributeTables by default for HFA/Imagine

[Robert Coup]

ERDAS Imagine (HFA) .img files can have RasterAttributeTable data, which is read via GDAL and the GDALRasterAttributeTable class. But HFADataset::CreateCopy() doesn't write out RAT data by default (via SetDefaultRAT()), so gdal_translate -of HFA some.img another.img doesn't preserve RAT data.

[Robert Coup]

Initial patch

[Robert Coup]

Patch against trunk attached.

Now the only differences in gdalinfo output after an .img > .img conversion using gdal_translate are:

# no overviews (expected) # the type of the 'Histogram' field is 1 (float) instead of 0 (integer) # precision changes on float fields (eg. 0.65 -> 0.6509803921568628)

None of which are real problems afaict.

I'm getting some more intensive testing done by Imagine users on a wide range of datasets, and I'll add unit tests before committing this.

[Robert Coup]

second patch, makes gdalwarp copy RATs as well when creating the output file.

[Robert Coup]

Support for gdal_translate when not using CreateCopy() (ie. via VRT), and preserving RATs in gtiff->gtiff conversions

[Even Rouault]

A few comments about 4903.3.diff :

• for gdalwarp, resampling methods other than nearest will alter original pixel values, possibly creating new ones. It is doubtful than copying the source RAT makes sense in that case
• for the VRT driver, serialization/deserialization into/from the VRT XML should also be implemented

[thjc]

attached patch v4 that drops RAT's when using non nearest/mode transformations in gdalwarp if the layer is thematic.

General discussion around serialisation was that this can be put off until there is a request for the feature. In particular large RAT's may not work well in the VRT XML (see rfc 40)

[Even Rouault]

4903.4.diff looks better. But I still have additional thoughts :

1) I think that the explicit copying of RAT is not necessary in the GTiff driver. It looks like it is already done by the PAM mechanism (gcore/gdalpamrasterband.cpp lines 613-624) thanks to the poDS->CloneInfo( poSrcDS, GCIF_PAM_DEFAULT & ~GCIF_MASK ); call at line 8521. I've just tried "gdal_translate ../autotest/gcore/data/byte.img out.tif" with current trunk and found that the PAM out.tif.aux.xml contains the histogram as a RAT (so RATs are already serialized in PAM, which could cause problem for large RATs...). Not sure if this applies to the HFA driver too.

2) In gdal_translate.cpp and gdalwarp.cpp, there might be cases where RAT copying is inappropriate. For example for an histogram RAT, if you don't copy the whole extent of the source raster, then the source histogram isn't relevant. In gdal_translate.cpp, in CopyBandInfo(), there's a flag that is passed to know if statistics can be copied. It could be used also in conjunction of examining the RAT nature to determine if copying it is appropriate. In gdalwarp, there's no such mechanism, but generally all operations done in gdalwarp will likely invalidate an existing histogram.

3) What would be great is to add new test cases for regression purpose in autotest/utilities/test_gdal_translate.py and autotest/utilities/test_gdalwarp.py to ensure that the RAT is copied (or not) during those operations.

[thjc]

1) I will test this and confirm, Rob recalls that hfa->tiff->hfa didn't work without this code, maybe there is something subtle there, or maybe trunk has changed since his testing.

2) This starts to get quite complex, but here are some thoughts...

We have a few different cases...the first major pivot is whether the RAT is related to a thematic or an athematic layer. RAT data associated with a thematic layer is much more britle and is more likely to be useless after transformations. In order to simplify this I think we could promote thematic/athematic to a first class property and build it into the gdal_rat structure.

athematic (or continuous) RAT layers are pretty robust. Most transformations (including color interpolation) should still work out meaningful to some extent. Colour space scaling potentially messes with the data, but even then if its a relative scale it can still make sense...so I think we should preserve the RAT data for athematic layers.

RATs for thematic layers are fine with cropping etc, you may end up with some redundant data but the rest should be fine. colour interpolation with nearest (or median) should also be fine, so some gdalwarp operations still make sense. Any scaling or non nearest colour interpolation will of invalidate an athematic RAT.

Then we come to histograms, they are invalidated by any operation except 1 to 1 translation to another format. This is true whether it is thematic or athematic. However, we should be able to recalculate the histogram data anyway.

So to summarise:

A) track thematic/athematic in the RAT class

B) discard non histogram RAT for thematic bands where colour level changes occur (scaling, or non nearest interpolation)

C) discard histogram data for anything except 1:1 translations, and recalculate for formats that have built in histogram concept (i.e. HFA). (The alternative here is to recalculate for all output formats if we had histogram to start with...)

D) never discard non histogram RAT data for athematic layers (it might be useful, we can't really judge)

3) I will work on some test cases.

[thjc]

okay, new patchset (number 5)

1) Your right, seems to work fine with this addition removed, so its removed now.

2) Added RAT Table Type and rules around discarding data in gdalwarp/gdal_translate

3) Added some test cases for gdal_translate. Existing test cases for gdalwarp seem to cover the basics of whether statistics are kept or not already...

[Even Rouault]

I think that poNewRAT is leaked in gdal_translate.cpp

Would be cool to add @since GDAL 2.0 in the doxygen of the new methods/functions.

For the sake of readability of the changeset, I'd prefer the regenerated Python bindings not to be part of it (or stick with SWIG 1.3.40 for now)

I think it would be appropriate to advertize on the mailing list about the proposed addition of the RAT Table Type to have more eyes on that. Are we sure that thematic is the appropriate default value ? The doc could likely be extended to explain a bit more about those concepts.

[thjc]

patchset v6

[thjc]

Fixed leak, cleaned SWIG files out and added GDAL 2.0 comments.

[cdestigter]

New patch fixes a bug in SetDefaultRAT() (it wrote the RAT to disk but didn't actually set band->poDefaultRAT )

[Even Rouault]

Note that RFC 40 patches have landed in trunk now. Your patch probably needs "some" adaptation.

[cdestigter]

New patch updated to apply to latest trunk

[cdestigter]

Removes a std::cout

[cdestigter]

clean for latest trunk

[Jukka Rahkonen]

Thanks for the contribution. I added PATCH to the summary for making it easier to find the patch for local compilation before someone finds time for making a review and integration.

[Even Rouault]

Robert is a committer so this patch is hopefully not stuck

[stephend]

Update for latest trunk, KEA driver support

[Even Rouault]

Implemented per ​https://github.com/OSGeo/gdal/pull/743 for GDAL 2.4