Fix crashes and issues with SVG rendering #5497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nyalldawson
force-pushed
the
svg_crash
branch
from
f64bf2d
to
18166f6
Compare
When the svg cache was full, any attempt to render an svg from the cache would crash QGIS. Fixes qgis#16643
…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 qgis#17089, qgis#17077
- Remove QgsSvgCacheEntry from public API (is an internal detail only) - Modernize code - Make protected QgsSvgCache members private, since this class is not designed to be subclassed
nyalldawson
force-pushed
the
svg_crash
branch
from
18166f6
to
b07f675
Compare
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 qgis#13565
nyalldawson
changed the title
|
I'd love a review if someone wants to look over this! |
|
I confirm issue 17089 is fixed when applying this PR. Nice. |
|
I can confirm ##16643 has been resolved in the master, even without this patch! |
This was referenced May 24, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues fixed:
When the svg cache was full, any attempt to render an svg from the cache would crash QGIS. (Fixes WMS tiles are always fetched from server, never from cache #16643)
Fix crashes caused by concurrent rendering of cached QPictures from QgsSvgCache (ftools "Convex hull(s)": does not work in qgis master when using "create convex hulls based on input field" #17089, Disappearing data #17077)
Check file modified time when retrieving svg images from cache (Crash when GPSD goes away and trying to connect again #13565)
(Underlying bugs seems to have been present forever, so fix will need to be backported)