Skip to content

Commit

Permalink
[OGR provider] Improve detection of layer geometry type
Browse files Browse the repository at this point in the history
When a OGR layer is of geometry type "unknown", we use the
geometry of the first feature to guess the geometry type. But
if this feature has no geometry, then we assume that we have no
geometry for the whole layer. This is a bit extreme. Let us
allow to probe a few features before giving up.

Fixes #15065
  • Loading branch information
rouault committed Jun 18, 2016
1 parent 7b7c377 commit 3b65568
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 12 deletions.
25 changes: 14 additions & 11 deletions src/providers/ogr/qgsogrprovider.cpp
Expand Up @@ -706,23 +706,26 @@ OGRwkbGeometryType QgsOgrProvider::getOgrGeomType( OGRLayerH ogrLayer )

// Some ogr drivers (e.g. GML) are not able to determine the geometry type of a layer like this.
// In such cases, we use virtual sublayers for each geometry if the layer contains
// multiple geometries (see subLayers) otherwise we guess geometry type from first feature
// multiple geometries (see subLayers) otherwise we guess geometry type from the first
// feature that has a geometry (limit us to a few features, not the whole layer)
if ( geomType == wkbUnknown )
{
geomType = wkbNone;
OGR_L_ResetReading( ogrLayer );
OGRFeatureH firstFeature = OGR_L_GetNextFeature( ogrLayer );
if ( firstFeature )
for ( int i = 0; i < 10; i++ )
{
OGRGeometryH firstGeometry = OGR_F_GetGeometryRef( firstFeature );
if ( firstGeometry )
{
geomType = OGR_G_GetGeometryType( firstGeometry );
}
else
OGRFeatureH nextFeature = OGR_L_GetNextFeature( ogrLayer );

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Jun 27, 2016

Collaborator

@rouault I'm seeing a crash here about 50% of the time in the TestQgsDataItem::testDirItemChildren() test. I can't work out what's causing this - it's happening on the first feature (i=0), so to me the code path looks the same as before.

This comment has been minimized.

Copy link
@rouault

rouault Jun 27, 2016

Author Contributor

I don't see anything wrong in the code and have run "valgrind output/bin/qgis_dataitemtest", which didn't report anything wrong. Have you tried valgrind ?

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Jun 27, 2016

Collaborator

I couldn't see anything wrong either, but the test is definitely flaky now! Here's the valgrind output I get:

QDEBUG : TestQgsDataItem::testDirItemChildren() src/providers/gdal/qgsgdalprovider.cpp: 2208: (buildSupportedRasterFileFilterAndExtensions) [2ms] Raster extension list built: vrt ovr tif tiff ntf toc xml img gff asc ddf dt0 dt1 dt2 png jpg jpeg mem gif gif n1 xpm bmp pix map mpr mpl rgb hgt ter ter nc nc hdf jp2 j2k grb jp2 rsw nat rst grd grd grd hdr rda pnm bil hdr bt lcp gtx gsb ACE2 hdr kro rik dem gxf hdf5 grd grc gen img blx sqlite sdat xyz hf2 pdf e00 webp dat bin mbtiles ppi zip gz tar tar.gz tgz
QDEBUG : TestQgsDataItem::testDirItemChildren() src/providers/gdal/qgsgdaldataitems.cpp: 288: (dataItem) [124ms] GDALOpen error # 1 : Couldn't determine X spacing 
QDEBUG : TestQgsDataItem::testDirItemChildren() src/core/qgsdataitem.cpp: 645: (setState) [141ms] item /home/nyall/dev/QGIS/tests/testdata/bug5598.shp set state 0 -> 2
QDEBUG : TestQgsDataItem::testDirItemChildren() src/core/qgsdataitem.cpp: 645: (setState) [107ms] item /home/nyall/dev/QGIS/tests/testdata/checker360by180.asc set state 0 -> 2
==14044== Invalid read of size 1
==14044==    at 0xC46C590: ??? (in /usr/lib/libgdal.so.1.18.3)
==14044==    by 0xC46C867: ??? (in /usr/lib/libgdal.so.1.18.3)
==14044==    by 0xC46B847: ??? (in /usr/lib/libgdal.so.1.18.3)
==14044==    by 0xC46B911: ??? (in /usr/lib/libgdal.so.1.18.3)
==14044==    by 0x37BFE373: QgsOgrProvider::getOgrGeomType(void*) (qgsogrprovider.cpp:760)
==14044==    by 0x37C1C5DC: dataItemForLayer(QgsDataItem*, QString, QString, void*, int) (qgsogrdataitems.cpp:131)
==14044==    by 0x37C1E323: dataItem (qgsogrdataitems.cpp:384)
==14044==    by 0x59258AA: QgsDataItemProviderFromPlugin::createDataItem(QString const&, QgsDataItem*) (qgsdataitemproviderregistry.cpp:46)
==14044==    by 0x591D08A: QgsDirectoryItem::createChildren() (qgsdataitem.cpp:863)
==14044==    by 0x404E2F: TestQgsDataItem::testDirItemChildren() (testqgsdataitem.cpp:116)
==14044==    by 0x40618B: TestQgsDataItem::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (testqgsdataitem.moc:59)
==14044==    by 0x4FCD6B4: QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (in /usr/lib/x86_64-linux-gnu/libQtCore.so.4.8.7)
==14044==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==14044== 
QFATAL : TestQgsDataItem::testDirItemChildren() Received signal 11
FAIL!  : TestQgsDataItem::testDirItemChildren() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 2 passed, 1 failed, 0 skipped

unfortunately ubuntu doesn't have debug packages available for the gdal build, so i can't get anything more specific than that...

This comment has been minimized.

Copy link
@rouault

rouault Jun 27, 2016

Author Contributor

Looks like a bug in GDAL itself. Which version is it ? Could you add a QgsDebugMsg to output the name of the dataset that is opened so perhaps to have a better clue of which driver is concerned ?

This comment has been minimized.

Copy link
@rouault

rouault Jun 27, 2016

Author Contributor

Reproduced on Ubuntu 16.04 with GDAL 1.11.3. Workarounded with e02661c and better fixed with https://trac.osgeo.org/gdal/ticket/6558

The crash occured on tests/testdata/curved_polys.gpkg

if ( !nextFeature )
break;

OGRGeometryH geometry = OGR_F_GetGeometryRef( nextFeature );
if ( geometry )
{
geomType = wkbNone;
geomType = OGR_G_GetGeometryType( geometry );
}
OGR_F_Destroy( firstFeature );
OGR_F_Destroy( nextFeature );
if ( geomType != wkbNone )
break;
}
OGR_L_ResetReading( ogrLayer );
}
Expand Down
13 changes: 12 additions & 1 deletion tests/src/python/test_provider_ogr.py
Expand Up @@ -16,7 +16,7 @@
import shutil
import tempfile

from qgis.core import QgsVectorLayer, QgsVectorDataProvider
from qgis.core import QgsVectorLayer, QgsVectorDataProvider, QgsWKBTypes
from qgis.testing import (
start_app,
unittest
Expand Down Expand Up @@ -66,6 +66,17 @@ def testUpdateMode(self):
self.assertTrue(vl.dataProvider().leaveUpdateMode())
self.assertEqual(vl.dataProvider().property("_debug_open_mode"), "read-write")

def testGeometryTypeKnownAtSecondFeature(self):

datasource = os.path.join(self.basetestpath, 'testGeometryTypeKnownAtSecondFeature.csv')
with open(datasource, 'wt') as f:
f.write('id,WKT\n')
f.write('1,\n')
f.write('2,POINT(2 49)\n')

vl = QgsVectorLayer(u'{}|layerid=0'.format(datasource), u'test', u'ogr')
self.assertTrue(vl.isValid())
self.assertEqual(vl.wkbType(), QgsWKBTypes.Point)

if __name__ == '__main__':
unittest.main()

0 comments on commit 3b65568

Please sign in to comment.