Opened 10 years ago

Last modified 10 years ago

#5442 closed enhancement

OGR Support for Selafin format — at Version 8

Reported by: fhissel Owned by: fhissel
Priority: normal Milestone:
Component: OGR_SF Version: unspecified
Severity: normal Keywords: selafin driver ogr
Cc:

Description (last modified by fhissel)

Selafin is a format used by some finite element software (mostly Telemac hydraulic model) to store numerical values at different points or elements and for different time steps.

This driver allows read-write support for Selafin 2D format in OGR.

Change History (14)

by fhissel, 10 years ago

Attachment: f2d_bowl_fe.slf added

Test case 1

by fhissel, 10 years ago

Attachment: r2d_tide-jmj_real_gen.slf added

Test case 2

comment:1 by Even Rouault, 10 years ago

A few remarks from my code review :

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string
  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.
  • port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping
  • the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.
  • the CreateLayer() and DeleteLayer() should be completely removed, or disabled with #ifdef notdef if you intend implementing them one day.
  • Same for CreateDataSource(), DeleteDataSource().
  • there are very long lines with if else clause on the same line, which make them difficult to read
  • the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Questions :

  • did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?
  • did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant
  • did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Wishes :

  • could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

in reply to:  1 ; comment:2 by fhissel, 10 years ago

Owner: changed from warmerdam to fhissel
Status: newassigned

Replying to rouault:

Thanks for your code review! I'm going to fix the driver.

A few remarks from my code review :

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

  • port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping

OK, I will use those macros.

  • the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.

OK, I will change that

OK

  • there are very long lines with if else clause on the same line, which make them difficult to read

OK

  • the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Yes, I will attach new smaller files.

Questions :

  • did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?

Yes, all tests passed without error (at least the first 100 or so, until I stopped the program).

  • did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant

It was OK with gcc under Linux, but I did not test is with MSVC. I will try it.

  • did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Yes, valgrind --leak-check=full did not detect any leak.

Wishes :

  • could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

OK, I am going to add one.

in reply to:  2 ; comment:3 by Even Rouault, 10 years ago

Replying to fhissel:

Replying to rouault:

  • OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

Indeed. I missed that line. So that part is OK.

  • The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

Yes, I know. I'm generally suspicious about long use - I've seen many times wrong uses like fread(&a_long, 1, 4, f) -, but in your code, it looks OK.

by fhissel, 10 years ago

Attachment: test2.slf added

Test case

in reply to:  3 comment:4 by fhissel, 10 years ago

Description: modified (diff)

Replying to rouault:

Replying to fhissel:

Replying to rouault:

I've just uploaded a new version of the driver. This one supports writing to Selafin format. A smaller test case and an autotest script are attached.

The driver has been tested with valgrind and test_ogrsf (there is one error in test_ogrsf but it is caused by a normal renumbering of FIDs when inserting or deleting a feature in some kind of layers).

I have not tried to compile it with MSVC (no Windows available).

comment:5 by Even Rouault, 10 years ago

Running "valgrind --leak-check=full test_ogrsf test2.slf" shows

INFO: SetNextByIndex() read test passed.
==9343== Invalid read of size 8
==9343==    at 0x5D8DBCB: OGRSelafinLayer::DeleteFeature(long) (ogrselafinlayer.cpp:649)
==9343==    by 0x4079EE: TestOGRLayerDeleteAndCreateFeature(OGRLayer*) (test_ogrsf.cpp:1905)
==9343==    by 0x408E4D: TestOGRLayer(OGRDataSource*, OGRLayer*, int) (test_ogrsf.cpp:2472)
==9343==    by 0x403920: ThreadFunctionInternal(ThreadContext*) (test_ogrsf.cpp:290)
==9343==    by 0x40352C: ThreadFunction(void*) (test_ogrsf.cpp:179)
==9343==    by 0x4033F9: main (test_ogrsf.cpp:136)
==9343==  Address 0x17a18308 is 0 bytes after a block of size 20,760 alloc'd
==9343==    at 0x4C274A8: malloc (vg_replace_malloc.c:236)
==9343==    by 0x5A0E495: VSIMalloc (cpl_vsisimple.cpp:505)
==9343==    by 0x59B992F: CPLMalloc (cpl_conv.cpp:136)
==9343==    by 0x5AF3064: Selafin::read_intarray(_VSILFILE*, long*&, bool) (io_selafin.cpp:348)
==9343==    by 0x5AF3BC4: Selafin::read_header(_VSILFILE*, char const*) (io_selafin.cpp:521)
==9343==    by 0x5D87E5F: OGRSelafinDataSource::OpenTable(char const*) (ogrselafindatasource.cpp:424)
==9343==    by 0x5D87752: OGRSelafinDataSource::Open(char const*, int) (ogrselafindatasource.cpp:284)
==9343==    by 0x5D894F9: OGRSelafinDriver::Open(char const*, int) (ogrselafindriver.cpp:54)
==9343==    by 0x5D8ED2B: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:226)
==9343==    by 0x403598: ThreadFunctionInternal(ThreadContext*) (test_ogrsf.cpp:198)
==9343==    by 0x40352C: ThreadFunction(void*) (test_ogrsf.cpp:179)
==9343==    by 0x4033F9: main (test_ogrsf.cpp:136)

And at compilation, the following warnings are generated :

io_selafin.cpp: In function ‘int Selafin::read_integer(VSILFILE*, long int&, bool)’:
io_selafin.cpp:272: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::write_integer(VSILFILE*, long int)’:
io_selafin.cpp:289: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘long int Selafin::read_string(VSILFILE*, char*&, bool)’:
io_selafin.cpp:299: warning: format not a string literal and no format arguments
io_selafin.cpp:304: warning: format not a string literal and no format arguments
io_selafin.cpp:311: warning: format not a string literal and no format arguments
io_selafin.cpp:316: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::write_string(VSILFILE*, char*, size_t)’:
io_selafin.cpp:327: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘long int Selafin::read_intarray(VSILFILE*, long int*&, bool)’:
io_selafin.cpp:338: warning: format not a string literal and no format arguments
io_selafin.cpp:343: warning: format not a string literal and no format arguments
io_selafin.cpp:350: warning: format not a string literal and no format arguments
io_selafin.cpp:354: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::write_intarray(VSILFILE*, long int*, size_t)’:
io_selafin.cpp:365: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::read_float(VSILFILE*, double&, bool)’:
io_selafin.cpp:376: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::write_float(VSILFILE*, double)’:
io_selafin.cpp:390: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘long int Selafin::read_floatarray(VSILFILE*, double**, bool)’:
io_selafin.cpp:400: warning: format not a string literal and no format arguments
io_selafin.cpp:405: warning: format not a string literal and no format arguments
io_selafin.cpp:412: warning: format not a string literal and no format arguments
io_selafin.cpp:416: warning: format not a string literal and no format arguments
io_selafin.cpp: In function ‘int Selafin::write_floatarray(VSILFILE*, double*, size_t)’:
io_selafin.cpp:427: warning: format not a string literal and no format arguments

--> The fix is to use CPLError(foo, bar, "%s", the_message)

The code does not compile on Windows for several reasons :

  • an easy one : you need to make the driver to be compiled. With the additionnal patch :
Index: ogr/ogrsf_frmts/makefile.vc
===================================================================
--- ogr/ogrsf_frmts/makefile.vc	(révision 27262)
+++ ogr/ogrsf_frmts/makefile.vc	(copie de travail)
@@ -4,7 +4,7 @@
 			avc rec mem vrt csv gmt bna kml gpx \
 			geoconcept xplane georss gtm dxf pgdump gpsbabel \
 			sua openair pds htf aeronavfaa edigeo svg idrisi arcgen \
-			segukooa segy pdf sxf openfilegdb wasp \
+			segukooa segy pdf sxf openfilegdb wasp selafin \
 			$(ARCOBJECTS_DIR) \
 			$(OGDIDIR) $(FMEDIR) $(OCIDIR) $(PG_DIR) $(DWGDIR) \
 			$(ODBCDIR) $(SQLITE_DIR) $(MYSQL_DIR) $(ILI_DIR) \
@@ -233,7 +233,7 @@
 				 sua\*.obj openair\*.obj pds\*.obj htf\*.obj \
 				 aeronavfaa\*.obj edigeo\*.obj svg\*.obj idrisi\*.obj \
 				 arcgen\*.obj segukooa\*.obj segy\*.obj pdf\*.obj sxf\*.obj \
-				 openfilegdb\*.obj wasp\*.obj \
+				 openfilegdb\*.obj wasp\*.obj selafin\*.obj \
 				$(OGDIOBJ) $(ODBCOBJ) $(SQLITE_OBJ) \
 				$(FMEOBJ) $(OCIOBJ) $(PG_OBJ) $(MYSQL_OBJ) \
 				$(ILI_OBJ) $(DWG_OBJ) $(SDE_OBJ) $(FGDB_OBJ) $(ARCDRIVER_OBJ) $(IDB_OBJ) \

  • more fundamental. There are various compilation errors. The above link I gave about Vagrant should make you able to compile with MSVC 2008 Express, through Wine, in the Vagrant Linux VM.
	cl   /nologo /MD /EHsc /Ox /D_CRT_SECURE_NO_DEPRECATE /D_CRT_NONSTDC_NO_DEPRECATE /DNDEBUG /W4 /wd4127 /wd4251 /wd4275 /wd4786 /wd4100 /wd4245 /wd4206 /wd4018 /wd4389 /DHAVE_SSE_AT_COMPILE_TIME -I..\..\..\port -I..\..\..\ogr -I..\..\..\gcore  -I..\..\..\alg -I..\..\..\ogr\ogrsf_frmts -I.. -I..\.. -DOGR_ENABLED  /c ogrselafindriver.cpp ogrselafindatasource.cpp ogrselafinlayer.cpp 
fixme:file:NeedCurrentDirectoryForExePathW (L"cl"): partial stub
fixme:heap:HeapSetInformation (nil) 1 (nil) 0
ogrselafindriver.cpp
ogrselafindriver.cpp(81) : error C3861: 'strptime': identifier not found
ogrselafindatasource.cpp
ogrselafinlayer.cpp
ogrselafinlayer.cpp(157) : error C2057: expected constant expression
ogrselafinlayer.cpp(157) : error C2466: cannot allocate an array of constant size 0
ogrselafinlayer.cpp(157) : error C2133: 'anData' : unknown size
ogrselafinlayer.cpp(325) : error C2057: expected constant expression
ogrselafinlayer.cpp(325) : error C2466: cannot allocate an array of constant size 0
ogrselafinlayer.cpp(325) : error C2133: 'anMap' : unknown size
ogrselafinlayer.cpp(329) : error C2668: 'sqrt' : ambiguous call to overloaded function
        z:\media\disk\vs90\VC\INCLUDE\math.h(581): could be 'long double sqrt(long double)'
        z:\media\disk\vs90\VC\INCLUDE\math.h(533): or       'float sqrt(float)'
        z:\media\disk\vs90\VC\INCLUDE\math.h(128): or       'double sqrt(double)'
        while trying to match the argument list '(long)'
Generating Code...
z:\home\even\gdal\svn\gdal-msvc\gdal\ogr\ogrsf_frmts\selafin\ogrselafindatasource.cpp(576) : warning C4701: potentially uninitialized local variable 'dfValues' used

in reply to:  5 comment:6 by fhissel, 10 years ago

Replying to rouault:

I managed to install Vagrant/Ubuntu Precise/Wine. Thanks for the tip! The new version I have just uploaded compiles under MSVC, and clang/gcc on Linux and valgrind does not report any error.

I did not get any warning with this CPLError thing... Anyway I changed them also.

There is still an error in test_ogrsf, in the second DeleteFeature. I suspect it is because the test program tries to check there is no feature with the same FID after it has deleted it. However, since FIDs are not stored in the Selafin file, the other features automatically have their FIDs translated when the first one is deleted. So when it is checked, there is again a feature with FID 0.

comment:7 by Even Rouault, 10 years ago

Ok, but now the ogr_selafin.py script fails :

  TEST: ogr_selafin_create_ds ... ERROR 1: It seems no file system object called 'tmp.slf' exists.
Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
success
  TEST: ogr_selafin_create_nodes ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
ERROR 3: Error when reading Selafin file

fail
    line 80: unable to create node feature
  TEST: ogr_selafin_create_elements ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
fail
    line 144: wrong value of attribute in element layer
  TEST: ogr_selafin_set_field ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
ERROR 5: List should have 1 elements
success

The error occurs at line 376 ( if (Selafin::read_integer(poHeader->fp,nLen,true)==0
) of ogrselafinlayer.cpp

Could you remove the warning about "Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.". This is a general requirement for any GDAL driver, so no need to spam the user.

When looking at CreateFeature()/SetFeature(), I also see that you assume that a feature has a geometry. This is not necessarily the case.

I'm also wondering if you needed to implement DeleteField(), AlterFieldDefn(), and even SetFeature() or DeleteFeature() etc.. They are advanced features only used by popular and general purpose formats that are meant to be work formats (shapefile, postgis, spatialite, etc...). Specialized formats like Selafin don't really need that I guess, and it could be just a read-only / write-only format, and not bother with modifying.

by fhissel, 10 years ago

Attachment: selafin_driver.diff added

Patch for the new driver

by fhissel, 10 years ago

Attachment: ogr_selafin.2.py added

Autotest script

by fhissel, 10 years ago

Attachment: ogr_selafin.py added

Autotest script

in reply to:  7 comment:8 by fhissel, 10 years ago

Description: modified (diff)

Replying to rouault:

Ok, but now the ogr_selafin.py script fails :

  TEST: ogr_selafin_create_ds ... ERROR 1: It seems no file system object called 'tmp.slf' exists.
Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
success
  TEST: ogr_selafin_create_nodes ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
ERROR 3: Error when reading Selafin file

fail
    line 80: unable to create node feature
  TEST: ogr_selafin_create_elements ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
fail
    line 144: wrong value of attribute in element layer
  TEST: ogr_selafin_set_field ... Warning 1: Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.
ERROR 5: List should have 1 elements
success

Yes, that was a regression of the patch for Windows. It is fixed now. I also checked the Python script with Valgrind and there is no memory leak (at least in the driver...)

The error occurs at line 376 ( if (Selafin::read_integer(poHeader->fp,nLen,true)==0
) of ogrselafinlayer.cpp

Could you remove the warning about "Although Selafin data-sources and layers are writable, it is up to the user to avoid writing on two layers at the same time because this could result in data corruption.". This is a general requirement for any GDAL driver, so no need to spam the user.

OK, done.

When looking at CreateFeature()/SetFeature(), I also see that you assume that a feature has a geometry. This is not necessarily the case.

Sorry, I was not aware of this. I added a test at the beginning of those two functions.

I'm also wondering if you needed to implement DeleteField(), AlterFieldDefn(), and even SetFeature() or DeleteFeature() etc.. They are advanced features only used by popular and general purpose formats that are meant to be work formats (shapefile, postgis, spatialite, etc...). Specialized formats like Selafin don't really need that I guess, and it could be just a read-only / write-only format, and not bother with modifying.

Yes, my purpose was mainly to allow the creation of Selafin files based on other GIS formats. But this may not require all the functions... I will post another patch with DeleteField, AlterFieldDefn and DeleteFeature marked as non implemented. SetFeature is actually one of the easiest to implement and can be useful for some purposes (changing some values in QGis...) so I will keep it for now.

Note: See TracTickets for help on using tickets.