Fix crashes caused by concurrent rendering of cached QPictures from Q…

…gsSvgCache QgsSvgCache::svgAsPicture was rendering an implicitly shared copy when the picture had already been cached. It turns out that rendering an implicitly shared QPicture copy isn't thread safe, and rendering shared copies simulataneously across different threads leads quickly to a crash. Accordingly we always detach the QPicture objects returned by svgAsPicture, so that the returned QPicture is safe to use across threads. Also add unit tests for this, and a similar unit test to verify that rendering of QImage based cached copies does *not* suffer the same issue. Fixes #17089, #17077
qgis · Oct 31, 2017 · a6eea72 · a6eea72 · rldhont · Jun 3, 2018
1 parent 942f431
commit a6eea72
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 13 deletions.
diff --git a/src/app/composer/qgscomposerpicturewidget.cpp b/src/app/composer/qgscomposerpicturewidget.cpp
@@ -448,7 +448,7 @@ QIcon QgsComposerPictureWidget::svgToIcon( const QString &filePath ) const
     strokeWidth = 0.6;
 
   bool fitsInCache; // should always fit in cache at these sizes (i.e. under 559 px ^ 2, or half cache size)
-  const QImage &img = QgsApplication::svgCache()->svgAsImage( filePath, 30.0, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
+  QImage img = QgsApplication::svgCache()->svgAsImage( filePath, 30.0, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
 
   return QIcon( QPixmap::fromImage( img ) );
 }

diff --git a/src/core/symbology/qgsfillsymbollayer.cpp b/src/core/symbology/qgsfillsymbollayer.cpp
@@ -1930,12 +1930,12 @@ void QgsSVGFillSymbolLayer::applyPattern( QBrush &brush, const QString &svgFileP
   {
     bool fitsInCache = true;
     double strokeWidth = context.renderContext().convertToPainterUnits( svgStrokeWidth, svgStrokeWidthUnit, svgStrokeWidthMapUnitScale );
-    const QImage &patternImage = QgsApplication::svgCache()->svgAsImage( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
-                                 context.renderContext().scaleFactor(), fitsInCache );
+    QImage patternImage = QgsApplication::svgCache()->svgAsImage( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
+                          context.renderContext().scaleFactor(), fitsInCache );
     if ( !fitsInCache )
     {
-      const QPicture &patternPict = QgsApplication::svgCache()->svgAsPicture( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
-                                    context.renderContext().scaleFactor() );
+      QPicture patternPict = QgsApplication::svgCache()->svgAsPicture( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
+                             context.renderContext().scaleFactor() );
       double hwRatio = 1.0;
       if ( patternPict.width() > 0 )
       {

diff --git a/src/core/symbology/qgsmarkersymbollayer.cpp b/src/core/symbology/qgsmarkersymbollayer.cpp
@@ -2009,8 +2009,8 @@ void QgsSvgMarkerSymbolLayer::renderPoint( QPointF point, QgsSymbolRenderContext
   if ( !context.renderContext().forceVectorOutput() && !rotated )
   {
     usePict = false;
-    const QImage &img = QgsApplication::svgCache()->svgAsImage( path, size, fillColor, strokeColor, strokeWidth,
-                        context.renderContext().scaleFactor(), fitsInCache, aspectRatio );
+    QImage img = QgsApplication::svgCache()->svgAsImage( path, size, fillColor, strokeColor, strokeWidth,
+                 context.renderContext().scaleFactor(), fitsInCache, aspectRatio );
     if ( fitsInCache && img.width() > 1 )
     {
       //consider transparency
@@ -2032,9 +2032,8 @@ void QgsSvgMarkerSymbolLayer::renderPoint( QPointF point, QgsSymbolRenderContext
   if ( usePict || !fitsInCache )
   {
     p->setOpacity( context.opacity() );
-    const QPicture &pct = QgsApplication::svgCache()->svgAsPicture( path, size, fillColor, strokeColor, strokeWidth,
-                          context.renderContext().scaleFactor(), context.renderContext().forceVectorOutput(), aspectRatio );
-
+    QPicture pct = QgsApplication::svgCache()->svgAsPicture( path, size, fillColor, strokeColor, strokeWidth,
+                   context.renderContext().scaleFactor(), context.renderContext().forceVectorOutput(), aspectRatio );
     if ( pct.width() > 1 )
     {
       p->save();

diff --git a/src/core/symbology/qgssvgcache.cpp b/src/core/symbology/qgssvgcache.cpp
@@ -170,7 +170,9 @@ QPicture QgsSvgCache::svgAsPicture( const QString &path, double size, const QCol
     trimToMaximumSize();
   }
 
-  return *( currentEntry->picture );
+  QPicture p = *( currentEntry->picture );
+  p.detach();
+  return p;
 }
 
 QByteArray QgsSvgCache::svgContent( const QString &path, double size, const QColor &fill, const QColor &stroke, double strokeWidth,

diff --git a/src/gui/symbology/qgssvgselectorwidget.cpp b/src/gui/symbology/qgssvgselectorwidget.cpp
@@ -262,7 +262,7 @@ QPixmap QgsSvgSelectorListModel::createPreview( const QString &entry ) const
     strokeWidth = 0.2;
 
   bool fitsInCache; // should always fit in cache at these sizes (i.e. under 559 px ^ 2, or half cache size)
-  const QImage &img = QgsApplication::svgCache()->svgAsImage( entry, mIconSize, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
+  QImage img = QgsApplication::svgCache()->svgAsImage( entry, mIconSize, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
   return QPixmap::fromImage( img );
 }
 

diff --git a/tests/src/core/testqgssvgcache.cpp b/tests/src/core/testqgssvgcache.cpp
@@ -20,7 +20,9 @@
 #include <QFileInfo>
 #include <QDir>
 #include <QDesktopServices>
-
+#include <QPicture>
+#include <QPainter>
+#include <QtConcurrent>
 #include "qgssvgcache.h"
 
 /**
@@ -40,6 +42,8 @@ class TestQgsSvgCache : public QObject
     void init() {} // will be called before each testfunction is executed.
     void cleanup() {} // will be called after every testfunction.
     void fillCache();
+    void threadSafePicture();
+    void threadSafeImage();
 
 };
 
@@ -77,6 +81,79 @@ void TestQgsSvgCache::fillCache()
   }
 }
 
+struct RenderPictureWrapper
+{
+  QgsSvgCache &cache;
+  QString svgPath;
+  double size = 100;
+  explicit RenderPictureWrapper( QgsSvgCache &cache, const QString &svgPath )
+    : cache( cache )
+    , svgPath( svgPath )
+  {}
+  void operator()( int )
+  {
+    QPicture pic = cache.svgAsPicture( svgPath, size, QColor( 255, 0, 0 ), QColor( 0, 255, 0 ), 1, 1, true );
+    QSize imageSize = pic.boundingRect().size();
+    QImage image( imageSize, QImage::Format_ARGB32_Premultiplied );
+    image.fill( 0 ); // transparent background
+    QPainter p( &image );
+    p.drawPicture( 0, 0, pic );
+  }
+};
+
+void TestQgsSvgCache::threadSafePicture()
+{
+  // QPicture playback is NOT thread safe with implicitly shared copies - this
+  // unit test checks that concurrent drawing of svg as QPicture from QgsSvgCache
+  // returns a detached copy which is safe to use across threads
+
+  // refs:
+  // https://issues.qgis.org/issues/17077
+  // https://issues.qgis.org/issues/17089
+
+  QgsSvgCache cache;
+  QString svgPath = TEST_DATA_DIR + QStringLiteral( "/sample_svg.svg" );
+
+  // smash picture rendering over multiple threads
+  QVector< int > list;
+  list.resize( 100 );
+  QtConcurrent::blockingMap( list, RenderPictureWrapper( cache, svgPath ) );
+}
+
+
+struct RenderImageWrapper
+{
+  QgsSvgCache &cache;
+  QString svgPath;
+  double size = 100;
+  explicit RenderImageWrapper( QgsSvgCache &cache, const QString &svgPath )
+    : cache( cache )
+    , svgPath( svgPath )
+  {}
+  void operator()( int )
+  {
+    bool fitsInCache = false;
+    QImage cachedImage = cache.svgAsImage( svgPath, size, QColor( 255, 0, 0 ), QColor( 0, 255, 0 ), 1, 1, fitsInCache );
+    QImage image( cachedImage.size(), QImage::Format_ARGB32_Premultiplied );
+    image.fill( 0 ); // transparent background
+    QPainter p( &image );
+    p.drawImage( 0, 0, cachedImage );
+  }
+};
+
+void TestQgsSvgCache::threadSafeImage()
+{
+  // This unit test checks that concurrent rendering of svg as QImage from QgsSvgCache
+  // works without issues across threads
+
+  QgsSvgCache cache;
+  QString svgPath = TEST_DATA_DIR + QStringLiteral( "/sample_svg.svg" );
+
+  // smash image rendering over multiple threads
+  QVector< int > list;
+  list.resize( 100 );
+  QtConcurrent::blockingMap( list, RenderImageWrapper( cache, svgPath ) );
+}
 
 QGSTEST_MAIN( TestQgsSvgCache )
 #include "testqgssvgcache.moc"