Opened 7 years ago

Closed 7 years ago

Last modified 7 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 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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 by mazhe, 7 years ago

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

comment:8 by Even Rouault, 7 years ago

" 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 by mazhe, 7 years ago

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?

in reply to:  9 comment:10 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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

comment:12 by antonio, 7 years ago

Cc: antonio added

comment:13 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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 by mazhe, 7 years ago

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 by Even Rouault, 7 years ago

Python bindings in a non-system dir:

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

by mazhe, 7 years ago

Attachment: roipac_driver.diff added

Implements a read-only driver for the ROI_PAC image format

by mazhe, 7 years ago

Attachment: srtm.dem added

dem test file for roi_pac, data

comment:17 by mazhe, 7 years ago

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.

in reply to:  17 comment:18 by Even Rouault, 7 years ago

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"

by mazhe, 7 years ago

Attachment: roipac.py added

Minimal test script

comment:19 by mazhe, 7 years ago

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 by Even Rouault, 7 years ago

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 by mazhe, 7 years ago

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 by Even Rouault, 7 years ago

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.

by mazhe, 7 years ago

Attachment: srtm.dem.rsc added

dem test file for roi_pac, header

comment:23 by mazhe, 7 years ago

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

comment:24 by Even Rouault, 7 years ago

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

comment:25 by Even Rouault, 7 years ago

Milestone: 2.0
Resolution: fixed
Status: newclosed

Closing. Further enhancements can go to more focused tickets.

comment:26 by Even Rouault, 7 years ago

Milestone: 2.02.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.