Opened 16 years ago
Closed 16 years ago
#2146 closed defect (fixed)
Geoconcept Open() not careful enough
Reported by: | warmerdam | Owned by: | dgrichard |
---|---|---|---|
Priority: | normal | Milestone: | 1.6.0 |
Component: | OGR_SF | Version: | svn-trunk |
Severity: | normal | Keywords: | geoconcept |
Cc: | warmerdam, Even Rouault |
Description
It seems that the geoconcept open function is not careful enough when trying to determine if a dataset is in fact a geoconcept file/fileset.
eg. warmerda@gdal64[75]% ogrinfo gdal OGR: OGROpen(gdal/0x607320) succeeded as Geoconcept Export. INFO: Open of `gdal' using driver `Geoconcept Export' successful. INFO: Internal data source name `gdal/LICENSE.TXT' different from user name `gdal'. OGR: GetLayerCount() = 0
(in this case gdal is my source directory).
Attachments (17)
Change History (51)
follow-up: 5 comment:1 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:2 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
follow-up: 6 comment:3 by , 16 years ago
Still problems with single files:
warmerda@gdal64[110]% ogrinfo Mills.mif OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept. INFO: Open of `Mills.mif' using driver `Geoconcept' successful. OGR: GetLayerCount() = 0
follow-up: 7 comment:4 by , 16 years ago
Cc: | added; removed |
---|---|
Owner: | changed from | to
Status: | reopened → new |
For the time being I have disabled the RegisterDriver() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.
/* Let CreateLayer do the job ... */ if( bUpdate ) { _pszName = CPLStrdup( pszName ); return TRUE; }
This logic will need to be updated in some non-obvious way by dgrichard.
comment:5 by , 16 years ago
Replying to warmerdam:
I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?
I have modified accordingly and my test script gives me the same results as before.
We really need a test script for this driver! I don't really know if I've completely broken it.
I have a small (for the moment) shell script for testing. It uses a Shapefile datasets (<1kb) to test basic creation and update as describe in drv_geoconcept.html. I will attach it to this ticket together with test files.
comment:6 by , 16 years ago
Replying to warmerdam:
Still problems with single files:
warmerda@gdal64[110]% ogrinfo Mills.mif OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept. INFO: Open of `Mills.mif' using driver `Geoconcept' successful. OGR: GetLayerCount() = 0
As for the moment, reading the Geoconcept file is still not implemented, I have just started to implement hight level API before coding the parsing of Geoconcept file at low level.
follow-up: 8 comment:7 by , 16 years ago
Replying to warmerdam:
For the time being I have disabled the RegisterDriver() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.
/* Let CreateLayer do the job ... */ if( bUpdate ) { _pszName = CPLStrdup( pszName ); return TRUE; }This logic will need to be updated in some non-obvious way by dgrichard.
Ok, I am going to check how to improve this (but, still need to code the parsing, I guess).
comment:8 by , 16 years ago
Replying to dgrichard:
Replying to warmerdam:
For the time being I have disabled the RegisterDriver() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.
I propose a new patch geoconcept-20081301.patch with lot of changes including the logic. The reading support is about to be completed as asked by Even. There are some more improvements to add like on-the-fly Geoconcept schema creation. I have already posted a new shell script for unit tests.
by , 16 years ago
Attachment: | geoconcept-20081301.patch added |
---|
by , 16 years ago
Attachment: | geoconcept-20080120.patch added |
---|
patch against trunk with better handling of read and SRS and lco CONFIG optional
follow-up: 13 comment:9 by , 16 years ago
I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled).
I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.
comment:10 by , 16 years ago
Cc: | added |
---|
follow-up: 14 comment:11 by , 16 years ago
Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference mainly), and a missing pointer initialization. Could you check that it's OK ?
I've a few observations :
- I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.
- You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.
- Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature())
- For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at https://svn.osgeo.org/gdal/trunk/autotest.
comment:12 by , 16 years ago
Didier, sorry for having mixed your first and last name my previous post.
comment:13 by , 16 years ago
Replying to rouault:
I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled).
I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.
My mistake for mitab_coordsys.cpp, I am working on it and forgot to remove it from the whole patch. Sorry.
comment:14 by , 16 years ago
Replying to rouault:
Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference mainly), and a missing pointer initialization. Could you check that it's OK ?
Seems to. As you noticed in the code, I had some question marks about Reference/Dereference.
I've a few observations :
- I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.
The definition is created at low level, I'll changed that.
- You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.
Yeap, I noticed that. I'll improve the process.
- Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature())
Ok, it is quite cumbersome !
The great thing would have been to have in common tools :
- a index (BTree or like);
- a geometric index (Quad or like);
- For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at https://svn.osgeo.org/gdal/trunk/autotest.
Ok, I'll give it a glance and manage to provide the autotest.
Thanks for all
follow-up: 17 comment:15 by , 16 years ago
Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.
follow-up: 18 comment:16 by , 16 years ago
Didier,
I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.
comment:17 by , 16 years ago
Replying to rouault:
Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.
I have to check for that. Geoconcept ID when set to -1 means the incrementation when importing into the GIS, otherwise use the ID. So, this should be not to complex to fix when writing. I am wondering what to do when reading (the export could have mixed -1 and IDs).
comment:18 by , 16 years ago
Replying to warmerdam:
Didier,
I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.
Frank, the following seems working : --- geoconcept.c 2008-01-20 22:05:48.000000000 +0100 +++ geoconcept.c 2008-01-20 22:05:46.000000000 +0100 @@ -1864,10 +1864,7 @@
theSubType= NULL;
}
- if( GetGCMode_GCIO(H)==vReadAccess_GCIO )
- {
- Rewind_GCIO(H,NULL);
- }
+ Rewind_GCIO(H,NULL);
return Meta;
}/* _completeHeader_GCIO */
I'll send the patch when I am through with other points I need to fix/upgrade.
comment:19 by , 16 years ago
I have added a very minimal read test (r13557) of one of the attached samples. I have so far been unable to successfully create a geoconcept file with ogr2ogr so I haven't adding any write testing.
by , 16 years ago
Attachment: | geoconcept-20080129.patch added |
---|
patch against trunk with various enhancements (bug fix for feature definition, round-tripping of SRS complete, spatial and attribute filters added, better support for update, FID support, etc ...)
comment:21 by , 16 years ago
Didier, I applied geoconcept-20080129.patch in r13624.
In r13625, I also moved some code in OGRSpatialReference2SysCoord_GCSRS, otherwise I was unable to assign EPSG:4326 (or other geographics SRS) as an output SRS for geoconcept. Does it look ok to you ?
I'm also wondering if there wouldn't be a prettier way to test the SRS in OGRSpatialReference2SysCoord_GCSRS. All those hard-coded parameters in the middle of code look scary to me. Couldn't they be put in a few arrays like it seems to have been done in ogr/ogrsf_frmts/mitab/mitab_spatialref.cpp ?
With SRS round-tripping, we should now be able to add write tests in the GDAL autotest.
follow-up: 25 comment:22 by , 16 years ago
Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.
comment:24 by , 16 years ago
comment:25 by , 16 years ago
Replying to rouault:
Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.
Patch for not handled SRS sent. I am currently working on re-organising the SRS comparizon in geoconcept_syscoord.c
by , 16 years ago
Attachment: | geoconcept-20080223.patch added |
---|
Upgrade of coordinate systems handler in Geoconcept driver
follow-up: 28 comment:27 by , 16 years ago
Didier,
I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test.
In http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr/ogr_geoconcept.py, ogr_gxt_2 reads the following file and translates it to Geoconcept, and read it back. The round-trip of the SRS is KO.
Input file :
//$DELIMITER " " //$QUOTED-TEXT "no" //$CHARSET ANSI //$UNIT Distance:m //$FORMAT 2 //$SYSCOORD {Type: 101} //$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class Private#Subclass Private#NbFields Primary_ID Secondary_ID Third_ID Private#X Private#Y -1 points points 3 PID1 SID1 TID1 0 1 -1 points points 3 PID2 SID2 2 3
Output file :
//$DELIMITER " " //$QUOTED-TEXT "no" //$CHARSET ANSI //$UNIT Distance:m //$FORMAT 2 //$SYSCOORD {Type: 113020768};{TimeZone: 113020824} //$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class Private#Subclass Private#NbFields Primary_ID Secondary_ID Third_ID Private#X Private#Y -1 points points 3 PID1 SID1 TID1 0 1 -1 points points 3 PID2 SID2 2 3
There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian_GCSRS(theSysCoord, 0);
Could you look at this ?
comment:28 by , 16 years ago
Replying to rouault:
Didier,
I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test.
There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian_GCSRS(theSysCoord, 0);
Added
Could you look at this ?
Allright, I'll have a look. Nevertheless, my unit tests were OK with ogrgc.sh; I had the same problem as you mentioned, but after a full compilation it was running ... I have also tested the SRS round-trip and EPSG:4326 (Type:101) was OK.
by , 16 years ago
Attachment: | geoconcept-20080103.patch added |
---|
Pragma DELIMITER enhancement, Pragma VERSION addition. Bug fixes in error messages.
comment:31 by , 16 years ago
Frank, Even,
I made few changes and fixes to the Geoconcept driver and change test data accordingly. I wonder whether I may open an enhancement request against Geoconcept for transferring changes/fixes next time or I still use this bug folder ?
I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?
We really need a test script for this driver! I don't really know if I've completely broken it.