Merge pull request #5497 from nyalldawson/svg_crash

Fix crashes and issues with SVG rendering

doc/api_break.dox

@@ -319,6 +319,7 @@ should now call QgsCoordinateReferenceSystem::invalidateCache() and QgsCoordinat
 - QgsEditorWidgetConfig was removed. Use QVariantMap instead.
 - QgsScaleExpression. Use QgsProperty with a QgsSizeScalePropertyTransformer instead.
 - QgsSvgAnnotationItem. Use QgsSvgAnnotation instead.
+- QgsSvgCacheEntry. This is an internal class and is no longer exposed to public API.
 - QgsSymbologyV2Conversion was removed. Reading of renderers from pre-1.0 versions is not supported anymore.
 - QgsTextAnnotationItem. Use QgsTextAnnotation instead.
 - QgsTransectSample. This class was unused and unmaintained.
@@ -2317,6 +2318,8 @@ QgsSvgCache        {#qgis_api_break_3_0_QgsSvgCache}
 - containsParamsV2() was removed. Use containsParamsV3() instead.
 - The rasterScaleFactor parameter was removed from all methods
 - svgAsImage(), svgAsPicture(), svgViewboxSize(), svgContent(), insertSvg(), cacheEntry() only accept absolute path to SVG file (relative paths will not be resolved).
+- The protected member insertSvg() was made private. QgsSvgCache is not intended to be subclassed.
+
 
 QgsSvgCacheEntry       {#qgis_api_break_3_0_QgsSvgCacheEntry}
 ----------------

python/core/symbology/qgssvgcache.sip

@@ -11,69 +11,6 @@
 
 
 
-class QgsSvgCacheEntry
-{
-
-%TypeHeaderCode
-#include "qgssvgcache.h"
-%End
-  public:
-
-    QgsSvgCacheEntry();
-
-    QgsSvgCacheEntry( const QString &path, double size, double strokeWidth, double widthScaleFactor, const QColor &fill, const QColor &stroke,
-                      double fixedAspectRatio = 0 ) ;
-%Docstring
- Constructor.
- \param path Absolute path to SVG file (relative paths are not resolved).
- \param size
- \param strokeWidth width of stroke
- \param widthScaleFactor width scale factor
- \param fill color of fill
- \param stroke color of stroke
- \param fixedAspectRatio fixed aspect ratio (optional)
-%End
-    ~QgsSvgCacheEntry();
-
-
-    QString path;
-%Docstring
-Absolute path to SVG file
-%End
-    double size;
-    double strokeWidth;
-    double widthScaleFactor;
-
-    double fixedAspectRatio;
-%Docstring
-Fixed aspect ratio
-%End
-
-    QSizeF viewboxSize;
-%Docstring
- SVG viewbox size.
-.. versionadded:: 2.14
-%End
-
-    QColor fill;
-    QColor stroke;
-    QImage *image;
-    QPicture *picture;
-    QByteArray svgContent;
-
-    QgsSvgCacheEntry *nextEntry;
-    QgsSvgCacheEntry *previousEntry;
-
-    bool operator==( const QgsSvgCacheEntry &other ) const;
-    int dataSize() const;
-%Docstring
-Return memory usage in bytes
- :rtype: int
-%End
-
-  private:
-    QgsSvgCacheEntry( const QgsSvgCacheEntry &rh );
-};
 
 class QgsSvgCache : QObject
 {
@@ -200,39 +137,6 @@ Get SVG content
 Emit a signal to be caught by qgisapp and display a msg on status bar
 %End
 
-  protected:
-
-    QgsSvgCacheEntry *insertSvg( const QString &path, double size, const QColor &fill, const QColor &stroke, double strokeWidth,
-                                 double widthScaleFactor, double fixedAspectRatio = 0 );
-%Docstring
- Creates new cache entry and returns pointer to it
- \param path Absolute path to SVG file
- \param size size of cached image
- \param fill color of fill
- \param stroke color of stroke
- \param strokeWidth width of stroke
- \param widthScaleFactor width scale factor
- \param fixedAspectRatio fixed aspect ratio (optional)
- :rtype: QgsSvgCacheEntry
-%End
-
-    void replaceParamsAndCacheSvg( QgsSvgCacheEntry *entry );
-    void cacheImage( QgsSvgCacheEntry *entry );
-    void cachePicture( QgsSvgCacheEntry *entry, bool forceVectorOutput = false );
-    QgsSvgCacheEntry *cacheEntry( const QString &path, double size, const QColor &fill, const QColor &stroke, double strokeWidth,
-                                  double widthScaleFactor, double fixedAspectRatio = 0 );
-%Docstring
-Returns entry from cache or creates a new entry if it does not exist already
- :rtype: QgsSvgCacheEntry
-%End
-
-    void trimToMaximumSize();
-%Docstring
-Removes the least used items until the maximum size is under the limit
-%End
-
-    void takeEntryFromList( QgsSvgCacheEntry *entry );
-
 };
 
 /************************************************************************

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 ) );
 }

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 )
       {

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();

src/core/symbology/qgssvgcache.cpp

@@ -36,15 +36,11 @@
 #include <QNetworkReply>
 #include <QNetworkRequest>
 
-QgsSvgCacheEntry::QgsSvgCacheEntry()
-  : path( QString() )
-  , fill( Qt::black )
-  , stroke( Qt::black )
-{
-}
+///@cond PRIVATE
 
 QgsSvgCacheEntry::QgsSvgCacheEntry( const QString &p, double s, double ow, double wsf, const QColor &fi, const QColor &ou, double far )
   : path( p )
+  , fileModified( QFileInfo( p ).lastModified() )
   , size( s )
   , strokeWidth( ow )
   , widthScaleFactor( wsf )
@@ -52,19 +48,18 @@ QgsSvgCacheEntry::QgsSvgCacheEntry( const QString &p, double s, double ow, doubl
   , fill( fi )
   , stroke( ou )
 {
-}
-
-
-QgsSvgCacheEntry::~QgsSvgCacheEntry()
-{
-  delete image;
-  delete picture;
+  fileModifiedLastCheckTimer.start();
 }
 
 bool QgsSvgCacheEntry::operator==( const QgsSvgCacheEntry &other ) const
 {
-  return other.path == path && qgsDoubleNear( other.size, size ) && qgsDoubleNear( other.strokeWidth, strokeWidth ) && qgsDoubleNear( other.widthScaleFactor, widthScaleFactor )
-         && other.fill == fill && other.stroke == stroke;
+  bool equal = other.path == path && qgsDoubleNear( other.size, size ) && qgsDoubleNear( other.strokeWidth, strokeWidth ) && qgsDoubleNear( other.widthScaleFactor, widthScaleFactor )
+               && other.fixedAspectRatio == fixedAspectRatio && other.fill == fill && other.stroke == stroke;
+
+  if ( equal && ( mFileModifiedCheckTimeout <= 0 || fileModifiedLastCheckTimer.hasExpired( mFileModifiedCheckTimeout ) ) )
+    equal = other.fileModified == fileModified;
+
+  return equal;
 }
 
 int QgsSvgCacheEntry::dataSize() const
@@ -80,6 +75,8 @@ int QgsSvgCacheEntry::dataSize() const
   }
   return size;
 }
+///@endcond
+
 
 QgsSvgCache::QgsSvgCache( QObject *parent )
   : QObject( parent )
@@ -101,6 +98,8 @@ QImage QgsSvgCache::svgAsImage( const QString &file, double size, const QColor &
   fitsInCache = true;
   QgsSvgCacheEntry *currentEntry = cacheEntry( file, size, fill, stroke, strokeWidth, widthScaleFactor, fixedAspectRatio );
 
+  QImage result;
+
   //if current entry image is 0: cache image for entry
   // checks to see if image will fit into cache
   //update stats for memory usage
@@ -125,24 +124,30 @@ QImage QgsSvgCache::svgAsImage( const QString &file, double size, const QColor &
     if ( cachedDataSize > MAXIMUM_SIZE / 2 )
     {
       fitsInCache = false;
-      delete currentEntry->image;
-      currentEntry->image = nullptr;
-      //currentEntry->image = new QImage( 0, 0 );
+      currentEntry->image.reset();
 
       // instead cache picture
       if ( !currentEntry->picture )
       {
         cachePicture( currentEntry, false );
       }
+
+      // ...and render cached picture to result image
+      result = imageFromCachedPicture( *currentEntry );
     }
     else
     {
       cacheImage( currentEntry );
+      result = *( currentEntry->image );
     }
     trimToMaximumSize();
   }
+  else
+  {
+    result = *( currentEntry->image );
+  }
 
-  return *( currentEntry->image );
+  return result;
 }
 
 QPicture QgsSvgCache::svgAsPicture( const QString &path, double size, const QColor &fill, const QColor &stroke, double strokeWidth,
@@ -160,7 +165,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,
@@ -186,6 +193,7 @@ QgsSvgCacheEntry *QgsSvgCache::insertSvg( const QString &path, double size, cons
     double widthScaleFactor, double fixedAspectRatio )
 {
   QgsSvgCacheEntry *entry = new QgsSvgCacheEntry( path, size, strokeWidth, widthScaleFactor, fill, stroke, fixedAspectRatio );
+  entry->mFileModifiedCheckTimeout = mFileModifiedCheckTimeout;
 
   replaceParamsAndCacheSvg( entry );
 
@@ -476,55 +484,32 @@ void QgsSvgCache::cacheImage( QgsSvgCacheEntry *entry )
     return;
   }
 
-  delete entry->image;
-  entry->image = nullptr;
+  entry->image.reset();
 
-  bool isFixedAR = entry->fixedAspectRatio > 0;
+  QSizeF viewBoxSize;
+  QSizeF scaledSize;
+  QSize imageSize = sizeForImage( *entry, viewBoxSize, scaledSize );
 
-  QSvgRenderer r( entry->svgContent );
-  double hwRatio = 1.0;
-  if ( r.viewBoxF().width() > 0 )
-  {
-    if ( isFixedAR )
-    {
-      hwRatio = entry->fixedAspectRatio;
-    }
-    else
-    {
-      hwRatio = r.viewBoxF().height() / r.viewBoxF().width();
-    }
-  }
-  double wSize = entry->size;
-  int wImgSize = static_cast< int >( wSize );
-  if ( wImgSize < 1 )
-  {
-    wImgSize = 1;
-  }
-  double hSize = wSize * hwRatio;
-  int hImgSize = static_cast< int >( hSize );
-  if ( hImgSize < 1 )
-  {
-    hImgSize = 1;
-  }
   // cast double image sizes to int for QImage
-  QImage *image = new QImage( wImgSize, hImgSize, QImage::Format_ARGB32_Premultiplied );
+  std::unique_ptr< QImage > image = qgis::make_unique< QImage >( imageSize, QImage::Format_ARGB32_Premultiplied );
   image->fill( 0 ); // transparent background
 
-  QPainter p( image );
-  if ( qgsDoubleNear( r.viewBoxF().width(), r.viewBoxF().height() ) )
+  QPainter p( image.get() );
+  QSvgRenderer r( entry->svgContent );
+  if ( qgsDoubleNear( viewBoxSize.width(), viewBoxSize.height() ) )
   {
     r.render( &p );
   }
   else
   {
-    QSizeF s( r.viewBoxF().size() );
-    s.scale( wSize, hSize, Qt::KeepAspectRatio );
-    QRectF rect( ( wImgSize - s.width() ) / 2, ( hImgSize - s.height() ) / 2, s.width(), s.height() );
+    QSizeF s( viewBoxSize );
+    s.scale( scaledSize.width(), scaledSize.height(), Qt::KeepAspectRatio );
+    QRectF rect( ( imageSize.width() - s.width() ) / 2, ( imageSize.height() - s.height() ) / 2, s.width(), s.height() );
     r.render( &p, rect );
   }
 
-  entry->image = image;
   mTotalSize += ( image->width() * image->height() * 32 );
+  entry->image = std::move( image );
 }
 
 void QgsSvgCache::cachePicture( QgsSvgCacheEntry *entry, bool forceVectorOutput )
@@ -535,13 +520,12 @@ void QgsSvgCache::cachePicture( QgsSvgCacheEntry *entry, bool forceVectorOutput
     return;
   }
 
-  delete entry->picture;
-  entry->picture = nullptr;
+  entry->picture.reset();
 
   bool isFixedAR = entry->fixedAspectRatio > 0;
 
   //correct QPictures dpi correction
-  QPicture *picture = new QPicture();
+  std::unique_ptr< QPicture > picture = qgis::make_unique< QPicture >();
   QRectF rect;
   QSvgRenderer r( entry->svgContent );
   double hwRatio = 1.0;
@@ -564,9 +548,9 @@ void QgsSvgCache::cachePicture( QgsSvgCacheEntry *entry, bool forceVectorOutput
   s.scale( wSize, hSize, isFixedAR ? Qt::IgnoreAspectRatio : Qt::KeepAspectRatio );
   rect = QRectF( -s.width() / 2.0, -s.height() / 2.0, s.width(), s.height() );
 
-  QPainter p( picture );
+  QPainter p( picture.get() );
   r.render( &p, rect );
-  entry->picture = picture;
+  entry->picture = std::move( picture );
   mTotalSize += entry->picture->size();
 }
 
@@ -576,7 +560,7 @@ QgsSvgCacheEntry *QgsSvgCache::cacheEntry( const QString &path, double size, con
   //search entries in mEntryLookup
   QgsSvgCacheEntry *currentEntry = nullptr;
   QList<QgsSvgCacheEntry *> entries = mEntryLookup.values( path );
-
+  QDateTime modified;
   QList<QgsSvgCacheEntry *>::iterator entryIt = entries.begin();
   for ( ; entryIt != entries.end(); ++entryIt )
   {
@@ -585,6 +569,14 @@ QgsSvgCacheEntry *QgsSvgCache::cacheEntry( const QString &path, double size, con
          qgsDoubleNear( cacheEntry->strokeWidth, strokeWidth ) && qgsDoubleNear( cacheEntry->widthScaleFactor, widthScaleFactor ) &&
          qgsDoubleNear( cacheEntry->fixedAspectRatio, fixedAspectRatio ) )
     {
+      if ( mFileModifiedCheckTimeout <= 0 || cacheEntry->fileModifiedLastCheckTimer.hasExpired( mFileModifiedCheckTimeout ) )
+      {
+        if ( !modified.isValid() )
+          modified = QFileInfo( path ).lastModified();
+
+        if ( cacheEntry->fileModified != modified )
+          continue;
+      }
       currentEntry = cacheEntry;
       break;
     }
@@ -906,6 +898,53 @@ void QgsSvgCache::printEntryList()
   }
 }
 
+QSize QgsSvgCache::sizeForImage( const QgsSvgCacheEntry &entry, QSizeF &viewBoxSize, QSizeF &scaledSize ) const
+{
+  bool isFixedAR = entry.fixedAspectRatio > 0;
+
+  QSvgRenderer r( entry.svgContent );
+  double hwRatio = 1.0;
+  viewBoxSize = r.viewBoxF().size();
+  if ( viewBoxSize.width() > 0 )
+  {
+    if ( isFixedAR )
+    {
+      hwRatio = entry.fixedAspectRatio;
+    }
+    else
+    {
+      hwRatio = viewBoxSize.height() / viewBoxSize.width();
+    }
+  }
+
+  // cast double image sizes to int for QImage
+  scaledSize.setWidth( entry.size );
+  int wImgSize = static_cast< int >( scaledSize.width() );
+  if ( wImgSize < 1 )
+  {
+    wImgSize = 1;
+  }
+  scaledSize.setHeight( scaledSize.width() * hwRatio );
+  int hImgSize = static_cast< int >( scaledSize.height() );
+  if ( hImgSize < 1 )
+  {
+    hImgSize = 1;
+  }
+  return QSize( wImgSize, hImgSize );
+}
+
+QImage QgsSvgCache::imageFromCachedPicture( const QgsSvgCacheEntry &entry ) const
+{
+  QSizeF viewBoxSize;
+  QSizeF scaledSize;
+  QImage image( sizeForImage( entry, viewBoxSize, scaledSize ), QImage::Format_ARGB32_Premultiplied );
+  image.fill( 0 ); // transparent background
+
+  QPainter p( &image );
+  p.drawPicture( QPoint( 0, 0 ), *entry.picture );
+  return image;
+}
+
 void QgsSvgCache::trimToMaximumSize()
 {
   //only one entry in cache

src/core/symbology/qgssvgcache.h

@@ -27,6 +27,8 @@
 #include <QUrl>
 #include <QObject>
 #include <QSizeF>
+#include <QDateTime>
+#include <QElapsedTimer>
 
 #include "qgis_core.h"
 
@@ -34,6 +36,10 @@ class QDomElement;
 class QImage;
 class QPicture;
 
+#ifndef SIP_RUN
+
+///@cond PRIVATE
+
 /**
  * \ingroup core
  * \class QgsSvgCacheEntry
@@ -42,7 +48,7 @@ class CORE_EXPORT QgsSvgCacheEntry
 {
   public:
 
-    QgsSvgCacheEntry();
+    QgsSvgCacheEntry() = delete;
 
     /**
      * Constructor.
@@ -56,7 +62,6 @@ class CORE_EXPORT QgsSvgCacheEntry
      */
     QgsSvgCacheEntry( const QString &path, double size, double strokeWidth, double widthScaleFactor, const QColor &fill, const QColor &stroke,
                       double fixedAspectRatio = 0 ) ;
-    ~QgsSvgCacheEntry();
 
     //! QgsSvgCacheEntry cannot be copied.
     QgsSvgCacheEntry( const QgsSvgCacheEntry &rh ) = delete;
@@ -65,6 +70,13 @@ class CORE_EXPORT QgsSvgCacheEntry
 
     //! Absolute path to SVG file
     QString path;
+
+    //! Timestamp when file was last modified
+    QDateTime fileModified;
+    //! Time since last check of file modified date
+    QElapsedTimer fileModifiedLastCheckTimer;
+    int mFileModifiedCheckTimeout = 30000;
+
     double size = 0.0; //size in pixels (cast to int for QImage)
     double strokeWidth = 0;
     double widthScaleFactor = 1.0;
@@ -78,10 +90,10 @@ class CORE_EXPORT QgsSvgCacheEntry
      */
     QSizeF viewboxSize;
 
-    QColor fill;
-    QColor stroke;
-    QImage *image = nullptr;
-    QPicture *picture = nullptr;
+    QColor fill = Qt::black;
+    QColor stroke = Qt::black;
+    std::unique_ptr< QImage > image;
+    std::unique_ptr< QPicture > picture;
     //content (with params replaced)
     QByteArray svgContent;
 
@@ -101,6 +113,9 @@ class CORE_EXPORT QgsSvgCacheEntry
 
 };
 
+///@endcond
+#endif
+
 /**
  * \ingroup core
  * A cache for images / pictures derived from svg files. This class supports parameter replacement in svg files
@@ -211,7 +226,10 @@ class CORE_EXPORT QgsSvgCache : public QObject
     //! Emit a signal to be caught by qgisapp and display a msg on status bar
     void statusChanged( const QString  &statusQString );
 
-  protected:
+  private slots:
+    void downloadProgress( qint64, qint64 );
+
+  private:
 
     /**
      * Creates new cache entry and returns pointer to it
@@ -239,10 +257,9 @@ class CORE_EXPORT QgsSvgCache : public QObject
     //Removes entry from the ordered list (but does not delete the entry itself)
     void takeEntryFromList( QgsSvgCacheEntry *entry );
 
-  private slots:
-    void downloadProgress( qint64, qint64 );
+    //! Minimum time (in ms) between consecutive svg file modified time checks
+    int mFileModifiedCheckTimeout = 30000;
 
-  private:
     //! Entry pointers accessible by file name
     QMultiHash< QString, QgsSvgCacheEntry * > mEntryLookup;
     //! Estimated total size of all images, pictures and svgContent
@@ -275,12 +292,24 @@ class CORE_EXPORT QgsSvgCache : public QObject
     //! For debugging
     void printEntryList();
 
+    /**
+     * Returns the target size (in pixels) and calculates the \a viewBoxSize
+     * for a cache \a entry.
+     */
+    QSize sizeForImage( const QgsSvgCacheEntry &entry, QSizeF &viewBoxSize, QSizeF &scaledSize ) const;
+
+    /**
+     * Returns a rendered image for a cached picture \a entry.
+     */
+    QImage imageFromCachedPicture( const QgsSvgCacheEntry &entry ) const;
+
     //! SVG content to be rendered if SVG file was not found.
     QByteArray mMissingSvg;
 
     //! Mutex to prevent concurrent access to the class from multiple threads at once (may corrupt the entries otherwise).
     QMutex mMutex;
 
+    friend class TestQgsSvgCache;
 };
 
 #endif // QGSSVGCACHE_H

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 );
 }
 

tests/src/core/CMakeLists.txt

@@ -180,6 +180,7 @@ SET(TESTS
  testqgsstatisticalsummary.cpp
  testqgsstringutils.cpp
  testqgsstyle.cpp
+ testqgssvgcache.cpp
  testqgssvgmarker.cpp
  testqgssymbol.cpp
  testqgstaskmanager.cpp

tests/src/core/testqgssvgcache.cpp

@@ -0,0 +1,259 @@
+/***************************************************************************
+     testqgssvgcache.cpp
+     --------------------
+    Date                 : October 2017
+    Copyright            : (C) 2017 by Nyall Dawson
+    Email                : nyall dot dawson at gmail dot com
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+#include "qgstest.h"
+#include <QObject>
+#include <QString>
+#include <QStringList>
+#include <QApplication>
+#include <QFileInfo>
+#include <QDir>
+#include <QDesktopServices>
+#include <QPicture>
+#include <QPainter>
+#include <QtConcurrent>
+#include <QElapsedTimer>
+#include "qgssvgcache.h"
+#include "qgsmultirenderchecker.h"
+#include "qgsapplication.h"
+
+/**
+ * \ingroup UnitTests
+ * This is a unit test for QgsSvgCache.
+ */
+class TestQgsSvgCache : public QObject
+{
+    Q_OBJECT
+
+  private:
+
+    QString mReport;
+
+    bool imageCheck( const QString &testName, QImage &image, int mismatchCount );
+
+  private slots:
+    void initTestCase();// will be called before the first testfunction is executed.
+    void cleanupTestCase();// will be called after the last testfunction was executed.
+    void init() {} // will be called before each testfunction is executed.
+    void cleanup() {} // will be called after every testfunction.
+    void fillCache();
+    void threadSafePicture();
+    void threadSafeImage();
+    void changeImage(); //check that cache is updated if svg source file changes
+
+};
+
+
+void TestQgsSvgCache::initTestCase()
+{
+  QgsApplication::init();
+  QgsApplication::initQgis();
+  mReport += "<h1>QgsSvgCache Tests</h1>\n";
+}
+
+void TestQgsSvgCache::cleanupTestCase()
+{
+  QgsApplication::exitQgis();
+
+  QString myReportFile = QDir::tempPath() + "/qgistest.html";
+  QFile myFile( myReportFile );
+  if ( myFile.open( QIODevice::WriteOnly | QIODevice::Append ) )
+  {
+    QTextStream myQTextStream( &myFile );
+    myQTextStream << mReport;
+    myFile.close();
+    //QDesktopServices::openUrl( "file:///" + myReportFile );
+  }
+}
+
+void TestQgsSvgCache::fillCache()
+{
+  QgsSvgCache cache;
+  // flood cache to fill it
+  QString svgPath = TEST_DATA_DIR + QStringLiteral( "/sample_svg.svg" );
+  bool fitInCache = false;
+
+  // we loop forever, continually increasing the size of the requested
+  // svg render. The continually changing image size should quickly fill
+  // the svg cache size, forcing use of non-cached images.
+  // We break after hitting a certain threshold of non-cached images,
+  // (after testing that the result is non-null, i.e. rendered on demand,
+  // not from cache).
+  int uncached = 0;
+  for ( double size = 1000; uncached < 10; size += 100 )
+  {
+    QImage image = cache.svgAsImage( svgPath, size, QColor( 255, 0, 0 ), QColor( 0, 255, 0 ), 1, 1, fitInCache );
+    QVERIFY( !image.isNull() );
+    if ( !fitInCache )
+      uncached++;
+  }
+}
+
+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 ) );
+}
+
+void TestQgsSvgCache::changeImage()
+{
+  bool inCache;
+  QgsSvgCache cache;
+  // no minimum time between checks
+  cache.mFileModifiedCheckTimeout = 0;
+
+  //copy an image to the temp folder
+  QString tempImagePath = QDir::tempPath() + "/svg_cache.svg";
+
+  QString originalImage = TEST_DATA_DIR + QStringLiteral( "/test_symbol_svg.svg" );
+  if ( QFileInfo::exists( tempImagePath ) )
+    QFile::remove( tempImagePath );
+  QFile::copy( originalImage, tempImagePath );
+
+  //render it through the cache
+  QImage img = cache.svgAsImage( tempImagePath, 200, QColor( 0, 0, 0 ), QColor( 0, 0, 0 ), 1.0,
+                                 1.0, inCache );
+  QVERIFY( imageCheck( "svgcache_changed_before", img, 30 ) );
+
+  // wait a second so that modified time is different
+  QElapsedTimer t;
+  t.start();
+  while ( !t.hasExpired( 1000 ) )
+  {}
+
+  //replace the image in the temp folder
+  QString newImage = TEST_DATA_DIR + QStringLiteral( "/test_symbol_svg2.svg" );
+  QFile::remove( tempImagePath );
+  QFile::copy( newImage, tempImagePath );
+
+  //re-render it
+  img = cache.svgAsImage( tempImagePath, 200, QColor( 0, 0, 0 ), QColor( 0, 0, 0 ), 1.0,
+                          1.0, inCache );
+  QVERIFY( imageCheck( "svgcache_changed_after", img, 30 ) );
+
+  // repeat, with minimum time between checks
+  QgsSvgCache cache2;
+  QFile::remove( tempImagePath );
+  QFile::copy( originalImage, tempImagePath );
+  img = cache2.svgAsImage( tempImagePath, 200, QColor( 0, 0, 0 ), QColor( 0, 0, 0 ), 1.0,
+                           1.0, inCache );
+  QVERIFY( imageCheck( "svgcache_changed_before", img, 30 ) );
+
+  // wait a second so that modified time is different
+  t.restart();
+  while ( !t.hasExpired( 1000 ) )
+  {}
+
+  //replace the image in the temp folder
+  QFile::remove( tempImagePath );
+  QFile::copy( newImage, tempImagePath );
+
+  //re-render it - not enough time has elapsed between checks, so file modification time will NOT be rechecked and
+  // existing cached image should be used
+  img = cache2.svgAsImage( tempImagePath, 200, QColor( 0, 0, 0 ), QColor( 0, 0, 0 ), 1.0,
+                           1.0, inCache );
+  QVERIFY( imageCheck( "svgcache_changed_before", img, 30 ) );
+}
+
+bool TestQgsSvgCache::imageCheck( const QString &testName, QImage &image, int mismatchCount )
+{
+  //draw background
+  QImage imageWithBackground( image.width(), image.height(), QImage::Format_RGB32 );
+  QgsRenderChecker::drawBackground( &imageWithBackground );
+  QPainter painter( &imageWithBackground );
+  painter.drawImage( 0, 0, image );
+  painter.end();
+
+  mReport += "<h2>" + testName + "</h2>\n";
+  QString tempDir = QDir::tempPath() + '/';
+  QString fileName = tempDir + testName + ".png";
+  imageWithBackground.save( fileName, "PNG" );
+  QgsRenderChecker checker;
+  checker.setControlName( "expected_" + testName );
+  checker.setRenderedImage( fileName );
+  checker.setColorTolerance( 2 );
+  bool resultFlag = checker.compareImages( testName, mismatchCount );
+  mReport += checker.report();
+  return resultFlag;
+}
+QGSTEST_MAIN( TestQgsSvgCache )
+#include "testqgssvgcache.moc"

tests/testdata/test_symbol_svg2.svg

@@ -0,0 +1,76 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!-- Generator: Adobe Illustrator 16.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
+
+<svg
+   xmlns:dc="http://purl.org/dc/elements/1.1/"
+   xmlns:cc="http://creativecommons.org/ns#"
+   xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
+   xmlns:svg="http://www.w3.org/2000/svg"
+   xmlns="http://www.w3.org/2000/svg"
+   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
+   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
+   version="1.1"
+   id="svg2"
+   inkscape:output_extension="org.inkscape.output.svg.inkscape"
+   sodipodi:docname="test_symbol_svg2.svg"
+   inkscape:version="0.91 r13725"
+   sodipodi:version="0.32"
+   x="0px"
+   y="0px"
+   width="306.33475"
+   height="484.79999"
+   viewBox="0 0 306.33475 484.79999"
+   enable-background="new 0 0 580 580"
+   xml:space="preserve"><metadata
+     id="metadata26"><rdf:RDF><cc:Work
+         rdf:about=""><dc:format>image/svg+xml</dc:format><dc:type
+           rdf:resource="http://purl.org/dc/dcmitype/StillImage" /><dc:title></dc:title></cc:Work></rdf:RDF></metadata><sodipodi:namedview
+     showgrid="false"
+     inkscape:cy="-115.04372"
+     inkscape:cx="-222.39645"
+     inkscape:zoom="0.46083856"
+     pagecolor="#ffffff"
+     bordercolor="#666666"
+     guidetolerance="10.0"
+     objecttolerance="10.0"
+     gridtolerance="10.0"
+     borderopacity="1.0"
+     id="base"
+     inkscape:current-layer="svg2"
+     inkscape:window-y="34"
+     inkscape:window-x="75"
+     inkscape:pageopacity="0.0"
+     inkscape:pageshadow="2"
+     inkscape:window-width="1014"
+     inkscape:window-height="711"
+     fit-margin-top="0"
+     fit-margin-left="0"
+     fit-margin-right="0"
+     fit-margin-bottom="0"
+     inkscape:window-maximized="0" /><defs
+     id="defs4" /><g
+     id="layer3"
+     transform="matrix(48.14969,0,0,48.14969,-673.10453,-80.905752)"
+     inkscape:label="Layout"
+     display="none"
+     style="display:none"><rect
+       id="rect4134"
+       x="1"
+       y="1"
+       display="inline"
+       width="10"
+       height="10"
+       style="display:inline;fill:none;stroke:#757575;stroke-width:0.1" /><rect
+       id="rect4136"
+       x="2"
+       y="2"
+       display="inline"
+       width="8"
+       height="8"
+       style="display:inline;fill:none;stroke:#757575;stroke-width:0.1" /></g><path
+     fill="param(fill)"
+     stroke="param(outline)"
+     stroke-width="param(outline-width)"
+     d="m 306.29574,465.246 c -0.076,10.965 -9.608,19.554 -21.7,19.554 l -0.179,-0.001 c -10.191,0 -18.79,-6.488 -20.891,-15.586 -14.36,0.915 -21.136,3.602 -28.298,6.441 -8.834,3.503 -17.969,7.125 -41.421,8.282 l -77.79,0.72 C 67.792742,484.118 28.808742,456.176 0.14574201,401.607 c -0.269,-0.512 -0.158,-1.14 0.27,-1.527 0.428,-0.389 1.06299999,-0.438 1.54599999,-0.123 l 2.563,1.678 c 0.188,0.123 0.34,0.293 0.44,0.494 9.638,19.316 45.785,36.26 77.355,36.26 20.794998,0 35.283998,-7.488 40.814998,-21.088 1.526,-7.885 9.78,-15.345 18.753,-17.111 l 0.232,-136.159 c -8.047,-0.854 -16.265,-9.484 -16.429,-17.656 l -4.264,-224.286 c -0.129,-6.844 1.612,-12.169 5.175,-15.83 5.975,-6.138 15.568,-6.214 21.3,-6.259 0.004,0 0.008,0 0.012,0 l 41.061,0.062 c 6.554,0.03 15.794,2.78 21.66,8.855 4.101,4.246 6.084,9.527 5.896,15.693 l -6.164,221.815 c -0.216,8.035 -8.456,16.616 -16.454,17.507 l -0.179,136.605 c 12.057,0.934 20.348,6.056 26.027,16.053 4.234,7.45 10.999,10.773 21.935,10.773 4.717,0 9.758,-0.598 14.633,-1.176 2.118,-0.251 4.297,-0.51 6.422,-0.715 l -0.037,-5.658 c 0.075,-10.813 9.862,-19.609 21.854,-19.609 12.018,0.019 21.782,8.835 21.767,19.652 l -0.039,45.389 z"
+     id="path24"
+     inkscape:connector-curvature="0" /></svg>