Utilise thread safe proj API within QgsCoordinateTransform

Avoids unpredictable behavior when transforms are being conducted in background threads, such as map renders. Refs #11441 This commit: 1. Uses thread_local storage for projCtx objects, to ensure that every thread correctly has its own projCtx context. 2. Refactors QgsCoordinateTransformPrivate so that the projPJ source and destination objects are instead stored in a map (by projCtx). This allows transforms to be transparently performed using the correct projPJ objects for the particular thread in which the transform is being conducted. This approach avoids expensive detachment of QgsCoordinateTransformPrivate, and allows a single QgsCoordinateTransformPrivate to be safely utilised by QgsCoordinateTransform objects in different threads.
qgis · May 16, 2017 · 4396e53 · 4396e53
1 parent ae492ab
commit 4396e53
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 42 deletions.
diff --git a/src/core/qgscoordinatetransform.cpp b/src/core/qgscoordinatetransform.cpp
@@ -474,8 +474,8 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double *
 
   // if the source/destination projection is lat/long, convert the points to radians
   // prior to transforming
-  if ( ( pj_is_latlong( d->mDestinationProjection ) && ( direction == ReverseTransform ) )
-       || ( pj_is_latlong( d->mSourceProjection ) && ( direction == ForwardTransform ) ) )
+  if ( ( pj_is_latlong( d->destProjection() ) && ( direction == ReverseTransform ) )
+       || ( pj_is_latlong( d->sourceProjection() ) && ( direction == ForwardTransform ) ) )
   {
     for ( int i = 0; i < numPoints; ++i )
     {
@@ -487,13 +487,13 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double *
   int projResult;
   if ( direction == ReverseTransform )
   {
-    projResult = pj_transform( d->mDestinationProjection, d->mSourceProjection, numPoints, 0, x, y, z );
+    projResult = pj_transform( d->destProjection(), d->sourceProjection(), numPoints, 0, x, y, z );
   }
   else
   {
-    Q_ASSERT( d->mSourceProjection );
-    Q_ASSERT( d->mDestinationProjection );
-    projResult = pj_transform( d->mSourceProjection, d->mDestinationProjection, numPoints, 0, x, y, z );
+    Q_ASSERT( d->sourceProjection() );
+    Q_ASSERT( d->destProjection() );
+    projResult = pj_transform( d->sourceProjection(), d->destProjection(), numPoints, 0, x, y, z );
   }
 
   if ( projResult != 0 )
@@ -515,8 +515,8 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double *
 
     QString dir = ( direction == ForwardTransform ) ? QObject::tr( "forward transform" ) : QObject::tr( "inverse transform" );
 
-    char *srcdef = pj_get_def( d->mSourceProjection, 0 );
-    char *dstdef = pj_get_def( d->mDestinationProjection, 0 );
+    char *srcdef = pj_get_def( d->sourceProjection(), 0 );
+    char *dstdef = pj_get_def( d->destProjection(), 0 );
 
     QString msg = QObject::tr( "%1 of\n"
                                "%2"
@@ -538,8 +538,8 @@ void QgsCoordinateTransform::transformCoords( int numPoints, double *x, double *
 
   // if the result is lat/long, convert the results from radians back
   // to degrees
-  if ( ( pj_is_latlong( d->mDestinationProjection ) && ( direction == ForwardTransform ) )
-       || ( pj_is_latlong( d->mSourceProjection ) && ( direction == ReverseTransform ) ) )
+  if ( ( pj_is_latlong( d->destProjection() ) && ( direction == ForwardTransform ) )
+       || ( pj_is_latlong( d->sourceProjection() ) && ( direction == ReverseTransform ) ) )
   {
     for ( int i = 0; i < numPoints; ++i )
     {

diff --git a/src/core/qgscoordinatetransform.h b/src/core/qgscoordinatetransform.h
@@ -280,7 +280,7 @@ class CORE_EXPORT QgsCoordinateTransform
 
     static void searchDatumTransform( const QString &sql, QList< int > &transforms );
 
-    QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate> d;
+    mutable QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate> d;
 };
 
 //! Output stream operator

diff --git a/src/core/qgscoordinatetransform_p.cpp b/src/core/qgscoordinatetransform_p.cpp
@@ -29,11 +29,21 @@ extern "C"
 
 /// @cond PRIVATE
 
+thread_local QgsProjContextStore QgsCoordinateTransformPrivate::mProjContext;
+
+QgsProjContextStore::QgsProjContextStore()
+{
+  context = pj_ctx_alloc();
+}
+
+QgsProjContextStore::~QgsProjContextStore()
+{
+  pj_ctx_free( context );
+}
+
 QgsCoordinateTransformPrivate::QgsCoordinateTransformPrivate()
   : mIsValid( false )
   , mShortCircuit( false )
-  , mSourceProjection( nullptr )
-  , mDestinationProjection( nullptr )
   , mSourceDatumTransform( -1 )
   , mDestinationDatumTransform( -1 )
 {
@@ -45,8 +55,6 @@ QgsCoordinateTransformPrivate::QgsCoordinateTransformPrivate( const QgsCoordinat
   , mShortCircuit( false )
   , mSourceCRS( source )
   , mDestCRS( destination )
-  , mSourceProjection( nullptr )
-  , mDestinationProjection( nullptr )
   , mSourceDatumTransform( -1 )
   , mDestinationDatumTransform( -1 )
 {
@@ -60,8 +68,6 @@ QgsCoordinateTransformPrivate::QgsCoordinateTransformPrivate( const QgsCoordinat
   , mShortCircuit( other.mShortCircuit )
   , mSourceCRS( other.mSourceCRS )
   , mDestCRS( other.mDestCRS )
-  , mSourceProjection( nullptr )
-  , mDestinationProjection( nullptr )
   , mSourceDatumTransform( other.mSourceDatumTransform )
   , mDestinationDatumTransform( other.mDestinationDatumTransform )
 {
@@ -72,14 +78,7 @@ QgsCoordinateTransformPrivate::QgsCoordinateTransformPrivate( const QgsCoordinat
 QgsCoordinateTransformPrivate::~QgsCoordinateTransformPrivate()
 {
   // free the proj objects
-  if ( mSourceProjection )
-  {
-    pj_free( mSourceProjection );
-  }
-  if ( mDestinationProjection )
-  {
-    pj_free( mDestinationProjection );
-  }
+  freeProj();
 }
 
 bool QgsCoordinateTransformPrivate::initialize()
@@ -109,43 +108,41 @@ bool QgsCoordinateTransformPrivate::initialize()
   bool useDefaultDatumTransform = ( mSourceDatumTransform == - 1 && mDestinationDatumTransform == -1 );
 
   // init the projections (destination and source)
+  freeProj();
 
-  pj_free( mSourceProjection );
-  QString sourceProjString = mSourceCRS.toProj4();
+  mSourceProjString = mSourceCRS.toProj4();
   if ( !useDefaultDatumTransform )
   {
-    sourceProjString = stripDatumTransform( sourceProjString );
+    mSourceProjString = stripDatumTransform( mSourceProjString );
   }
   if ( mSourceDatumTransform != -1 )
   {
-    sourceProjString += ( ' ' + datumTransformString( mSourceDatumTransform ) );
+    mSourceProjString += ( ' ' + datumTransformString( mSourceDatumTransform ) );
   }
 
-  pj_free( mDestinationProjection );
-  QString destProjString = mDestCRS.toProj4();
+  mDestProjString = mDestCRS.toProj4();
   if ( !useDefaultDatumTransform )
   {
-    destProjString = stripDatumTransform( destProjString );
+    mDestProjString = stripDatumTransform( mDestProjString );
   }
   if ( mDestinationDatumTransform != -1 )
   {
-    destProjString += ( ' ' +  datumTransformString( mDestinationDatumTransform ) );
+    mDestProjString += ( ' ' +  datumTransformString( mDestinationDatumTransform ) );
   }
 
   if ( !useDefaultDatumTransform )
   {
-    addNullGridShifts( sourceProjString, destProjString );
+    addNullGridShifts( mSourceProjString, mDestProjString );
   }
 
-  mSourceProjection = pj_init_plus( sourceProjString.toUtf8() );
-  mDestinationProjection = pj_init_plus( destProjString.toUtf8() );
+  initializeCurrentContext();
 
 #ifdef COORDINATE_TRANSFORM_VERBOSE
   QgsDebugMsg( "From proj : " + mSourceCRS.toProj4() );
   QgsDebugMsg( "To proj   : " + mDestCRS.toProj4() );
 #endif
 
-  if ( !mDestinationProjection || !mSourceProjection )
+  if ( !destProjection() || !sourceProjection() )
   {
     mIsValid = false;
   }
@@ -191,6 +188,44 @@ bool QgsCoordinateTransformPrivate::initialize()
   return mIsValid;
 }
 
+void QgsCoordinateTransformPrivate::initializeCurrentContext()
+{
+  mProjLock.lockForWrite();
+  mProjProjections.insert( reinterpret_cast< uintptr_t>( mProjContext.get() ), qMakePair( pj_init_plus_ctx( mProjContext.get(), mSourceProjString.toUtf8() ),
+                           pj_init_plus_ctx( mProjContext.get(), mDestProjString.toUtf8() ) ) );
+  mProjLock.unlock();
+}
+
+projPJ QgsCoordinateTransformPrivate::sourceProjection()
+{
+  mProjLock.lockForRead();
+  if ( mProjProjections.contains( reinterpret_cast< uintptr_t>( mProjContext.get() ) ) )
+  {
+    projPJ src = mProjProjections.value( reinterpret_cast< uintptr_t>( mProjContext.get() ) ).first;
+    mProjLock.unlock();
+    return src;
+  }
+  mProjLock.unlock();
+
+  initializeCurrentContext();
+  return sourceProjection();
+}
+
+projPJ QgsCoordinateTransformPrivate::destProjection()
+{
+  mProjLock.lockForRead();
+  if ( mProjProjections.contains( reinterpret_cast< uintptr_t>( mProjContext.get() ) ) )
+  {
+    projPJ dest = mProjProjections.value( reinterpret_cast< uintptr_t>( mProjContext.get() ) ).second;
+    mProjLock.unlock();
+    return dest;
+  }
+  mProjLock.unlock();
+
+  initializeCurrentContext();
+  return destProjection();
+}
+
 QString QgsCoordinateTransformPrivate::stripDatumTransform( const QString &proj4 ) const
 {
   QStringList parameterSplit = proj4.split( '+', QString::SkipEmptyParts );
@@ -309,4 +344,18 @@ void QgsCoordinateTransformPrivate::setFinder()
 #endif
 }
 
+void QgsCoordinateTransformPrivate::freeProj()
+{
+  mProjLock.lockForWrite();
+  QMap < uintptr_t, QPair< projPJ, projPJ > >::const_iterator it = mProjProjections.constBegin();
+  for ( ; it != mProjProjections.constEnd(); ++it )
+  {
+    pj_free( it.value().first );
+    pj_free( it.value().second );
+  }
+  mProjProjections.clear();
+  mProjLock.unlock();
+}
+
 ///@endcond
+
diff --git a/src/core/qgscoordinatetransform_p.h b/src/core/qgscoordinatetransform_p.h
@@ -32,6 +32,25 @@
 #include "qgscoordinatereferencesystem.h"
 
 typedef void *projPJ;
+typedef void *projCtx;
+
+/**
+ * \class QgsProjContextStore
+ * \ingroup core
+ * Used to create and store a proj projCtx object, correctly freeing the context upon destruction.
+ */
+class QgsProjContextStore
+{
+  public:
+
+    QgsProjContextStore();
+    ~QgsProjContextStore();
+
+    projCtx get() { return context; }
+
+  private:
+    projCtx context;
+};
 
 class QgsCoordinateTransformPrivate : public QSharedData
 {
@@ -48,6 +67,7 @@ class QgsCoordinateTransformPrivate : public QSharedData
     ~QgsCoordinateTransformPrivate();
 
     bool initialize();
+    void initializeCurrentContext();
 
     //! Flag to indicate whether the transform is valid (ie has a valid
     //! source and destination crs)
@@ -65,15 +85,23 @@ class QgsCoordinateTransformPrivate : public QSharedData
     //! QgsCoordinateReferenceSystem of the destination (map canvas) coordinate system
     QgsCoordinateReferenceSystem mDestCRS;
 
-    //! Proj4 data structure of the source projection (layer coordinate system)
-    projPJ mSourceProjection;
-
-    //! Proj4 data structure of the destination projection (map canvas coordinate system)
-    projPJ mDestinationProjection;
+    QString mSourceProjString;
+    QString mDestProjString;
+    projPJ sourceProjection();
+    projPJ destProjection();
 
     int mSourceDatumTransform;
     int mDestinationDatumTransform;
 
+    /**
+     * Thread local proj context storage. A new proj context will be created
+     * for every thread.
+     */
+    static thread_local QgsProjContextStore mProjContext;
+
+    QReadWriteLock mProjLock;
+    QMap < uintptr_t, QPair< projPJ, projPJ > > mProjProjections;
+
     static QString datumTransformString( int datumTransform );
 
   private:
@@ -85,6 +113,8 @@ class QgsCoordinateTransformPrivate : public QSharedData
     void addNullGridShifts( QString &srcProjString, QString &destProjString ) const;
 
     void setFinder();
+
+    void freeProj();
 };
 
 /// @endcond