Fix crashes caused by concurrent rendering of cached QPictures from QgsSvgCache

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
This commit is contained in:
Nyall Dawson 2017-10-31 10:24:18 +10:00
parent 942f431f0f
commit a6eea7205c
6 changed files with 91 additions and 13 deletions

View File

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

View File

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

View File

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

View File

@ -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,

View File

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

View File

@ -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"