Check file modified time when retrieving svg images from cache

If file has been modified since the cache, regenerate a new cache image. We don't want to check the file modified time too often though, (e.g., we don't want to check for every point render in a 100k point file), so use a hardcoded 30 second minimum time between consecutive file modified checks. This means that file modifications occuring more often than every 30 seconds won't be picked up till 30 seconds has elapsed since the last modification. But at the same time it means that if the render takes < 30 seconds we'll only check each svg at most once (and if a render takes > 30 seconds, adding a few more milliseconds won't hurt!). Fixes #13565
qgis · Oct 31, 2017 · 0b2de85 · 0b2de85
1 parent b07f675
commit 0b2de85
Show file tree

Hide file tree

Showing 7 changed files with 211 additions and 6 deletions.
diff --git a/src/core/symbology/qgssvgcache.cpp b/src/core/symbology/qgssvgcache.cpp
@@ -40,19 +40,26 @@
 
 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 )
   , fixedAspectRatio( far )
   , fill( fi )
   , stroke( ou )
 {
+  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
@@ -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 );
 
@@ -552,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 )
   {
@@ -561,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;
     }

diff --git a/src/core/symbology/qgssvgcache.h b/src/core/symbology/qgssvgcache.h
@@ -27,6 +27,8 @@
 #include <QUrl>
 #include <QObject>
 #include <QSizeF>
+#include <QDateTime>
+#include <QElapsedTimer>
 
 #include "qgis_core.h"
 
@@ -46,7 +48,7 @@ class CORE_EXPORT QgsSvgCacheEntry
 {
   public:
 
-    QgsSvgCacheEntry() = default;
+    QgsSvgCacheEntry() = delete;
 
     /**
      * Constructor.
@@ -68,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;
@@ -248,6 +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 );
 
+    //! Minimum time (in ms) between consecutive svg file modified time checks
+    int mFileModifiedCheckTimeout = 30000;
+
     //! Entry pointers accessible by file name
     QMultiHash< QString, QgsSvgCacheEntry * > mEntryLookup;
     //! Estimated total size of all images, pictures and svgContent
@@ -297,6 +309,7 @@ class CORE_EXPORT QgsSvgCache : public QObject
     //! 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
diff --git a/tests/src/core/testqgssvgcache.cpp b/tests/src/core/testqgssvgcache.cpp
@@ -23,7 +23,10 @@
 #include <QPicture>
 #include <QPainter>
 #include <QtConcurrent>
+#include <QElapsedTimer>
 #include "qgssvgcache.h"
+#include "qgsmultirenderchecker.h"
+#include "qgsapplication.h"
 
 /**
  * \ingroup UnitTests
@@ -33,8 +36,11 @@ class TestQgsSvgCache : public QObject
 {
     Q_OBJECT
 
-  public:
-    TestQgsSvgCache() = default;
+  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.
@@ -44,6 +50,7 @@ class TestQgsSvgCache : public QObject
     void fillCache();
     void threadSafePicture();
     void threadSafeImage();
+    void changeImage(); //check that cache is updated if svg source file changes
 
 };
 
@@ -52,10 +59,22 @@ 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()
@@ -155,5 +174,86 @@ void TestQgsSvgCache::threadSafeImage()
   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"
diff --git a/...trol_images/expected_svgcache_changed_after/expected_svgcache_changed_after.png b/...trol_images/expected_svgcache_changed_after/expected_svgcache_changed_after.png
diff --git a/...images/expected_svgcache_changed_after/expected_svgcache_changed_after_mask.png b/...images/expected_svgcache_changed_after/expected_svgcache_changed_after_mask.png
diff --git a/...ol_images/expected_svgcache_changed_before/expected_svgcache_changed_before.png b/...ol_images/expected_svgcache_changed_before/expected_svgcache_changed_before.png
diff --git a/tests/testdata/test_symbol_svg2.svg b/tests/testdata/test_symbol_svg2.svg