Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#3113 closed enhancement (fixed)

OGR GTM Driver

Reported by: lpiga Owned by: Even Rouault
Priority: normal Milestone: 1.7.0
Component: OGR_SF Version: unspecified
Severity: normal Keywords:
Cc:

Description

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.

Attachments (9)

gdal-ogr-gtm-20090826.patch (2.6 KB) - added by lpiga 12 years ago.
Patch for adding the driver on the last svn revision (17580)
gdal-ogr-gtm-20090827.patch (87.3 KB) - added by lpiga 12 years ago.
Patch with the corrections
gdal-ogr-gtm-20090827_2.patch (2.8 KB) - added by lpiga 12 years ago.
Some minor corretions and warning message when opening a file that is not using WGS84 datum
gdal-ogr-gtm-20090901.patch (1.8 KB) - added by lpiga 12 years ago.
Correction for supporting multilinestring layer
gdal-ogr-gtm-20090902.patch (2.0 KB) - added by lpiga 12 years ago.
Starting the documentation
gtm_time.patch (23.8 KB) - added by Even Rouault 12 years ago.
autotest_091222.patch (9.2 KB) - added by lpiga 12 years ago.
Autotest scripf for OGR GTM Driver
samplemap.gtm (28.0 KB) - added by lpiga 12 years ago.
File to be used with the autotest script
autotest_091223.patch (10.9 KB) - added by lpiga 12 years ago.
Patch with write test

Download all attachments as: .zip

Change History (41)

Changed 12 years ago by lpiga

Attachment: gdal-ogr-gtm-20090826.patch added

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

comment:1 Changed 12 years ago by Even Rouault

Owner: changed from warmerdam to Even Rouault
Status: newassigned

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 ;-)

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

Changed 12 years ago by lpiga

Attachment: gdal-ogr-gtm-20090827.patch added

Patch with the corrections

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

comment:4 Changed 12 years ago by Even Rouault

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

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

Changed 12 years ago by lpiga

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

comment:6 Changed 12 years ago by Even Rouault

Milestone: 1.7.0

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 ?

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

comment:9 Changed 12 years ago by lpiga

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

Changed 12 years ago by lpiga

Attachment: gdal-ogr-gtm-20090901.patch added

Correction for supporting multilinestring layer

comment:10 Changed 12 years ago by Even Rouault

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

comment:11 Changed 12 years ago by lpiga

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

I'm attaching the patch.

Changed 12 years ago by lpiga

Attachment: gdal-ogr-gtm-20090902.patch added

Starting the documentation

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

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

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

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

Changed 12 years ago by Even Rouault

Attachment: gtm_time.patch added

comment:16 Changed 12 years ago by lpiga

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

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

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

comment:19 in reply to:  18 Changed 12 years ago by lpiga

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

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

comment:21 Changed 12 years ago by Even Rouault

Leaonardo,

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

comment:22 Changed 12 years ago by lpiga

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

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

Changed 12 years ago by lpiga

Attachment: autotest_091222.patch added

Autotest scripf for OGR GTM Driver

Changed 12 years ago by lpiga

Attachment: samplemap.gtm added

File to be used with the autotest script

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

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

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

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

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

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

Changed 12 years ago by lpiga

Attachment: autotest_091223.patch added

Patch with write test

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

comment:31 Changed 12 years ago by Even Rouault

Resolution: fixed
Status: assignedclosed

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.

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

Note: See TracTickets for help on using tickets.