Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#5776 closed enhancement (fixed)

[PATCH] Adding support for ROI_PAC image format

Reported by: mazhe Owned by: warmerdam
Priority: normal Milestone: 2.0.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: ROI_PAC driver
Cc: antonio

Description

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!

Attachments (4)

roipac_driver.diff (24.8 KB) - added by mazhe 7 years ago.
Implements a read-only driver for the ROI_PAC image format
srtm.dem (240 bytes) - added by mazhe 7 years ago.
dem test file for roi_pac, data
roipac.py (2.9 KB) - added by mazhe 7 years ago.
Minimal test script
srtm.dem.rsc (650 bytes) - added by mazhe 7 years ago.
dem test file for roi_pac, header

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by 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.

comment:2 Changed 7 years ago by 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.

comment:3 Changed 7 years ago by 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)

comment:4 Changed 7 years ago by 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

comment:5 Changed 7 years ago by 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/)

comment:6 Changed 7 years ago by 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.

comment:7 Changed 7 years ago by mazhe

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

comment:8 Changed 7 years ago by 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

comment:9 Changed 7 years ago by 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?

comment:10 in reply to:  9 Changed 7 years ago by 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

comment:11 Changed 7 years ago by mazhe

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

comment:12 Changed 7 years ago by antonio

Cc: antonio added

comment:13 Changed 7 years ago by 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.

comment:14 Changed 7 years ago by 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?

comment:15 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by Even Rouault

Python bindings in a non-system dir:

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

Changed 7 years ago by mazhe

Attachment: roipac_driver.diff added

Implements a read-only driver for the ROI_PAC image format

Changed 7 years ago by mazhe

Attachment: srtm.dem added

dem test file for roi_pac, data

comment:17 Changed 7 years ago by 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.

comment:18 in reply to:  17 Changed 7 years ago by 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"

Changed 7 years ago by mazhe

Attachment: roipac.py added

Minimal test script

comment:19 Changed 7 years ago by 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.

comment:20 Changed 7 years ago by 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

comment:21 Changed 7 years ago by 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.

comment:22 Changed 7 years ago by 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.

Changed 7 years ago by mazhe

Attachment: srtm.dem.rsc added

dem test file for roi_pac, header

comment:23 Changed 7 years ago by mazhe

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

comment:24 Changed 7 years ago by Even Rouault

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

comment:25 Changed 7 years ago by Even Rouault

Milestone: 2.0
Resolution: fixed
Status: newclosed

Closing. Further enhancements can go to more focused tickets.

comment:26 Changed 6 years ago by Even Rouault

Milestone: 2.02.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.