Opened 14 years ago

Closed 9 years ago

#3638 closed enhancement (fixed)

[PATCH] OGR Driver for Norwegian SOSI (.sos) files.

Reported by: relet Owned by: relet
Priority: normal Milestone:
Component: default Version: svn-trunk
Severity: normal Keywords:
Cc: warmerdam

Description

http://www.statkart.no/nor/SOSI/SOSI_in_English/

Just to leave a note that we are working on it. I'll attach some code when there is a stable version.

Attachments (16)

sositest.png (52.3 KB ) - added by relet 14 years ago.
Norwegian administrative boundaries visualized by mapserver, read from SOSI file using OGR connection.
sosi-topo2.png (117.4 KB ) - added by relet 14 years ago.
Another snapshot of Norway rendered entirely from SOSI files using mapserver OGR connection.
GNUmakefile (243 bytes ) - added by relet 14 years ago.
makefile.vc (210 bytes ) - added by relet 14 years ago.
trunk.diff (2.3 KB ) - added by relet 14 years ago.
20BygnAnlegg.SOS (201.7 KB ) - added by relet 14 years ago.
example sosi file - contains geometries
20Tekst5000biterISO8859-10.sos (1.2 KB ) - added by relet 14 years ago.
example sosi file - contains text elements
ogr_sosi.h (4.6 KB ) - added by relet 14 years ago.
ogrsosidatasource.cpp (18.4 KB ) - added by relet 14 years ago.
ogrsosilayer.cpp (10.2 KB ) - added by relet 14 years ago.
valgrind.log (130.8 KB ) - added by relet 14 years ago.
ogrsosidriver.cpp (3.8 KB ) - added by relet 14 years ago.
fyba_melding.cpp (2.2 KB ) - added by relet 14 years ago.
sosi.patch.20487 (31.8 KB ) - added by relet 13 years ago.
Patch based on r20487- some bugfixes (proper UTF encoding), cleanup, and a code stub for datafile creation.
sosi.patch.22134 (7.5 KB ) - added by relet 13 years ago.
patch based on r22134 - support for BUEP (arc) element using interpolation to linestrings
sosi_plugin.diff (2.0 KB ) - added by sbl 10 years ago.
Changed build-routines for SOSI-support as plugin

Download all attachments as: .zip

Change History (47)

by relet, 14 years ago

Attachment: sositest.png added

Norwegian administrative boundaries visualized by mapserver, read from SOSI file using OGR connection.

by relet, 14 years ago

Attachment: sosi-topo2.png added

Another snapshot of Norway rendered entirely from SOSI files using mapserver OGR connection.

by relet, 14 years ago

Attachment: GNUmakefile added

by relet, 14 years ago

Attachment: makefile.vc added

by relet, 14 years ago

Attachment: trunk.diff added

by relet, 14 years ago

Attachment: 20BygnAnlegg.SOS added

example sosi file - contains geometries

by relet, 14 years ago

example sosi file - contains text elements

comment:1 by relet, 14 years ago

Resolution: fixed
Status: newclosed

Here we go. I have attached the driver code as single attachments so that they remain readable.

You will want to store most files in ./ogr/ogrsf_frmts/sosi/ with the following exceptions

  • trunk.diff contains some modifications to trunk, to enable the SOSI driver in general and in the GNUmakefiles.
  • the .sos files are example files which display some geometries and text elements in Finnmark. (The geometries are mostly disconnected, this is correct)
  • the .png files are older examples of driver progress, you can ignore these.

Known issues:

  • You will need the FYBA library (and the helper libraries GM and UT) available as Windows VC++ binary LIB and DLL from http://www.statkart.no/nor/SOSI/Program/ (last section). We hope to release that for other architectures or as source code in the future.
  • SOSI files are often enough encoded in ISO8859-10, as are the examples. I added the #define for that, so that the CPL transcoding generates a sensible error message when we try to read the file. The current fallback to ISO8859-1 works, but will mess with your Saami.
  • To actually include SOSI support on windows you will have to edit the makefile.vc files in ogrsf_frmts to include:
    • "sosi" in DIRLIST
    • "sosi\*.obj" and the three FYBA .lib files in the "lib" list
  • and ogrsf_frmts/generic to include:
    • "-DSOSI_ENABLED" in BASEFORMATS (or make it optional)

comment:2 by Even Rouault, 14 years ago

Resolution: fixed
Status: closedreopened
Summary: OGR Driver for Norwegian SOSI (.sos) files.[PATCH] OGR Driver for Norwegian SOSI (.sos) files.

Reopening. If closed, I'm afraid it will be forgotten forever !

I've looked at the new files and the first impression I got is that the indentation should be fixed ! Please conform with http://trac.osgeo.org/gdal/wiki/rfc8_devguide, especially for :

# Use 4 character indentation levels.
# Use spaces instead of hard tab characters in source code.

I've noticed an extensive use of printf() in the code. This should be avoided. Use CPLError() for errors that must be reported to the user (by default on standard error stream, or possibly caught by a user callback), or CPLDebug() for debug info that appears only if selectively turned on with environment variable CPL_DEBUG=ON.

I also think there are a few memory leaks. If you test on Linux, I'd advise you using valgrind. For example :

  • in OGRSOSILayer::GetNextFeature(), you construct a poGeom object and assign it to the feature with SetGeometry(). I think you should rather use SetGeometryDirectly() so that the feature becomes the owner of the geometry and frees it when necessary (I can be wrong on that point. This is just by quick code inspection).
  • In ogrosidatasource.cpp, the return value of CPLRecode() should be freed with CPLFree() after having been passed to the OGRFieldDefn constructor.
  • poFeatureDefn probably also leak. A poFeatureDefn->Release(); should be probably added in the destructor of the layer
  • There might be other leaks...

At line 195 of ogrsosilayer.cpp, you use CPLMalloc(), but you deallocate with free(). For consistency, you should rather use CPLFree() (you can catch those kinds of errors by uncommenting #define DEBUG_VSIMALLOC in port/cpl_vsisimple.cpp. This will crash horribly when you don't pair things correctly ;-).

Questions :

  • In OGRSOSIDataSource::Open(), I see that LC_Init() is called. Is it safe to call it several times in a process lifetime ?
  • Is there a LC_Terminate() or something like that ? If LC_Init() initializes memory, that would be cool to have a terminate function called in the driver destructor to free what needs to be freed.
  • What is the runtime cost of LC_Init() and HO_TestSOSI() ? If they're fast, it's fine. If it takes some time, something should be done to have a fast detection of SOSI file, or in fact a fast detection of *non* SOSI files. The aim is that the opening of non-SOSI files that can be opened by another OGR driver isn't delayed too much.

I've not been able to test the driver, as my current environment is Linux.

Personnal note that doesn't engage the GDAL project: I'm wondering if it wouldn't be more convenient to drop the content of your FYBA lib in the driver itself, mainly to avoid one more GDAL external dependency and make users life easier, unless the FYBA source code is huge, isn't cross-platform or cannot be released under the terms of the X/MIT licence... or you don't want that for any reason (if it's used by third-party projects, ...). Anyway, that isn't a requirement and it's too soon to call for that because I haven't seen what it looks like ;-)

(Note: Starting with next week, I will be mostly away for the 3 following weeks)

comment:3 by relet, 14 years ago

Thanks a lot for the very quick review! I'm going to address the issues today and update the files above. And I'll address your questions when I've verified some my answers. :)

Just a quick stupid question - I have used valgrind/alleyoop against ogr2ogr and ogrinfo through the development process at the most verbose level I could find. I debugged the memory management until, at this point, the only message it repeats, is:

==10466== HEAP SUMMARY:
==10466==     in use at exit: 0 bytes in 0 blocks
==10466==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==10466== 
==10466== All heap blocks were freed -- no leaks are possible

That made me confident, but some of these mistakes you mention should have been reported, shouldn't they? Have I been missing something?

in reply to:  3 comment:4 by relet, 14 years ago

Never mind. I must have dropped the option to trace child processes somewhere. The lack of leaks should've made me more suspicious. :}

[x] Indentation fixed
[x] spaces used
[x] printfs replaced by CPLError and CPLDebug as appropriate

[x] Geometry set directly
[x] All output of CPLRecode CPLFree'd again
[x] poFeatureDefn released
[x] some other leaks fixed. ;)

I'm still not entirely confident that I caught them all, and would appreciate any help with that. I'll attach a log - there are still some leaks reported from processes which I couldn't identify as my driver or the fyba lib - these two would have debug symbols. I could also provide you with a copy of libfyba for linux i386 if that helps. I'm just not able to release it or the source code for now.

[x] DEBUG_VSIMALLOC enabled and not seen any horrible crashes.
[x] LC_Init moved to the constructor of OGRSOSIDriver, since it is indeed not safe to call it multiple times in the currently released version. 
[x] LC_Close added to the destructor.

The file checks are reasonably fast - I'll try to profile them to have some numbers. ogrinfo on an unknown file still takes 0.143s to terminate.

The option to release FYBA with a MIT licence exists, and if that happens, I will definitely remove the dependency here. (It is using another Hungarian naming convention though, which is certain to confuse people. I speak from experience.)

Going to replace the files now.

by relet, 14 years ago

Attachment: ogr_sosi.h added

by relet, 14 years ago

Attachment: ogrsosidatasource.cpp added

by relet, 14 years ago

Attachment: ogrsosilayer.cpp added

by relet, 14 years ago

Attachment: valgrind.log added

by relet, 14 years ago

Attachment: ogrsosidriver.cpp added

comment:5 by Even Rouault, 14 years ago

The version of fyba_melding.cpp you've attached is incorrect. It looks like the result of a svn merge that conflicted. Not sure which implementation is wanted: the empty one or the one with Norwegian error messages and the Windows Beep() ;-) You could probably map those error callbacks to CPLError() or CPLDebug() I imagine.

Anyway I've tried on a Windows VM to compile and run it. I managed to get it compile and link, although that's painful currently : a better approach would be to add commented variables in nmake.opt and use the variable names in the makefiles to get the include directories, get optionnal compilation. You can take example on the other drivers that depend on third-party DLL.

So I managed to link, but ogrinfo fails to start with an "the application couldn't be initalized correctly (0xc01500002).". I downloaded the Visual C++ 2005 Redistribuable DLL as I saw that fyba_dll.dll has a dependency to MSVCR80.DLL but that didn't help. And curiously that fact that fyba_dll.dll is in the path or not doesn't change the error message.

by relet, 14 years ago

Attachment: fyba_melding.cpp added

in reply to:  5 comment:6 by relet, 14 years ago

Can you try running dependency walker to see what is missing? http://www.dependencywalker.com/

in reply to:  5 comment:7 by relet, 14 years ago

saw that fyba_dll.dll has a dependency to MSVCR80.DLL but that didn't help.

I am still trying to reproduce the problem - did you link against the .dll or against the .lib?

comment:8 by Even Rouault, 14 years ago

I've finally integrated the driver in r20487, but I'm still unable to execute ogrinfo. Dependecy Walker mentions "The Side-by-Side configuration information for FYBA_DLL.dll contains errors". No way to solve this on my side. I presume there's an issue with the way you compiled the fyba stuff. You should try to deploy it on a machine with no Visual Studio installed. (I compile GDAL with Visual Studio 2008 Express for the record)

in reply to:  8 comment:9 by tamas, 13 years ago

In response to a question I've already posted some comments to the -dev list with regards to this driver, see http://permalink.gmane.org/gmane.comp.gis.gdal.devel/25135

The Side-by-Side configuration information error reported above may be due to an incorrect compilation of the dependent library, probably the manifest file (describing the CRT dependency) is not embedded as a resource in the dll. In order to integrate the driver in the daily builds of http://vbkto.dyndns.org/sdk/ the proper compilation for the supported compilers and architectures would also be required. It could probably be done easily if the corresponding sources was available somewhere do download.

by relet, 13 years ago

Attachment: sosi.patch.20487 added

Patch based on r20487- some bugfixes (proper UTF encoding), cleanup, and a code stub for datafile creation.

comment:10 by relet, 13 years ago

You are right, and we are working on it. These mills grind slowly, but we should be able to release the source code eventually and include it in this driver.

comment:11 by Even Rouault, 13 years ago

sosi.patch.20487 commited in r21065

by relet, 13 years ago

Attachment: sosi.patch.22134 added

patch based on r22134 - support for BUEP (arc) element using interpolation to linestrings

comment:12 by Even Rouault, 13 years ago

Reviewing sosi.patch.2213, I think that naming sqr() a method is a bit risky. There's a risk of symbol conflict with such a short and generic name. Perhaps making it a static function, or a static method of the class, or a macro... would avoit it.

I also want to mention that there is a OGRGeometryFactory::approximateArcAngles() method, but I do not know if it is directly usable for your needs.

Any news on the open-sourcing of the fyba lib ?

comment:13 by Even Rouault, 12 years ago

I just saw your post on IRC. Is the open-sourcing of the fyba lib made such as it is intended to be just included in the directory where lies the OGR SOSI driver (and thus with the licence GDAL/OGR use), or is it a third-party lib that should be compiled separately and then refered by ./configure / nmake.opt ? If the former, we'll need to review it of course (you can attach it in this ticket). If the latter, not necessarily ;-)

comment:14 by relet, 12 years ago

Our release will be a standalone library, as it is used by other software packages. But I made sure that the licence is MIT/X style as is ogr's, and would like to include it in this driver directly. I wanted to see if you have any technical comments or objections after seeing the code. ;)

If that's ok with you I would like to send you a package directly, since the initial release is coordinated with PR and might take a moment longer. :P

comment:15 by Even Rouault, 12 years ago

ok, you can send the package to me directly (provided you don't ask me to sign a NDA ;-). I can't promise a review in depth, mostly because I don't know anything about the SOSI format itself. What would be intersting to know is how you intend to manage the coexistence of the standalone library and its import into GDAL tree. The integration solution you describe could be similar to what is done with the MITAB or AVC drivers, that have both a upstream project, with its own revision control and bug tracker. When a bug related to one of this driver is filed in GDAL Trac, the procedure we follow is then to make sure that it is first fixed in the upstream project and then merged back in GDAL source tree to avoid forking from the upstream project.

Anyway, even if you cannot share your code with everybody, it might be good that you send an email to gdal-dev to share your plans and gets a feedback from the community. I woud like that the GDAL PSC knows on how you intend to manage the coexistence of the standalone library and its import into GDAL tree, and can express its opinion.

comment:16 by relet, 12 years ago

Thanks, good point. We had in mind to maintain the library in our public svn. I currently expect more contributions coming from ongoing internal development than external contributors but that might just as well change. The integration upstream sounds good. I'm a bit reluctant to set up an own bug tracker, just because it's a lot of trouble to maintain and ward from spam for a single small project. If it was for me, we would just use github or google code. I am going to discuss that.

I will send an email to gdal-dev then.

comment:17 by Even Rouault, 12 years ago

I've seen your message on IRC but we're never connected at the same time apparently. You'll find my email address in the COMMITERS file at the root of GDAL source tree.

comment:18 by Even Rouault, 12 years ago

Thomas (assuming it is your first name from the bickbucket invitation),

I've looked for 10 minutes at the FYBA code, but honestly don't expect me to spend more... The size of the code, the comments, functions and variable names in Norwegian, plus my lack ok knowledge of the formats are show stoppers for me to review as a after-day work (not sure I'd accept it as a day job either ;-)).

Well, from this quick visit, I have just a few comments :

  • The X/MIT licence should be included in all source files
  • the indentation is somewhat inconsistant. I guess there is a mix of spaces and tabs. Tabs should be banned as a way of indenting code
  • I see there are global variables. Doesn't sound good if there are multiple OGR SOSI datasources open at the same time...
  • I have the feeling the code is mainly designed for Windows (stdafx files, etc). How does it fly with other environments such as Linux ? I see the use of the "long" type: no issue with 64 bit Linux ?

I'm now convinced that including this code in GDAL would require discussion and agreement of the PSC. It is obvious that no-one apart you or your team would be able to maintain it, so I think that it would require you to be a GDAL commiter to do the integration and maintenance work. Perhaps in a first step, it could be done in a sandbox.

comment:19 by Even Rouault, 12 years ago

Cc: warmerdam added

comment:20 by relet, 12 years ago

Thanks for the assessment. I guess it's good that you now have a feel for what the library looks like. ;)

Quick comments:

  • including the license and beautifying the code is no problem of course
  • The library was usually written for a wide spectrum of systems (hence the "UT" portability lib), then maintained for many years on Windows only. I got it to run fine on Linux 32/64bit, but lack the understanding of porting issues to call it a proper port.
  • I have been running fyba/sosi as the backend of a mapserver instance during testing, but not done further testing with multiple instances. I put that on my todo list.
  • We are running the code on 64bit Linux (usually for conversion jobs from SOSI->PostGIS). No issues with that so far, even for massive datasets, but I wouldn't totally exclude it.

comment:21 by relet, 12 years ago

To those following the thread.

The source code to fyba is available at https://bitbucket.org/relet/fyba Some binary builds of fyba and ogr/sosi are available in the "releases" branch, but I'll be happy to include more or link to other builds if you manage to compile on platforms I do not have access to.

comment:22 by relet, 11 years ago

Update because of increased interest:

The latest versions can be found at our github: http://github.com/kartverket/gdal http://github.com/kartverket/fyba

Would love to get some help with correct the automake setup in gdal, so that I can issue a pull request to the official branch now and then.

comment:23 by sbl, 10 years ago

Recently, build process for the OpenFYBA-library has been improved and GDAL (version 1.9.2 and later) was build and tested successfully with SOSI support both on Windows 7 and Linux (Ubuntu 12.04). Translation of the documentation of the OpenFYBA lib is in progress too. For OpenFYBA also .deb packages are planned. For build hints see: http://trac.osgeo.org/gdal/wiki/SOSI

It would be great to make the SOSI driver more easily accessible for users especially on Windows (see: http://trac.osgeo.org/osgeo4w/ticket/359). We now do have MS Visual Studio (2012) project files for compilation on Windows. How should we proceed if we were aiming at packaging the OpenFYBA lib as a plugin in OSGeo4W?

comment:24 by Even Rouault, 10 years ago

I'm not sure if OSGeo4W uses VS 2012. Might be 2010, but you'd have to check with OSGeo4W devs.

On the GDAL side, you will need to improve ogr/ogrsf_frmts/sosi/makefile.vc to add a plugin target. There are some similar examples, e.g. ogr/ogrsf_frmts/pg/makefile.vc

comment:25 by sbl, 10 years ago

Thanks! I adjusted the ogr/ogrsf_frmts/sosi/makefile.vc according to the example together with some modifications of ogr/ogrsf_frmts/makefile.vc and nmake.opt.

After GDAL/OGR was compiled with SOSI-plugin I could build a plugin-.dll by doing: nmake /f makefile.vc plugin in ogr/ogrsf_frmts/sosi

My question is now, how do I activate the plugin? ogrinfo --format gives me no indication for SOSI-support

comment:26 by sbl, 10 years ago

I see I forgot to do: nmake /f makefile.vc plugin-install But that did not help either...

comment:27 by Even Rouault, 10 years ago

If you deploy it in OSGeo4W you need to deploy the plugin DLL as well as the SOSI/FYBA ones in the appropraite directory of OSGeo4W (search where other GDAL plugins are set). If it is a custom install, you need to set the GDAL_DRIVER_PATH environmenet variable to point to the directory where your plugin + SOSI/FYBA DLLs are installed.

by sbl, 10 years ago

Attachment: sosi_plugin.diff added

Changed build-routines for SOSI-support as plugin

comment:28 by sbl, 10 years ago

With the diff I attached in place, GDAL could also be built with SOSI support as a plugin on Windows.

comment:29 by Even Rouault, 10 years ago

Looks good. trunk r26547 : "Windows build: add support for building the OGR SOSI driver as a plugin (patch by sbl, #3638)"

comment:30 by Jukka Rahkonen, 9 years ago

Ticket seems to be ready to be closed or is there still something more to come? Build process is documented in http://trac.osgeo.org/gdal/wiki/SOSI.

comment:31 by sbl, 9 years ago

Resolution: fixed
Status: reopenedclosed

Closing the ticket. OSGeo4W provides the driver as plugin and on Debian / Ubuntu at least a package of the underlying OpenFYBA library is available... For future enhancements (e.g. SOSI 4.5 (and UTF-8) support or writing functinality) new and more specific tickets could be filed...

Note: See TracTickets for help on using tickets.