[GDAL provider] Revise how referencing counting is done on mGdalBaseDataset

rouault · rouault · commit 651ccb43bf02 · 2018-10-14T19:10:22.000+02:00
initBaseDataset() used to take a reference in the case where
mGdalDataset == mGdalBaseDataset (non warped VRT) and we dropped it when
closing the dataset, which was OK

However buidPyramids() failed to acquire this reference. There was no
negative consequence as GDALDerefenceDataset() just decremented a
reference count, and GDALClose() ignores it for a non-shared dataset,
however this was an incorrected use

It is simpler for the mind to call GDALDerefenceDataset() only when
mGdalBaseDataset != mGdalDataset.
diff --git a/src/providers/gdal/qgsgdalprovider.cpp b/src/providers/gdal/qgsgdalprovider.cpp
@@ -392,7 +392,7 @@ bool QgsGdalProvider::cacheGdalHandlesForLaterReuse( QgsGdalProvider *provider,
       {
         mgDatasetCacheSize --;
         DatasetPair pair = mgDatasetCache[ candidateProvider ].takeLast();
-        if ( pair.mGdalBaseDataset )
+        if ( pair.mGdalBaseDataset != pair.mGdalDataset )
         {
           GDALDereferenceDataset( pair.mGdalBaseDataset );
         }
@@ -435,7 +435,7 @@ void QgsGdalProvider::closeCachedGdalHandlesFor( QgsGdalProvider *provider )
     {
       mgDatasetCacheSize --;
       DatasetPair pair = iter.value().takeLast();
-      if ( pair.mGdalBaseDataset )
+      if ( pair.mGdalBaseDataset != pair.mGdalDataset )
       {
         GDALDereferenceDataset( pair.mGdalBaseDataset );
       }
@@ -464,7 +464,7 @@ QgsGdalProvider::~QgsGdalProvider()
     }
     else
     {
-      if ( mGdalBaseDataset )
+      if ( mGdalBaseDataset != mGdalDataset )
       {
         GDALDereferenceDataset( mGdalBaseDataset );
       }
@@ -498,7 +498,10 @@ void QgsGdalProvider::closeDataset()
   }
   mValid = false;
 
-  GDALDereferenceDataset( mGdalBaseDataset );
+  if ( mGdalBaseDataset != mGdalDataset )
+  {
+    GDALDereferenceDataset( mGdalBaseDataset );
+  }
   mGdalBaseDataset = nullptr;
 
   GDALClose( mGdalDataset );
@@ -2606,7 +2609,6 @@ void QgsGdalProvider::initBaseDataset()
     {
       QgsLogger::warning( QStringLiteral( "Warped VRT Creation failed." ) );
       mGdalDataset = mGdalBaseDataset;
-      GDALReferenceDataset( mGdalDataset );
     }
     else
     {
@@ -2616,7 +2618,6 @@ void QgsGdalProvider::initBaseDataset()
   else
   {
     mGdalDataset = mGdalBaseDataset;
-    GDALReferenceDataset( mGdalDataset );
   }
 
   if ( !hasGeoTransform )
@@ -2645,12 +2646,7 @@ void QgsGdalProvider::initBaseDataset()
     {
       appendError( ERRMSG( tr( "Cannot get GDAL raster band: %1" ).arg( msg ) ) );
 
-      GDALDereferenceDataset( mGdalBaseDataset );
-      mGdalBaseDataset = nullptr;
-
-      GDALClose( mGdalDataset );
-      mGdalDataset = nullptr;
-      mValid = false;
+      closeDataset();
       return;
     }
     // if there are subdatasets, leave the dataset open for subsequent queries

Original file line number	Diff line number	Diff line change
`@@ -392,7 +392,7 @@ bool QgsGdalProvider::cacheGdalHandlesForLaterReuse( QgsGdalProvider *provider,`
`392`	`392`	`{`
`393`	`393`	`mgDatasetCacheSize --;`
`394`	`394`	`DatasetPair pair = mgDatasetCache[ candidateProvider ].takeLast();`
`395`		`- if ( pair.mGdalBaseDataset )`
	`395`	`+ if ( pair.mGdalBaseDataset != pair.mGdalDataset )`
`396`	`396`	`{`
`397`	`397`	`GDALDereferenceDataset( pair.mGdalBaseDataset );`
`398`	`398`	`}`
`@@ -435,7 +435,7 @@ void QgsGdalProvider::closeCachedGdalHandlesFor( QgsGdalProvider *provider )`
`435`	`435`	`{`
`436`	`436`	`mgDatasetCacheSize --;`
`437`	`437`	`DatasetPair pair = iter.value().takeLast();`
`438`		`- if ( pair.mGdalBaseDataset )`
	`438`	`+ if ( pair.mGdalBaseDataset != pair.mGdalDataset )`
`439`	`439`	`{`
`440`	`440`	`GDALDereferenceDataset( pair.mGdalBaseDataset );`
`441`	`441`	`}`
`@@ -464,7 +464,7 @@ QgsGdalProvider::~QgsGdalProvider()`
`464`	`464`	`}`
`465`	`465`	`else`
`466`	`466`	`{`
`467`		`- if ( mGdalBaseDataset )`
	`467`	`+ if ( mGdalBaseDataset != mGdalDataset )`
`468`	`468`	`{`
`469`	`469`	`GDALDereferenceDataset( mGdalBaseDataset );`
`470`	`470`	`}`
`@@ -498,7 +498,10 @@ void QgsGdalProvider::closeDataset()`
`498`	`498`	`}`
`499`	`499`	`mValid = false;`
`500`	`500`
`501`		`- GDALDereferenceDataset( mGdalBaseDataset );`
	`501`	`+ if ( mGdalBaseDataset != mGdalDataset )`
	`502`	`+ {`
	`503`	`+ GDALDereferenceDataset( mGdalBaseDataset );`
	`504`	`+ }`
`502`	`505`	`mGdalBaseDataset = nullptr;`
`503`	`506`
`504`	`507`	`GDALClose( mGdalDataset );`
`@@ -2606,7 +2609,6 @@ void QgsGdalProvider::initBaseDataset()`
`2606`	`2609`	`{`
`2607`	`2610`	`QgsLogger::warning( QStringLiteral( "Warped VRT Creation failed." ) );`
`2608`	`2611`	`mGdalDataset = mGdalBaseDataset;`
`2609`		`- GDALReferenceDataset( mGdalDataset );`
`2610`	`2612`	`}`
`2611`	`2613`	`else`
`2612`	`2614`	`{`
`@@ -2616,7 +2618,6 @@ void QgsGdalProvider::initBaseDataset()`
`2616`	`2618`	`else`
`2617`	`2619`	`{`
`2618`	`2620`	`mGdalDataset = mGdalBaseDataset;`
`2619`		`- GDALReferenceDataset( mGdalDataset );`
`2620`	`2621`	`}`
`2621`	`2622`
`2622`	`2623`	`if ( !hasGeoTransform )`
`@@ -2645,12 +2646,7 @@ void QgsGdalProvider::initBaseDataset()`
`2645`	`2646`	`{`
`2646`	`2647`	`appendError( ERRMSG( tr( "Cannot get GDAL raster band: %1" ).arg( msg ) ) );`
`2647`	`2648`
`2648`		`- GDALDereferenceDataset( mGdalBaseDataset );`
`2649`		`- mGdalBaseDataset = nullptr;`
`2650`		`-`
`2651`		`- GDALClose( mGdalDataset );`
`2652`		`- mGdalDataset = nullptr;`
`2653`		`- mValid = false;`
	`2649`	`+ closeDataset();`
`2654`	`2650`	`return;`
`2655`	`2651`	`}`
`2656`	`2652`	`// if there are subdatasets, leave the dataset open for subsequent queries`