[PATCH] Adding support for ROI_PAC image format

[mazhe]

Hi,

I would like to submit to review (hoping for inclusion) a driver intended to read images in the format of the JPL's InSAR processing ROI_PAC project (<​http://aws.roipac.org/cgi-bin/moin.cgi>).

This is yet another binary file with ascii header driver, with only read support for now.

It stores the original metadata in the ROI_PAC domain (something inspired by what the Envi driver does) and support georeferencing. There is only two problems right now:

• One the the sub-formats (raw) use complex bytes, I'll have to overload the read function to convert them to GDT_CInt16
• UTM projections are somewhat flawed in the format which do not specify the N/S property

Plans after that would be to support create/update, but also to implement a driver for the newer JPL ISCE project image format (really similar to ROI_PAC, except headers are xml).

Thanks for your work & time!

[Even Rouault]

Matthieu, a few observations :

• the driver declares GDAL_DCAP_VIRTUALIO = "YES" but there are a few uses of FILE* or VSIFOpen(), or CPLReadLine(). All I/O should be done with VSILFILE* and the VSIF*L API. A good way of checking it works is to put the files into a zip and try "gdalinfo /vsizip/my.zip/my.dataset"
• Identify() implementation is too costly currently. The ideal is that it doesn't trigger any I/O for a file that is not a ROIPAC dataset. It should first check that the extension is in the white list, and then check if there's a a .rsc file. And for the later test, you can use poOpenInfo->GetSiblingFiles(). Caution with some setting it can return a NULL list, in which case you can do a test similars to you.
• fpHeader is not closed at line 184 (error code path)
• would be good for relief of mind if member variables could be initialized in the constructor of ROIPACDataset
• pszProjection not CPLFree()'d in the destructor
• GetGeoTransform() should ideally return CE_Failure if the dummy geotransform [ 0, 1, 0, 0, 0, 1 ] is returned. Usually done with a bValidGeoTransform member variable that is set when putting significant values in the geotransform matrix.
• GetProjectionRef() should not return NULL if pszProjection == NULL but ""
• a few indentation inconsistencies show when looking at http://trac.osgeo.org/gdal/attachment/ticket/5776/roipac_driver.diff
• papszTokens will be leaked in the break case at line 169
• poDriver->SetMetadataItem( GDAL_DMD_EXTENSION, "" ); should be removed
• poDriver->SetMetadataItem( GDAL_DCAP_RASTER, "YES"); should be added
• frmts/raw/makefile.vc not updated
• frmts/frmt_various.html not updated with a #ROIPAC section
• frmts/formats_list.html not updated
• A test script in autotest/gdrivers would be helpful. Possibly with just fake and tiny data.

[mazhe]

Wow, and I thought I reviewed it enough for submission, sorry for the large quantity of errors... I updated the patch resolving most of the issues mentioned, I still have to check poOpenInfo->GetSiblingFiles?() and the test scripts.

[Even Rouault]

No problem. Still seeing some use of VSIFOpen()/VSIFClose(). And the modify logic in Identify() is not correct: it returns TRUE if extension is "int", "slc", etc... without checking the presence of the .rsc file.

Other point Identify() should not return an error message. It should return FALSE silently. And the VSIFOpen() in Identify() isn't paired with a Close. Perhaps better use VSIStatL() instead (in the case where poOpenInfo->GetSiblingFiles() returns a NULL list)

[mazhe]

Does it means that every file must be opened with VSIFOpenL()? I was under the impression that text headers could be used with the non largefile functions

[Even Rouault]

If you don't open every file with VSIFOpenL(), then you cannot advertize GDAL_DCAP_VIRTUALIO = "YES" (and opening in zipped file will not work for example, or with /vsicurl/)

[mazhe]

Ok, now I understand: I rewrited correctly the Identify() function, and migrated every file opening/closing to VSIF*L function. gdalinfo worked with a tar archive.

[mazhe]

One new version, which checks for sibling files both in Open() and Identify().

[Even Rouault]

" int iFile = CSLFindString(papszSiblingFiles, CPLResetExtension( osName, "hdr" ) ); " looks inappropriate. Probably result of copy&paste from other driver.

And in the case where papszSiblingFiles != NULL, Identify() doesn't need to do the VSIStatL() since the file is guaranteed to exist if getRscFilename() returns a non-empty string.

The comment "Confirm that the header is compatible with a JDEM dataset" is also not appropriate

[mazhe]

Yes, copy/paste were a bit tricky, I even renamed *Header* variables to *Rsc* so I could search/replace a bit safer, but yet...

Regarding the VSIStatl() call, would it be acceptable to put it in the getRscFilename() function when papszSiblingFiles == NULL, and make the function return only a filename if the file exists?

[Even Rouault]

Replying to mazhe:

Regarding the VSIStatl() call, would it be acceptable to put it in the getRscFilename() function when papszSiblingFiles == NULL, and make the function return only a filename if the file exists?

Sound reasonable

[mazhe]

Here is the new version, so getRscFilename() will return an empty string if no sibling files and supposed name do not exist.

[Even Rouault]

I'm a bit confused by getRscFilename().

In the if ( papszSiblingFiles == NULL ) case , CPLFormFilename( NULL, poOpenInfo->pszFilename, "rsc" ) will cause ".rsc" to be appended to the image filename, is that intended ? You can test this case by specifying GDAL_DISABLE_READDIR_ON_OPEN=YES as environmenet variable or configuration option. There's also an indentation oddity. And in the other case, you will test first the image filename but with its extension replaced by .rsc, and if not found, .rsc appended to the filename. This is not consistent with first case.

Otherwise, the last missing piece must be the Python autotest script.

[mazhe]

I could not really figure how sibling files works, so I copied the logic used in the case of Envi files: what's loosely defined as the standard in the case of ROI_pAC is that you have a file with a certain extension, and a companion file ending with .extension.rsc .

I guess I'll throw the "replace the extension with .rsc" which would not make sense here, but that would tell that if there's a sibling file, it is the same filename as the input file?

[mazhe]

Patch updated, I still have to figure how to install the python binding in a non-system dir in order to test the script.

[Even Rouault]

Python bindings in a non-system dir:

cd swig/python
python setup.py build
export PYTHONPATH=$PWD/build/lib.XXXXXXXXX
enjoy!

[mazhe]

Implements a read-only driver for the ROI_PAC image format

[mazhe]

dem test file for roi_pac, data

[mazhe]

Thanks, it was more having to do to the fact libtool refused to produce shared library on my system (FreeBSD) and disabling it produced strange linking error (apps not linking with libgdal).

So I have a minimal driver test script which use a file converted from srtm/srtm.ers as a dem file. I only have to understand how you produce a checksum for the GDALTest() function.

[Even Rouault]

So I have a minimal driver test script which use a file converted from srtm/srtm.ers as a dem

file. I only have to understand how you produce a checksum for the GDALTest() function.

"gdalinfo -checksum the.raster"

[mazhe]

Minimal test script

[mazhe]

Thanks, it is done. Appart from checking every subtype possible, or a tgz'ed file, I'm not sure what test I could had until Create() is implemented.

[Even Rouault]

A minimal level of testing as already done by your script is already a good step. Tell me when you think you're done with the code. After integration, you'll be available to check actual test coverage by looking at ​http://rawgit.com/rouault/gdalautotest-coverage-results/master/coverage_html/index.html

[mazhe]

If you agree, I would like for this part to be committed, so further work could be diff'ed easily against the source tree.

[Even Rouault]

The tests don't pass:

('old = ', (-180.0083333, 0.0083333333000000006, 0.0, -59.991666700000003, 0.0, -0.0083333333000000006))
('new = ', (-180.0083333, 0.00083333332999999999, 0.0, -59.991666700000003, 0.0, -0.00083333332999999999))

See the difference. My gt[1] and gt[5] values are the tenth of your values.

[mazhe]

dem test file for roi_pac, header

[mazhe]

Oops, my bad, there was indeed an error in the header file, I thought I uploaded the right one...

[Even Rouault]

trunk r28173 "Add read-only raster driver for image formats of JPL's ROI_PAC project (contributed by Matthieu Volat, #5776)"

[Even Rouault]

Closing. Further enhancements can go to more focused tickets.

[Even Rouault]

Milestone renamed