Skip to content
Permalink
Browse files

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
  • Loading branch information
nyalldawson committed Oct 31, 2017
1 parent 942f431 commit a6eea7205c72a1be837ab43b79aad0c67a92a9b2
@@ -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 ) );
}
@@ -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 )
{
@@ -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();
@@ -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,
@@ -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 );
}

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

2 comments on commit a6eea72

@rldhont

This comment has been minimized.

Copy link
Contributor

@rldhont rldhont replied Jun 3, 2018

@nyalldawson Do you think this fix can be ported to release-2_18 with 75b7edf ?

@nyalldawson

This comment has been minimized.

Copy link
Collaborator Author

@nyalldawson nyalldawson replied Jun 3, 2018

@rldhont sure, should be safe to do! Go ahead :)

Please sign in to comment.
You can’t perform that action at this time.