Opened 11 years ago

Closed 11 years ago

#5030 closed defect (fixed)

Add extra support in Intergraph driver

Reported by: cleo Owned by: warmerdam
Priority: normal Milestone: 1.10.0
Component: GDAL_Raster Version: 1.9.2
Severity: normal Keywords: INGR RLE
Cc:

Description

Added the following functionality.

  • Enable reading bitonal rle files wider than 22784 (0x5900) pixels.
  • Add RESOLUTION metadata/option to read/write DPI.
  • Add write support for .rle (bitonal rle files) to test the above.

All files written with the INGR driver are uncompressed. I needed some output files with 0x5900 scanline header tags to make sure it no longer crashed. So I added write support for compressed .rle files.

The issue with the 0x5900 scanline header tag was that the existing code would just skip over the header. That's a problem if it's a file that has no scanline headers. In such a case, the 0x5900 is a valid span of 22784. I have encountered such a file and is a requirement for us that these load properly. The fix was non-trivial, but the main code did not change much. I added several checks beforehand to ensure that the scanline header really was there. Codepath for files smaller than 22784 pixels wide is almost identical to what was there before.

I've tested with several files with different conditions. I attached a file that has a span of 0x5900 and has no header. This would not display correctly before.

All patches are based off the trunk. I added the final files and patches in the zip file.

Attachments (3)

ingr_support.zip (27.9 KB ) - added by cleo 11 years ago.
test.rle (177.3 KB ) - added by cleo 11 years ago.
ingr_support_v2.zip (28.5 KB ) - added by cleo 11 years ago.

Download all attachments as: .zip

Change History (19)

by cleo, 11 years ago

Attachment: ingr_support.zip added

by cleo, 11 years ago

Attachment: test.rle added

comment:1 by cleo, 11 years ago

test.rle is a file 30000 pixels wide with no scanline headers. Has black at the beginning of exactly 22784 (0x5900) pixels, then all white on the right side.

comment:2 by cleo, 11 years ago

Any thoughts on a generic metadata tag for DPI? There is one in TIFF and likely in PNG that we'll need along with the INGR driver. One issue is that TIFF has both X and Y resolution while INGR has one value for both. Haven't checked PNG yet. It's a little cumbersome to have driver specific code to get DPI.

Also, the RESOLUTION metadata tag is enabled for create options, copy and reads for all Intergraph files.

comment:3 by Even Rouault, 11 years ago

The RLE read/write support doesn't round-trip. It seems to transform 0->1 and 1->0

Tested with :

gdal_translate ../autotest/gcore/data/1bit.bmp 1bit.rle -of ingr gdal_translate 1bit.rle 1bit_2.rle -of ingr

gdalinfo -checksum ../autotest/gcore/data/1bit.bmp returns 200 gdalinfo -checksum 1bit.rle returns 804 gdalinfo -checksum 1bit_2.rle returns 200

In IntergraphDataset::Create(), const char *pszCompression should be NULL initialized, otherwise there's a crash for non-RLE INGR datasets

in reply to:  3 comment:4 by cleo, 11 years ago

The 0->1 inversion is something I noticed during reads in the original version (it also flips the palette, so the image information is technically correct, though the data is inverted). I did not want to change the original behaviour as I didn't want to risk affecting anything else.

I think the proper fix is to not flip the bits nor the palette during reads. I can fix the reads rather easily. It's actually just a matter of deleting one line. I'll have to make sure to pass a different boolean when creating the palette for RLE files. BTW, you may want to check that when you write a 1bit.bmp to a different ingr format (not RLE), that the palette is not inverted if you expect black to be palette entry 0. IOW, take a .rle file and export it to a different ingr format (will be uncompressed). Load the first pixel in both and retrieve their respective palette entries and check that they are the same.

I will fix the inversion and the compression variable issue, but may want to check the palette entries of the existing codebase for ingr.

comment:5 by cleo, 11 years ago

I'm going to leave the palette the same. I'm now confident that the original RLE loading code was incorrect and that it was not supposed to flip the bits upon reads. The RLE tiling code does not flip the bits either. So one of them is wrong.

I will have another patch shortly where the loads act the same as the tiling code (no bit inversions) and the compression variable is set to NULL.

by cleo, 11 years ago

Attachment: ingr_support_v2.zip added

comment:6 by cleo, 11 years ago

Added a new zip file with new files for IntergraphDataset.cpp and IngrTypes.cpp and their patches. The rest is the same. These patches are made assuming the patches in the first zip file were not applied. There are only a few lines that changed.

This will no longer flip bits upon reads and added the NULL assignment. I did a checksum test and the files give the same result both from bmp and from a pre-existing rle.

comment:7 by Even Rouault, 11 years ago

Hum, I don't know anything about INGR, but that seems to mean that the INGR driver didn't read correctly RLE compressed images and would return a raster that was displayed them in inverse video w.r.t. how they are displayed in Intergraph software ?

comment:8 by cleo, 11 years ago

The bits are definitely being inverted. I unfortunately don't have any third party app that can read Intergraph files. But I used a hex editor and looked at the data and the first pixel of the file I linked in this ticket should be 0. Before my recent fix, that pixel would report incorrectly as 1.

Also, we have our own INGR reader. The pixel values that GDAL produced would be inverted from our own reader. After my recent fix (removing the pixel inversion), they are identical, both in pixel values and palette colours.

comment:9 by Even Rouault, 11 years ago

I'm a bit hesitant to apply without a double check with Intergraph software. Of course this change breaks the ingr_8 test of the GDAL autotest suite ( that reads http://svn.osgeo.org/gdal/trunk/autotest/gdrivers/data/frmt10.cot ). Perhaps the previous expected result was wrong. INGR RLE support has seen a bit of activity in http://trac.osgeo.org/gdal/ticket/1959.

comment:10 by cleo, 11 years ago

Why would .cot files fail? None of the changes could have affected 8bit RLE compression. Format 9 is what was affected, not format 10.

In any case, there are two problems with the INGR driver right now without my fix. The only reason the second issue (the inverted bits) was discovered is because I wrote a writable version for .rle files. My first patch left the read code intact, but fixed loading for large files. This should be fixed regardless of what other problems are seen. Could we at least apply that? Only IngrTypes.cpp would be patched for this (using my first patch).

Right now, the most pressing concern is that GDAL cannot load large .rle files with or without scanline headers.

comment:11 by Even Rouault, 11 years ago

r25785 "INGR: Enable reading bitonal rle files wider than 22784 (0x5900) pixels; Add RESOLUTION metadata/option to read/write DPI; Add write support for .rle (bitonal rle files) to test the above (patch ingr_support.zip by cleo, #5030)"

I've integrated the first version of your changes (with the pszCompression set to NULL). My comment about ingr_8 was wrong : the file tested by ingr_8 is http://svn.osgeo.org/gdal/trunk/autotest/gdrivers/data/frmt09.cot (and I've checked that its type is indeed RunLengthEncoded = 9 )

comment:12 by cleo, 11 years ago

Sounds good. Thank you.

About the format 9 inversion, note that my write code does not invert any bits. It is the reads that invert the bits. So your unit test reads the inverted bits and writes those inverted bits to an uncompressed file as is. When you compare them, they are identical since by that point, you're working with post-processed (inverted) data only. My write code does not invert the bits either. However, when reading the file that was written out, it would go through the RLE read code that flips the data (unlike the uncompressed reads which do not flip the data). So the original file was flipped once, while reading the new file makes it appear as flipped twice (once during the original read before being written out and once during the unit test read). So with the original being flipped once and the new file being flipped twice, you end up with a roundtrip that fails.

I will check with a third party app that can load RLE files and see what it gives. I'm fairly certain it will give the same results as my second patch.

comment:13 by cleo, 11 years ago

I tried it with a third party app and the colours are indeed inverted from what GDAL shows. The third party app displays the same as our old INGR loader.

Besides, it is obvious from the code that the pixels are inverted compared to the GDAL tiled loader. So there's a contradiction within the GDAL code between the tiled and non-tiled reads.

And to correct a previous comment, the palette is not flipped in GDAL. It is correct. 0 is supposed to be white (background) and 1 is supposed to be black (foreground) and that's what it is in GDAL. But the image displays inverted because of the flipped pixels.

comment:14 by Even Rouault, 11 years ago

ok, so I conclude that ingr_support_v2.zip is appropriate, and the expected checksum of ingr_8 should be adjusted to the new got value, right ?

comment:15 by cleo, 11 years ago

Yes, that is correct. With the v2 fix, I verified that the images are identical to the third party app. It also matches with our old reader and matches the GDAL code for tiled reads for .rle files.

comment:16 by Even Rouault, 11 years ago

Milestone: 1.10.0
Resolution: fixed
Status: newclosed

r25809 "INGR: fix value inversion when reading type 9 (bitonal RLE) untiled files (#5030)"

Note: See TracTickets for help on using tickets.