OGR GTM Driver

[lpiga]

Today I finished implementing a OGR GTM Driver. The driver deals with GPSTrackMaker files whose extension is .gtm

The announcement is on my Website ​http://lampiao.lsc.ic.unicamp.br/~piga/ogr_gtm_driver.html

The driver files can be found on my Website, too. ​http://lampiao.lsc.ic.unicamp.br/~piga/ogr_gtm_driver_files.html

I'd like to know if it is possible to distribute the driver with the official implementation of GDAL. Tomorrow I'll write a detailed documentation.

[lpiga]

Patch for adding the driver on the last svn revision (17580)

[Even Rouault]

Leaonardo,

first, your patch only contains the modified files that already exists in GTM, not the new ones you added. I guess you didn't 'svn add ogr/ogrsf_frmts/gtm', which explains why your 'svn diff > gtm.patch' didn't include them.

I was a bit frustrated, so I decided to add manually the files from your website in my gdal tree and give it a try.

Well, I think you have a few fixes to do. I downloaded a sample file from ​http://www.gpstm.com, namely this one ​http://www.gpstm.com/download.php?page=7&file=63 (La Paz - peru), gunzip'ed it. The good news is that ogrinfo did recognize it, but :

1) First try :

$ogrinfo -ro -al lapaz_peru
INFO: Open of `lapaz_peru'
      using driver `GPSTrackMaker' successful.

Layer name: lapaz_peru_waypoints
Geometry: Point
Feature Count: 20
Layer SRS WKT:
GEOGCS["WGS 84",
    DATUM["WGS_1984",
        SPHEROID["WGS 84",6378137,298.257223563,
            AUTHORITY["EPSG","7030"]],
        TOWGS84[0,0,0,0,0,0,0],
        AUTHORITY["EPSG","6326"]],
    PRIMEM["Greenwich",0,
        AUTHORITY["EPSG","8901"]],
    UNIT["degree",0.0174532925199433,
        AUTHORITY["EPSG","9108"]],
    AUTHORITY["EPSG","4326"]]
name: String (0.0)
comment: String (0.0)
icon: Integer (0.0)

Layer name: lapaz_peru_tracks
Geometry: Line String
Feature Count: 4
Layer SRS WKT:
GEOGCS["WGS 84",
    DATUM["WGS_1984",
        SPHEROID["WGS 84",6378137,298.257223563,
            AUTHORITY["EPSG","7030"]],
        TOWGS84[0,0,0,0,0,0,0],
        AUTHORITY["EPSG","6326"]],
    PRIMEM["Greenwich",0,
        AUTHORITY["EPSG","8901"]],
    UNIT["degree",0.0174532925199433,
        AUTHORITY["EPSG","9108"]],
    AUTHORITY["EPSG","4326"]]
name: String (0.0)
type: Integer (0.0)
color: Integer (0.0)

The feature count is not 0, so apparently the driver has some problems to rewind to the beginning.

2) Then I decided to avoid the call of GetFeatureCount() and added the -q parameter:

$ogrinfo -ro -al -q lapaz_peru

Layer name: lapaz_peru_waypoints
OGRFeature(lapaz_peru_waypoints):0
  name (String) = 
  comment (String) = 
  icon (Integer) = 0
  POINT (0.0 90134834483887118685051674187999860337026435105479681886132371456.0)

OGRFeature(lapaz_peru_waypoints):1
  name (String) = 
  comment (String) = 
  icon (Integer) = 0
  POINT (0.0 0.0)

OGRFeature(lapaz_peru_waypoints):2
  name (String) =    
  comment (String) = (
  icon (Integer) = -15168
  POINT (0 0)

OGRFeature(lapaz_peru_waypoints):3
  name (String) = 
  comment (String) = 
  icon (Integer) = -23271
  POINT (0.0 -0.0)

OGRFeature(lapaz_peru_waypoints):4
  name (String) = Ë©œQë$¨ÉU&
  comment (String) = –Ìžô¿[/g=[»ÎÖúûAßíwoº±ï—ç–û,]?oŽÜ÷÷´æ—_ªÍTèŸ
(ìû+jé{»¶vwÑï×èYt½­zs[
{ÅñÅ*òY^KµÊϺ²r^e*ïߣWU¦Œ£­…Ð×OÛÖPÍùe
’Ë+Ì!}Æã#nk(y*…µÊVXP¶À)Ž«c_a²5Q§A—ÝhosDL-¾0ѯY¯ßµ{Ö#tÉ3‘Tl¿9=¶âg´ŠömšŽížÝ{tÆ“Ҳꪎç±ì+qOg„WßNWX(„¢r¾Úq
]ýv*³WÔöVbž
UßØê¢Õ
þ9’ÜyûƒU™íiÎ4Â}¦ï•Sû:õ]Mµh¾võ³éíË;z(-½ŽíÉ9è×¾}­ù¿¡l|·ýÕ…Iù¬$c!qç"d¤Lˆ$,†^ý–-£5ºn=oA
V¥Œ_|¿Ó¦Ý=q¯F@tK¨ÙljŒ¸N@}j€œêèr[giÓa¦›s[\¥
"€ZÄå|„NÝ4û“_–o{¾«eÝ„ê¶ =v_=y5ç²É‹Þj„{
ÍÝcìvUÙuœc×îÕ¿ì{)Ã$;)Û-m?éËoqQX9[¦0„²Ý€Â‘Sü_
kìôe¬ÃUªïcÁÌYŠ"É'ñ‡«Ï»5ÔΛ@r"äøX?÷8¯†~RGT#‚®ºì`ÆÈ“+GQŽˆõ@[e¦:³ª°êšúíycÖÏ56u“é»
¶ö}}aí¢(³¿à»
óè„«²¶À­»!
’×I¢ã¦¬_²x:p£Ñu3V|o€|WÏÑfÌgÒuVHÑÔU9m¢”;=VSw}Ûf·ý·µZ7OV›sÛׇs;8
S‡‘\Ÿâ7Š·Ùu\'ªwUCFbJyJé¥~m½öxÕ°ˆÈ‚3Q÷YÓkêéùW¶z µmu]ÅòëÃØb†M½ŒòçÃŽîÇ´…ðÓÖi§u½¯aÛo–œpì-uÙúÁ†
[T;>Ò;'Öhûx‹3ÊÞèÝ¢£fšç»u›¬öwc"7ÆÖQІ°6A{´HUÆؐd•Bhá¦j8²øì¢_a˜ëî–Ÿˆ4'ñÞÒ*Üzra(H°“H?KaŽË­¬«)×{±³0øö‚‡@nŽ=p¯'afʝ?
UâënÔ¿ ³®Ôe§\íÅòbz~ô,½wauYþG³¨§ešN¾»ãç·Ïƒ¾¾zå²…Þõ›a¨iÛM›:¾ÆÑ
“´vÙ'ŠáôÄb,Å	êì4õâ*ÊgLŒdeö·.À9‚B(B+Û@…
A	Ø
  icon (Integer) = -16128
  POINT (0 0)

OGRFeature(lapaz_peru_waypoints):5
  name (String) = ÿÚ
  comment (String) = qÜÈb}%Ü/Îì¡yî·mZö!uªÆL™2dÉ“&L™2dÉ“&L™2dÉ“&L™2dÉ“&L™2dÉ“uÈä$ÉÜžâ'(z‰öR=âHS‰U—€
öu+؍úó¢ÆL™2dÈÉ“&L™2dÉ“&L›£&L™2dÉ“&L™2dɺ2`£ƒ*È	»Dâ#0UŸ×ûF8ÊÊ×?ˆD-j¶!³§f¹dËÉ“&L™2dÉ“&L™2dÉ“&L™2dÉ“&L™2dÉ“&LIˆ!”dBCÕ‰Œ°hJ,*-`	œ3‰× a±®iµ“&L™2dÉ“&X¦L±L™2dÉ“&L™2dÉ‘	“&L™2dÉ”Äd‡x?sØÄ÷ŒÆ³Ø[9X%˜®Eþá
  icon (Integer) = -9898
  POINT (0 0)

OGRFeature(lapaz_peru_waypoints):6
  name (String) = âàjí¶ÔLÇ+
  comment (String) = | %AŒ„Ì°¯Y¦uŽxUµ%×	Ø«ÔÚxhn‘GÇù
  icon (Integer) = 17221
  POINT (0 0)

and at that points, ogrinfo segfaults. gdb shows that the call to Waypoint* poWaypoint = poDS->fetchNextWaypoint(); at line 242 of gtmwaypointlayer.cpp returns a NULL pointer which causes a crash just after. But obviously the content before the crash is garbaged. So, are the files from that site supposed to be supported ? If not, you should do something to support the ones from that site, or detect that they are in a version not supported by your driver. Or does it work for you and it is something specific to my environment which is currently XP/MinGW GCC 4.4 (my Linux machine has crashed, so I'm stucked with an old windows beast borrowed). A few GDAL/OGR drivers have needed specific patches for MinGW, but after quick review of your code, I don't think that's the issue here.

3) Pointing to/Attaching samples that work for you would be helpful too. Ideally, we should have a bit of testing as a python script in autotest/ogr wich freely redistributable small data like it's done for all other drivers. But that might come later once the issues with the code itself are solved

4) My quick code review also showed a major portability issue when you read/write binary data. Your code will only work on little-endian machines, and machines who have no memory alignment constraints. SPARC32 for example has neither of this 2 properties, so you should use the endianness portability macros of cpl_port.h

For example, in gtm.cpp, I see readFile( &stringSize, 1, 2 ) where stringSize is an unsigned int. You could rather create a readShort() method that would do : { unsigned short val; readFile(&val, 1, 2); return CPL_LSBINT16PTR(&val); } the same for int, doubles

The VSIFWriteL() calls in OGRGTMDataSource::~OGRGTMDataSource() would need similar fixing.

As far as the alignment problem, here's an example in GTMWaypointLayer::WriteFeatureAttributes(). The icon written at line 163 of gtmwaypointlayer.cpp is at offset 12+commentLength in its buffer. On some architectures, this will cause a SIGBUS if (12 + commentLength) is not a multiple of the type written, here sizeof(unsigned short)=2. So, you should rather do something like

unsigned short tmp; tmp = icon; tmp = CPL_LSBWORD16(tmp); memcpy(pBuffer+12+commentLength, tmp, sizeof(tmp));

5) Minor point : in GTMWaypointLayer::GTMWaypointLayer() and GTMTrackLayer::GTMTrackLayer(), I have noticed a reference to the KML driver in the error message...

6) I have noticed that comment "We are implementing just WGS84, although GTM supports other datum formats". Is it checked somewhere to trigger an error/warning to the user ? Advertizing WGS84 when it's not the case might be dangerours. Not dealing with all datums is fine I guess, but some warning is needed at minimum.

7) On the style point of view, I have hardly anything to say. Looks mostly as the average OGR driver. I have just noticed a few tab characters that should be expanded to spaces. And you use mostly 2 spaces indentation. We can probably live with that.

8) and yes, documentation of the driver would be cool too.

And that's all for now ;-)

[lpiga]

Your comments are welcome. I haven't tested with ogrinfo, this explains 1 and 2

1) I haven't implemented FeatureCount() and also I have forgotten to reset some counters, that's why the ogrinfo without -q was returning 0.

2) I haven't tested with ogrinfo, just ogr2ogr. The latter does a call to ResetReading() the former didn't; so, it was causing segmentation falt when calling ogringo with -q

3) I'll do that.

4) I follow your suggestion and I've already done these changes. Plese, check if I did all necessary changes to have a portable code.

5) Copy and paste problem. I have corrected it already.

6) Actually I'm not 100% sure about this. GTM supports writing its features using other data, like SAD and etc. I forced the driver to write just using WSG84. I think GTM doesn't support keep features without data. In this way, I've copy the KML transformation, but I'm not sure if it is working.

7) I've used the power of emacs and corrected the indentation problem.

8) Because of these problems, I haven't written the documentation yet.

I'm attaching a new patch with the changes and including the new files. Hope this time it works.

[lpiga]

Patch with the corrections

[Even Rouault]

Leonardo,

I've pushed in trunk in r17588 your GTM driver with the few following additionnal fixes and improvements :

• void GTM::rewindTrack() : add missing trackpointFetched = 0 reinit so that ogrinfo works properly on those layers
• GetNextFeature() methods : add checks when fetchNextWaypoint() / fetchNextTrack() return NULL. This could probably occur if the file was shorter than expected
• GTM::isValid() : add a forgotten endianness macro
• added missing windows makefile.vc
• assign poSRS to each returned feature's geometry (found by utility apps/test_ogrsf)
• GetFeatureCount() must use the slow path when a spatial or attribute filter is specified (found by utility apps/test_ogrsf)
• OGRGTMLayer::TestCapability(OLCFastFeatureCount) can return TRUE if no filter is active
• in OGRGTMDataSource::Open(), the bTestOpen parameter was misnamed. it is bUpdate. so if bUpdate == TRUE, the driver returns NULL as the driver supports only read-only or write-only mode
• implement OGRGTMLayer::CreateField() to avoid an error message when the name of the field is the one expected by each layer
• fix GTM::fetchNextWaypoint() and fetchNextTrack() so that a comment of size 0 doesn't prevent from reading the waypoint
• fix waypoint name trimming when the name is empty
• in GTM::findFirstTrackpointOffset(), add a boolean attribute to readUShort() to indicate success of the read. The seek() in practice never fails, so the only mean to check that the seek is correct is to check the read. This is usefull if the GTM header has a corrupted waypoint number. Without the check, we'll loop for a very long time
• in GTM::readHeaderNumbers(), check that nwptstyles, nwpts, ntcks, n_maps and n_tk are not < 0. n_tk < 0 could cause GetFeatureCount() to return -1, which may be a problem if an app uses that value to create an array.
• in OGRGTMDataSource::Create(), use CPLGenerateTempFilename() instead of tmpnam() for better portability

in OGRGTMDataSource::AppendTemporaryFiles(), replace (numTrackpoints != 0 && numTracks != 0) with (numTrackpoints != 0

numTracks != 0)

• in OGRGTMDataSource::~OGRGTMDataSource(), use VSIUnlink to destroy the temporary files
• in OGRGTMDataSource::Create(), fix copy&paste typo in tests for checking if fpTmpTrackpoints != NULL and fpTmpTracks != NULL
• in CreateFeature() methods, actually use the poCT method to reproject the source geometry from source layer SRS to WGS84

About 6), the latest mentionned fix should be OK for the write part. But for the reading part, I guess there's somewhere in the header a datum code that should be checked to see if it's WGS84.

From now, you can attach changes as patches against what is in trunk.

[Even Rouault]

r17589 /trunk/gdal/ogr/ogrsf_frmts/gtm/ogrgtmdatasource.cpp: Fix compilation with -DDEBUG

[lpiga]

Thank you for the improvements. I did some minor corrections. Also this version displays a warning message if the user tries to open a file that is not using WGS84 datum.

I'm attaching the patch

[lpiga]

Some minor corretions and warning message when opening a file that is not using WGS84 datum

[Even Rouault]

gdal-ogr-gtm-20090827_2.patch applied in r17592 with minor change in the warning message to reflect clearly the fact that no conversion will be done.

By the way, is there a specification of the format somewhere ?

[lpiga]

Yes, there is. ​http://www.gpstm.com/download/GTM211_format.pdf

[Even Rouault]

Cool. I've added in r17594 support for transparent reading of .gtz (gzip compressed) files. Was quite easy as CPL has a VSIF*L gzip "driver"

[lpiga]

MULTILINESTRING layer support was not working correctly. The patch gdal-ogr-gtm-20090901.patch fixes it.

[lpiga]

Correction for supporting multilinestring layer

[Even Rouault]

r17604 /trunk/gdal/ogr/ogrsf_frmts/gtm/ (gtmtracklayer.cpp ogrgtmdatasource.cpp): GTM driver : Correction for supporting multilinestring layer (by lpiga, #3113)

[lpiga]

I've started writing the documentation. Could you suggest what else I may write?

I'm attaching the patch.

[lpiga]

Starting the documentation

[Even Rouault]

r17608 /trunk/gdal/ogr/ogrsf_frmts/ (gtm/drv_gtm.html ogr_formats.html): GTM driver: add documentation.

r17609 /trunk/gdal/ogr/ogrsf_frmts/gtm/drv_gtm.html: GTM : Mention that it is available since GDAL 1.7.0

I've just added that we assumed the GTM file to be WGS84 too when reading.

[lpiga]

Great! On your first comment you mentioned something about testing and autotest (third item).

I couldn't find the folder with the samples. Could you give me some instruction on how to provide these sample files?

[Even Rouault]

The GDAL/OGR autotest suite is made of Python scripts using the GDAL/OGR Python API. You can find them there for OGR : http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. You can take example on one existing script and adapt it for GTM. For testing the read part, either you can put a very small test file in the data directory (freely redistributable of course), ideally not produced by the driver but by a third-party "reference" software, or if it is not possible, produce it with the driver and read it back.

[Even Rouault]

Leonardo, I'm attaching a gtm_time.patch that adds the capability to read and write the date of waypoints, as well as reading the altitude as the Z coordinate (you already provided support for writing it). If you're fine with it, I'll commit it.

[lpiga]

Sure, you can commit it. I think that's the spirit of free software. Everybody can contribute.

[Even Rouault]

As you're the author of the driver, I just wanted to be sure that it looked OK to you from a technical point of view. Patch commited in r17612.

[lpiga]

OK. I've read your code and tested it. For waypoints it is working perfect!

But for tracks we have to think better. Because we are reading altitude and datetime, but we are not using it.

On my implementation I merged tracks and trackpoints concepts using linestrings. Because tracks are sequence of points we can do it. But when we add altitude and datetime we have two additional metadata for each point of tracks. So, altitude and datetime are being just read but we are not storing it.

I think we can create and extra layer called trackpoint so that we can store this extra information.

[lpiga]

Or we can leave tracks without altitude and date. What do you think?

[Even Rouault]

I just added the low level support for altitude and datetime in tracks for the sake of completness, but full support would of course require extra work.

If you are interested into it, you could do something similar to what is done in the GPX driver, that is to say create an additional trackpoint layer in which each feature is a track point that can have additionnal fields such as the datetime, a trackid fields that contains the index of the corresponding track and a trackptid that is the index of the trackpoint inside its track. For the altitude there are two possibilities, it could be reported as the Z coordinate of each point of the linestring layer and/or for each point in the trackpoint layer.

[Even Rouault]

Leaonardo,

are you interested in providing a test script in the Python autotest suite for the OGR GTM driver ?

[lpiga]

Yes, I am. I just need some information about how to do it. Could you give me some?

[Even Rouault]

See my above comment in the ticket : "The GDAL/OGR autotest suite is made of Python scripts using the GDAL/OGR Python API....". So a requirement is of course that you've working GDAL/OGR python bindings.

If you have specific questions, don't hesitate to ask.

[lpiga]

Autotest scripf for OGR GTM Driver

[lpiga]

File to be used with the autotest script

[lpiga]

I've started doing the autotest script. I think the read part is done. How can I do a test for writing? I write a gtm file using Python than I read it? Is it correct?

[Even Rouault]

Leonardo,

Thanks! I've commited your test in r18366 /trunk/autotest/ogr/ (data/samplemap.gtm ogr_gtm.py): Add autotest for OGR GTM driver (contributed by Leonardo Piga, #3113)

I had to comment out the 'if feat.GetField('time') != None' tests on the track layer, as the field doesn't exist for that layer and it raised an exception. Which version of the python bindings do you use ?

For testing the writing, yes writing a GTM file with the OGR API and then reading it back should be sufficient.

[lpiga]

I'm using python 2.6. I didn't know how to deal with null time. Then I print the feat.GetField?('time') and it returned me None. So I did the test with none values and it worked fine on my computer (Ubuntu 9.04 with python 2.6). Anyway, by commenting out the lines must work as well.

I'm coding the write test. It should be ready soon.

[Even Rouault]

I meant : do you use old generation python bindings or 'new' generation (swig generated) python bindings ? In the latter case, did you refresh them recently ? I'm just surprised I get an exception (I also tested with Python 2.6, but the Python version is not the most discriminant part), and you not.

[lpiga]

Sorry, but I didn't understand your question. How do I check if I have the old or new generation?

I checked out the trunk version, configured it, ran make and then make install. Just it. Then I ran the autotest.

[Even Rouault]

If you do 'import gdal' inside python console and if it says 'gdal.py was placed in a namespace, it is now available as osgeo.gdal (DeprecationWarning)', it means that you use new generation bindings.

[lpiga]

Patch with write test

[lpiga]

So, I'm using the new generation bindings.

I've finished the write test implementation. Please, take a look at my last patch and check if everything is OK.

Is there anything else to do?

Thanks

[Even Rouault]

Commited in r18376 /trunk/autotest/ogr/ogr_gtm.py: Add tests for OGR GTM write support (#3113)

I think that's just perfect now.

Thanks for your work Leonardo.

I'm closing the ticket.

[Even Rouault]

Your write test enabled me to realize that one piece of magic was missing for windows support .

r18381 /trunk/gdal/ogr/ogrsf_frmts/generic/makefile.vc: Add missing -DGTM_ENABLED to make GTM driver works on Win32 (#3113)