Fix a deadlock with local COPC files in 2D rendering

Follow up of qgis#56388

It was possible to trigger a deadlock when using a local COPC file
by zooming in/out while there was 2D rendering happening - this caused
a situation where point cloud index was trying to recursively lock
a mutex that the same thread has already locked.

fetchNodeHierarchy() for local COPC files now unlocks the mutex before
calling fetchHierarchyPage() just like what happens with remote COPC files.

While fixing that, I have removed now duplicate code between the local and
remote COPC support. The logic for dealing with hierarchy mutex should
be clearer now as well - we do not call other functions of the index
with mutex locked.

(cherry picked from commit 13446ee)

src/core/pointcloud/qgscopcpointcloudindex.cpp

@@ -280,10 +280,12 @@ bool QgsCopcPointCloudIndex::fetchNodeHierarchy( const IndexedPointCloudNode &n
     if ( nodesCount < 0 )
     {
       auto hierarchyNodePos = mHierarchyNodePos.constFind( n );
+      mHierarchyMutex.unlock();
       fetchHierarchyPage( hierarchyNodePos->first, hierarchyNodePos->second );
+      mHierarchyMutex.lock();
     }
   }
-  return true;
+  return mHierarchy.contains( n );
 }
 
 void QgsCopcPointCloudIndex::fetchHierarchyPage( uint64_t offset, uint64_t byteSize ) const
@@ -292,6 +294,11 @@ void QgsCopcPointCloudIndex::fetchHierarchyPage( uint64_t offset, uint64_t byteS
   std::unique_ptr<char []> data( new char[ byteSize ] );
   mCopcFile.read( data.get(), byteSize );
 
+  populateHierarchy( data.get(), byteSize );
+}
+
+void QgsCopcPointCloudIndex::populateHierarchy( const char *hierarchyPageData, uint64_t byteSize ) const
+{
   struct CopcVoxelKey
   {
     int32_t level;
@@ -312,7 +319,7 @@ void QgsCopcPointCloudIndex::fetchHierarchyPage( uint64_t offset, uint64_t byteS
 
   for ( uint64_t i = 0; i < byteSize; i += sizeof( CopcEntry ) )
   {
-    CopcEntry *entry = reinterpret_cast<CopcEntry *>( data.get() + i );
+    const CopcEntry *entry = reinterpret_cast<const CopcEntry *>( hierarchyPageData + i );
     const IndexedPointCloudNode nodeId( entry->key.level, entry->key.x, entry->key.y, entry->key.z );
     mHierarchy[nodeId] = entry->pointCount;
     mHierarchyNodePos.insert( nodeId, QPair<uint64_t, int32_t>( entry->offset, entry->byteSize ) );
@@ -321,13 +328,7 @@ void QgsCopcPointCloudIndex::fetchHierarchyPage( uint64_t offset, uint64_t byteS
 
 bool QgsCopcPointCloudIndex::hasNode( const IndexedPointCloudNode &n ) const
 {
-  fetchNodeHierarchy( n );
-  mHierarchyMutex.lock();
-
-  auto it = mHierarchy.constFind( n );
-  const bool found = it != mHierarchy.constEnd() && ( *it ) >= 0;
-  mHierarchyMutex.unlock();
-  return found;
+  return fetchNodeHierarchy( n );
 }
 
 QList<IndexedPointCloudNode> QgsCopcPointCloudIndex::nodeChildren( const IndexedPointCloudNode &n ) const

src/core/pointcloud/qgscopcpointcloudindex.h

@@ -93,14 +93,15 @@ class CORE_EXPORT QgsCopcPointCloudIndex: public QgsPointCloudIndex
     bool loadHierarchy();
 
     //! Fetches all nodes leading to node \a node into memory
-    virtual bool fetchNodeHierarchy( const IndexedPointCloudNode &n ) const;
+    bool fetchNodeHierarchy( const IndexedPointCloudNode &n ) const;
 
     /**
      * Fetches the COPC hierarchy page at offset \a offset and of size \a byteSize into memory
-     * \note: This function is NOT thread safe and the mutex mHierarchyMutex needs to be locked before entering
      */
     virtual void fetchHierarchyPage( uint64_t offset, uint64_t byteSize ) const;
 
+    void populateHierarchy( const char *hierarchyPageData, uint64_t byteSize ) const;
+
     QByteArray fetchCopcStatisticsEvlrData();
 
     bool mIsValid = false;

src/core/pointcloud/qgsremotecopcpointcloudindex.cpp

@@ -51,30 +51,6 @@ std::unique_ptr<QgsPointCloudIndex> QgsRemoteCopcPointCloudIndex::clone() const
   return std::unique_ptr<QgsPointCloudIndex>( clone );
 }
 
-QList<IndexedPointCloudNode> QgsRemoteCopcPointCloudIndex::nodeChildren( const IndexedPointCloudNode &n ) const
-{
-  fetchNodeHierarchy( n );
-
-  mHierarchyMutex.lock();
-  Q_ASSERT( mHierarchy.contains( n ) );
-  QList<IndexedPointCloudNode> lst;
-  lst.reserve( 8 );
-  const int d = n.d() + 1;
-  const int x = n.x() * 2;
-  const int y = n.y() * 2;
-  const int z = n.z() * 2;
-  mHierarchyMutex.unlock();
-
-  for ( int i = 0; i < 8; ++i )
-  {
-    int dx = i & 1, dy = !!( i & 2 ), dz = !!( i & 4 );
-    const IndexedPointCloudNode n2( d, x + dx, y + dy, z + dz );
-    if ( fetchNodeHierarchy( n2 ) && mHierarchy[n] >= 0 )
-      lst.append( n2 );
-  }
-  return lst;
-}
-
 void QgsRemoteCopcPointCloudIndex::load( const QString &url )
 {
   mUrl = QUrl( url );
@@ -134,40 +110,6 @@ QgsPointCloudBlockRequest *QgsRemoteCopcPointCloudIndex::asyncNodeData( const In
          blockOffset, blockSize, pointCount, *mLazInfo.get() );
 }
 
-bool QgsRemoteCopcPointCloudIndex::hasNode( const IndexedPointCloudNode &n ) const
-{
-  return fetchNodeHierarchy( n );
-}
-
-bool QgsRemoteCopcPointCloudIndex::fetchNodeHierarchy( const IndexedPointCloudNode &n ) const
-{
-  QMutexLocker locker( &mHierarchyMutex );
-
-  QVector<IndexedPointCloudNode> ancestors;
-  IndexedPointCloudNode foundRoot = n;
-  while ( !mHierarchy.contains( foundRoot ) )
-  {
-    ancestors.push_front( foundRoot );
-    foundRoot = foundRoot.parentNode();
-  }
-  ancestors.push_front( foundRoot );
-  for ( IndexedPointCloudNode n : ancestors )
-  {
-    auto hierarchyIt = mHierarchy.constFind( n );
-    if ( hierarchyIt == mHierarchy.constEnd() )
-      return false;
-
-    int nodesCount = *hierarchyIt;
-    if ( nodesCount < 0 )
-    {
-      auto hierarchyNodePos = mHierarchyNodePos.constFind( n );
-      locker.unlock();
-      fetchHierarchyPage( hierarchyNodePos->first, hierarchyNodePos->second );
-    }
-  }
-  return mHierarchy.contains( n );
-}
-
 bool QgsRemoteCopcPointCloudIndex::isValid() const
 {
   return mIsValid;
@@ -198,31 +140,7 @@ void QgsRemoteCopcPointCloudIndex::fetchHierarchyPage( uint64_t offset, uint64_t
 
   QByteArray data = reply->data();
 
-  struct CopcVoxelKey
-  {
-    int32_t level;
-    int32_t x;
-    int32_t y;
-    int32_t z;
-  };
-
-  struct CopcEntry
-  {
-    CopcVoxelKey key;
-    uint64_t offset;
-    int32_t byteSize;
-    int32_t pointCount;
-  };
-
-  QMutexLocker locker( &mHierarchyMutex );
-
-  for ( uint64_t i = 0; i < byteSize; i += sizeof( CopcEntry ) )
-  {
-    CopcEntry *entry = reinterpret_cast<CopcEntry *>( data.data() + i );
-    const IndexedPointCloudNode nodeId( entry->key.level, entry->key.x, entry->key.y, entry->key.z );
-    mHierarchy[nodeId] = entry->pointCount;
-    mHierarchyNodePos.insert( nodeId, QPair<uint64_t, int32_t>( entry->offset, entry->byteSize ) );
-  }
+  populateHierarchy( data.constData(), byteSize );
 }
 
 void QgsRemoteCopcPointCloudIndex::copyCommonProperties( QgsRemoteCopcPointCloudIndex *destination ) const

src/core/pointcloud/qgsremotecopcpointcloudindex.h

@@ -54,15 +54,11 @@ class CORE_EXPORT QgsRemoteCopcPointCloudIndex: public QgsCopcPointCloudIndex
 
     std::unique_ptr<QgsPointCloudIndex> clone() const override;
 
-    QList<IndexedPointCloudNode> nodeChildren( const IndexedPointCloudNode &n ) const override;
-
     void load( const QString &url ) override;
 
     std::unique_ptr<QgsPointCloudBlock> nodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) override;
     QgsPointCloudBlockRequest *asyncNodeData( const IndexedPointCloudNode &n, const QgsPointCloudRequest &request ) override;
 
-    bool hasNode( const IndexedPointCloudNode &n ) const override;
-
     bool isValid() const override;
 
     QgsPointCloudIndex::AccessType accessType() const override { return QgsPointCloudIndex::Remote; }
@@ -74,7 +70,6 @@ class CORE_EXPORT QgsRemoteCopcPointCloudIndex: public QgsCopcPointCloudIndex
     void copyCommonProperties( QgsRemoteCopcPointCloudIndex *destination ) const;
 
   protected:
-    virtual bool fetchNodeHierarchy( const IndexedPointCloudNode &nodeId ) const override;
     virtual void fetchHierarchyPage( uint64_t offset, uint64_t byteSize ) const override;
 
     QUrl mUrl;