Skip to content

Commit a6eea72

Browse files
nyalldawsonnyalldawson
committedOct 31, 2017
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
·
ltr-3_40final-3_0_0
1 parent 942f431 commit a6eea72

File tree

6 files changed

+91
-13
lines changed

6 files changed

+91
-13
lines changed
 

‎src/app/composer/qgscomposerpicturewidget.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ QIcon QgsComposerPictureWidget::svgToIcon( const QString &filePath ) const
448448
strokeWidth = 0.6;
449449

450450
bool fitsInCache; // should always fit in cache at these sizes (i.e. under 559 px ^ 2, or half cache size)
451-
const QImage &img = QgsApplication::svgCache()->svgAsImage( filePath, 30.0, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
451+
QImage img = QgsApplication::svgCache()->svgAsImage( filePath, 30.0, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
452452

453453
return QIcon( QPixmap::fromImage( img ) );
454454
}

‎src/core/symbology/qgsfillsymbollayer.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1930,12 +1930,12 @@ void QgsSVGFillSymbolLayer::applyPattern( QBrush &brush, const QString &svgFileP
19301930
{
19311931
bool fitsInCache = true;
19321932
double strokeWidth = context.renderContext().convertToPainterUnits( svgStrokeWidth, svgStrokeWidthUnit, svgStrokeWidthMapUnitScale );
1933-
const QImage &patternImage = QgsApplication::svgCache()->svgAsImage( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
1934-
context.renderContext().scaleFactor(), fitsInCache );
1933+
QImage patternImage = QgsApplication::svgCache()->svgAsImage( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
1934+
context.renderContext().scaleFactor(), fitsInCache );
19351935
if ( !fitsInCache )
19361936
{
1937-
const QPicture &patternPict = QgsApplication::svgCache()->svgAsPicture( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
1938-
context.renderContext().scaleFactor() );
1937+
QPicture patternPict = QgsApplication::svgCache()->svgAsPicture( svgFilePath, size, svgFillColor, svgStrokeColor, strokeWidth,
1938+
context.renderContext().scaleFactor() );
19391939
double hwRatio = 1.0;
19401940
if ( patternPict.width() > 0 )
19411941
{

‎src/core/symbology/qgsmarkersymbollayer.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2009,8 +2009,8 @@ void QgsSvgMarkerSymbolLayer::renderPoint( QPointF point, QgsSymbolRenderContext
20092009
if ( !context.renderContext().forceVectorOutput() && !rotated )
20102010
{
20112011
usePict = false;
2012-
const QImage &img = QgsApplication::svgCache()->svgAsImage( path, size, fillColor, strokeColor, strokeWidth,
2013-
context.renderContext().scaleFactor(), fitsInCache, aspectRatio );
2012+
QImage img = QgsApplication::svgCache()->svgAsImage( path, size, fillColor, strokeColor, strokeWidth,
2013+
context.renderContext().scaleFactor(), fitsInCache, aspectRatio );
20142014
if ( fitsInCache && img.width() > 1 )
20152015
{
20162016
//consider transparency
@@ -2032,9 +2032,8 @@ void QgsSvgMarkerSymbolLayer::renderPoint( QPointF point, QgsSymbolRenderContext
20322032
if ( usePict || !fitsInCache )
20332033
{
20342034
p->setOpacity( context.opacity() );
2035-
const QPicture &pct = QgsApplication::svgCache()->svgAsPicture( path, size, fillColor, strokeColor, strokeWidth,
2036-
context.renderContext().scaleFactor(), context.renderContext().forceVectorOutput(), aspectRatio );
2037-
2035+
QPicture pct = QgsApplication::svgCache()->svgAsPicture( path, size, fillColor, strokeColor, strokeWidth,
2036+
context.renderContext().scaleFactor(), context.renderContext().forceVectorOutput(), aspectRatio );
20382037
if ( pct.width() > 1 )
20392038
{
20402039
p->save();

‎src/core/symbology/qgssvgcache.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,9 @@ QPicture QgsSvgCache::svgAsPicture( const QString &path, double size, const QCol
170170
trimToMaximumSize();
171171
}
172172

173-
return *( currentEntry->picture );
173+
QPicture p = *( currentEntry->picture );
174+
p.detach();
175+
return p;
174176
}
175177

176178
QByteArray QgsSvgCache::svgContent( const QString &path, double size, const QColor &fill, const QColor &stroke, double strokeWidth,

‎src/gui/symbology/qgssvgselectorwidget.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ QPixmap QgsSvgSelectorListModel::createPreview( const QString &entry ) const
262262
strokeWidth = 0.2;
263263

264264
bool fitsInCache; // should always fit in cache at these sizes (i.e. under 559 px ^ 2, or half cache size)
265-
const QImage &img = QgsApplication::svgCache()->svgAsImage( entry, mIconSize, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
265+
QImage img = QgsApplication::svgCache()->svgAsImage( entry, mIconSize, fill, stroke, strokeWidth, 3.5 /*appr. 88 dpi*/, fitsInCache );
266266
return QPixmap::fromImage( img );
267267
}
268268

‎tests/src/core/testqgssvgcache.cpp

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@
2020
#include <QFileInfo>
2121
#include <QDir>
2222
#include <QDesktopServices>
23-
23+
#include <QPicture>
24+
#include <QPainter>
25+
#include <QtConcurrent>
2426
#include "qgssvgcache.h"
2527

2628
/**
@@ -40,6 +42,8 @@ class TestQgsSvgCache : public QObject
4042
void init() {} // will be called before each testfunction is executed.
4143
void cleanup() {} // will be called after every testfunction.
4244
void fillCache();
45+
void threadSafePicture();
46+
void threadSafeImage();
4347

4448
};
4549

@@ -77,6 +81,79 @@ void TestQgsSvgCache::fillCache()
7781
}
7882
}
7983

84+
struct RenderPictureWrapper
85+
{
86+
QgsSvgCache &cache;
87+
QString svgPath;
88+
double size = 100;
89+
explicit RenderPictureWrapper( QgsSvgCache &cache, const QString &svgPath )
90+
: cache( cache )
91+
, svgPath( svgPath )
92+
{}
93+
void operator()( int )
94+
{
95+
QPicture pic = cache.svgAsPicture( svgPath, size, QColor( 255, 0, 0 ), QColor( 0, 255, 0 ), 1, 1, true );
96+
QSize imageSize = pic.boundingRect().size();
97+
QImage image( imageSize, QImage::Format_ARGB32_Premultiplied );
98+
image.fill( 0 ); // transparent background
99+
QPainter p( &image );
100+
p.drawPicture( 0, 0, pic );
101+
}
102+
};
103+
104+
void TestQgsSvgCache::threadSafePicture()
105+
{
106+
// QPicture playback is NOT thread safe with implicitly shared copies - this
107+
// unit test checks that concurrent drawing of svg as QPicture from QgsSvgCache
108+
// returns a detached copy which is safe to use across threads
109+
110+
// refs:
111+
// https://issues.qgis.org/issues/17077
112+
// https://issues.qgis.org/issues/17089
113+
114+
QgsSvgCache cache;
115+
QString svgPath = TEST_DATA_DIR + QStringLiteral( "/sample_svg.svg" );
116+
117+
// smash picture rendering over multiple threads
118+
QVector< int > list;
119+
list.resize( 100 );
120+
QtConcurrent::blockingMap( list, RenderPictureWrapper( cache, svgPath ) );
121+
}
122+
123+
124+
struct RenderImageWrapper
125+
{
126+
QgsSvgCache &cache;
127+
QString svgPath;
128+
double size = 100;
129+
explicit RenderImageWrapper( QgsSvgCache &cache, const QString &svgPath )
130+
: cache( cache )
131+
, svgPath( svgPath )
132+
{}
133+
void operator()( int )
134+
{
135+
bool fitsInCache = false;
136+
QImage cachedImage = cache.svgAsImage( svgPath, size, QColor( 255, 0, 0 ), QColor( 0, 255, 0 ), 1, 1, fitsInCache );
137+
QImage image( cachedImage.size(), QImage::Format_ARGB32_Premultiplied );
138+
image.fill( 0 ); // transparent background
139+
QPainter p( &image );
140+
p.drawImage( 0, 0, cachedImage );
141+
}
142+
};
143+
144+
void TestQgsSvgCache::threadSafeImage()
145+
{
146+
// This unit test checks that concurrent rendering of svg as QImage from QgsSvgCache
147+
// works without issues across threads
148+
149+
QgsSvgCache cache;
150+
QString svgPath = TEST_DATA_DIR + QStringLiteral( "/sample_svg.svg" );
151+
152+
// smash image rendering over multiple threads
153+
QVector< int > list;
154+
list.resize( 100 );
155+
QtConcurrent::blockingMap( list, RenderImageWrapper( cache, svgPath ) );
156+
}
80157

81158
QGSTEST_MAIN( TestQgsSvgCache )
82159
#include "testqgssvgcache.moc"

2 commit comments

Comments
 (2)

rldhont commented on Jun 3, 2018

@rldhont
rldhont
on Jun 3, 2018
Contributor

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

nyalldawson commented on Jun 3, 2018

@nyalldawson
nyalldawson
on Jun 3, 2018
CollaboratorAuthor

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

Please sign in to comment.