Replace QPainter with Blend2D in CMapIMG#1131
Conversation
|
First quick impression:
|
|
From a package maintainance perspective those shouldn't be a big thing.
They are pulled in via cmake automatically and we manage their version by setting a git tag. Almost no distro packages these so we don't even have to handle the option to build against system versions.
Strange... For me it's the difference between often >1s per draw to ~300ms which is quite a large difference in usability.
Your GPU should not make a difference. Blend2D is a pure software rasterizer so runs only on CPU. With the profiler you could easily check what impact reading the data has
as the loading step will show up as it's own zone.
|
|
My attempt to build the PR jmlzr:QMS-1130 with my default CMake parameters failed on Windows 11: There are lots of new CMAKE variables. I used their default settings. More info is certainly needed to build successfully on Windows. Must there any additional packages be downloaded before building? Sorry for the message layout - I copied it from the output log. |
|
@wthaem Yes cmake will need to download a few packages via FetchContent. Can you check at the start of cmake output if that failed or something? If you run cmake directly it should normally already fail at that point though... If you had the build directory already before switching to this branch you could maybe try cmake with |
|
same problem on a Linux machine. Even with cmake from scratch. The tracy header path is not set properly for the target. |
|
I'm starting the build always with The file Edit: ChatGPT info: "Does ROCprofiler-SDK exist on Windows? Officially: no, not as a supported Windows package." How to switch off the use of tracy? My settings: |
|
I'm sorry I should have read your error message more carefully! Turns out I accidentally added an include to a qmaptool file (and qmaptool is not linked with Tracy). Since I always just built the qmapshack target I did not notice. I fixed that by removing the include and confirmed that now all our targets build successfully. You can ignore that missing rocprofiler. That's just an optional dependency of Tracy we don't need. |
|
Now building is ok and nothing special noticed with a quick first test (KMz now again without blurry). |
|
Sure: the KMZ topic belongs to the other PR. But I noticed again the difference in quality with this PR. I tried IMG maps and many types of raster maps. Loading, zooming, panning of IMGs was pretty quick. Image quality on my notebook screen quite good. My notebook has a good CPU! |
|
Currently, I am trying to build on macOS. I get tons of warnings like these: Might it be possible to expand cmake configuration not to throw these warnings? |
|
@JFritzle Pretty sure that's the same issue I already described in the PR description and Apple's clang just uses a different wording for that warning then GCC:
|
|
Did not notice a performance improvement with map Reit_Wanderkarte_Italia_11_25.img on my Windows 11 PC: |
|
Haven't had the guts to deal with Tracy yet. But my hands-on experience is similar. On my high performance work laptop I can notice a small difference in performance. On my old private laptop no noticeable gain. But on both machines I have to increase the details level quite a lot to get a noticeable lag. More than I usually would do. So the benefit is on the lower side I am a bit undecided. Pulling in new dependencies is always a risk and makes me feel uneasy. On the other hand side I highly appreciate and acknowledge every work donated to QMapShack. This places me in between chairs. It's always a hard decision based on the estimated future of the feature. Will it cause more benefit than future trouble? There is already quite some stuff I would like to get rid of (alglib, quazip) either by kicking out the feature or replacing it with native code. Dropping features is always trouble because of this one user popping up actually using it. And replacing code is another problem on it's own. For example what happens if I have to decide that blend2d can't be used anymore for whatever reason. I have to revert to Qt. Users might notice the performance loss and complain. Restoring the former code is possible. But might be more trouble than expected. So there must be a benefit worth it. For example replacing the QMapShack native code with the Garmin SDK was a tough decision, too. Apart from a not 100% clarified license situation, it's now depending on Garmin's lib. On the other hand side, the native code was done ages ago by someone not reachable any more. The newer devices implement new features that require to actually notice the change (reported by users), understanding the rather complex code and fixing the stuff without breaking something. To get rid of this maintenance burden picking the Gamrin Lib was a choice. So long story short: I am still undecided. Even after writing all of this. |
|
That's very strange. I guess you guys must be either using maps only in areas with very little features or have screens with low resolution if you don't have noticeable lag with the previous renderer. @kiozen I don't get your point tbh. If you ever wanna get rid of Blend2D you can just revert. It will only be used in the IMG renderer and that one is unmaintained since a while anyways, so not even like there is much of a chances of there being changes that then might need back porting. Most importantly: Why would a user complaining about performance in the future after a hypothetically possible revert matter more than one complaining now about current performance? |
|
Tested on a 2560x1440 screen. With Freizeitkarte and a big City like Rome. I am not really that much of a fan of very large screens. I appreciate multiple (cheap) screens. But that's just personal taste. I passed the problem of a speed up vs dependency to Claude, asking if a performance gain could be achieved with Qt, too. This is the first answer. Of course this has to be taken with a grain of salt. This is not a proven enhancement with measured performance gain. But it seems to be interesting enough to give it a try. Let's see how much development time I can spare on that the next week. PR #1131 — Replace QPainter with Blend2D in CMapIMG The PR substitutes Blend2D for Qt's software rasterizer in CMapIMG's geometry pipeline (drawPolygons, drawPolylines, drawPoints, drawPois) while keeping text on QPainter. The benchmark numbers are real (5× polyline, 2.5× polygon), but the implementation has several correctness gaps and the core architectural question — is Blend2D the right fix — deserves scrutiny. Findings:
Can Qt alone close most of this gap? Yes — significantly. The Blend2D speedup is real, but it's partly chasing the symptom (slow rasterizer) while some of the root causes are algorithmic:
Bottom line: Items 1–3 alone could realistically recover 50–70% of Blend2D's measured gain with no new dependency, no premultiply round-trip hazard, and no hybrid-API surface to maintain. I'd suggest doing those first (especially re-enabling simplification if it's safe), profiling again, and then deciding whether the remaining gap justifies pulling in Blend2D. |
|
A grain of salt is underselling it. You have to be really careful with letting Claude think where it can make performance gains. It's wrong a lot of time cause it misjudges the costs of things. For example I can already tell you that this pixel format conversion is way too cheap to matter. Also I wanted to look into if changing the format of the double buffers in IDrawContext would be possible without regressions, as even the Qt docs say that for composition work the premultiplied is preferred. I'm very doubtful on the other points as well as I spent quite some time before the port to Blend2D with letting it optimize the Qt renderer and I got to only around 20%. Plus I could see in Tracy that almost all time was spent in Qt's rasterizer. If you look at benchmarks you will also see that Qt is just very very slow: https://blend2d.com/performance.html Maybe seeing the Blend2D port gave it ideas that indeed would make a difference with Qt so please try it if you'd like but I'd be very surprised if it got anywhere close. I found that Claude gets way better at performance optimization if you let it instrument all it's suspicions and run the program with Tracy recording it. Then you ether tell it what the expensive zones are or even just feed it I have a few more commits for this branch local that I still need to review and clean up before I push. Got some more performance out of it, made some refactoring plus I fixed that bug (that was in the original Qt version and been bugging me since forever) of patterned fills shifting around. |
|
Ok, agreed. Fixing it on the Qt side does not hit the same performance by quite a gap compared to Blend2d. So the only option to speed up rendering is to rely on non Qt code. Go ahead. |
Add the profiler Tracy to the project and start instrumenting some functions relevant to IMG rendering. With tracy turned off at compile time this won't add any overhead. Having a profiler available is essential to guide performance improvements. To use it turn on tracy with -DTRACY_ENABLE=1 then build the profiler application itself, launch QMS and the profiler and connect it. The tracy project will end in _deps/tracy-src inside the build dir.
Dramatically speed up IMG rendering by replacing the very slow Qt vector engine with the much faster Blend2D for all but text rendering.
Both classes declared `virtual ~...() = default;` but are never used polymorphically. That user-declared destructor added a vtable pointer to every object and suppressed the implicit (noexcept) move constructor / assignment, so the QVector<CGarminPolygon>/<CGarminPoint> buffers used during RGN decode could only grow by copying their QPolygonF/QStringList members. Removing the destructor (rule of zero) restores move relocation and drops the vtable. No behaviour change.
- Replace C-style casts with reinterpret_cast/static_cast in readFile, readBasics, readSubfileBasics and loadSubDiv. - Make the raw RGN data pointers (pRawData, pOffset) const-correct. - Hoist the repeated subfile.parts["TRE"]/["RGN"]/["LBL"]/["NET"] map lookups into single bound references instead of re-querying the QMap per offset. - Header: typedef -> using for polytype_t/pointtype_t; collapse map_level_t::operator== to a single boolean expression. - getToolTip: fold the two identical "join dict values, skip the - placeholder" loops into a single appendValues lambda. - Replace the STREETNAME_THRESHOLD macro with a typed, documented constexpr qreal kStreetNameThreshold.
- drawPolygons: bucket polygon indices by type once and walk polygonDrawOrder hitting only buckets that exist, replacing the O(types x N) scan that re-iterated the whole polygon list once per draw-order entry. - drawPolygons/drawPoints/drawPois: look up the property maps with constFind and a static-default fallback instead of non-const operator[], which silently inserted a default entry for every missing type on every frame (mutating shared state and polluting tooltip lookups). drawPolylines skips absent types via constFind too. - Submit geometry to Blend2D in bulk: QPointF is layout-compatible with BLPoint (static_assert), so toBLPoints() feeds a QPolygonF straight to fill_polygon / stroke_polygon / stroke_polyline. Drops the per-line BLPath allocation and the vertex-by-vertex polyToPath helper. - Modernize the (disabled) debug border blocks to the current Blend2D API.
Profiling showed CMapIMG::draw spends ~44% in loadVisibleData and ~20% in drawPolylines. Add named Tracy zones to localize the cost inside those: - loadSubDiv, CGarminPolygon::decode/decode2, CGarminPoint::decode/decode2 -> isolate geometry decode. - strtbl.get (CGarminStrTbl6/8/Utf8::get) -> isolate label lookup (an mmap + codepage decode per labelled object, much of it wasted when zoomed out). - readFile.RGN -> separate the per-frame RGN read/XOR from decode. - isCompletelyOutside, img2line, polyline.pixmap -> isolate culling and the per-segment pixmap-along-line work. - IDrawContext::convertRad2Px(QPolygonF&) -> PROJ conversion share; the QPointF overload already had a zone, the polygon overload did not.
Tracy showed label decoding was the largest slice of loadVisibleData: every labelled object triggered an mmap (via CFileExt::data) + a codepage decode in IGarminStrTbl, redone every frame although label content is immutable. Turn IGarminStrTbl::get() into a non-virtual wrapper over a bounded LRU QCache<quint64, QStringList> (key (type << 32) | offset); on a miss it delegates to a new protected virtual decode() - the renamed per-coding get() of CGarminStrTbl6/8/Utf8. A hit needs no mmap or decode, so it also shrinks the per-subfile CFileExt::free() munmap loop. QCache evicts least-recently-used entries, so memory stays bounded while panning very large gmapsupp files. The cache is only touched from the (serialized) draw thread, so it needs no lock. Tracy: strtbl.get now measures the wrapper (hits+misses), strtbl.decode the miss path only - their call-count ratio reads out the hit rate.
convertRad2Px/convertPx2Rad map rad<->px as (proj(p) - focusInMetres) / (scale*zoomFactor) + center, and reprojected the focus on every call - 2.5M+ times per draw on a dense vector map (Tracy: ~13% of a draw, spread across drawPolygons + drawPolylines). - Cache focus-in-metres in IDrawContext::focusInM, computed once in draw() right after focus is set (its only write site) and reused by all three conversions. Recomputed every draw, so setProjection is handled for free. - Drop the per-call QVector<p_t> allocation in the polygon overload: the rare antimeridian wrap-fixes now go into a stack QVarLengthArray, and scale*zoomFactor is hoisted out of the per-point loop. Behaviour-preserving for every caller (GIS items, DEM, overlays, ...): focusInM equals the old per-call convertRad2M(focus), and the antimeridian check is kept verbatim (it tests y, never fires, left as-is rather than "fixed").
Anchor the pattern origin in global coordinates so the patterns are independent of the viewport and it's position.
|
Rebased on current dev and added most of the commits I mentioned that I had by now reviewed and cleaned up. I don't really like the cache focus-in-metres in IDrawContext for convertRad2Px commit, might remove that again. Also gave only 1-2% performance. |
|
If you change anything in those central methods like convertRad2Px please do it in it's own PR. I want this completely isolated from anything else to revert it easily if anything breaks |
4 participants
What is the linked issue for this pull request:
QMS-#1130
What you have done:
Added Tracy as a profiler and instrumented the IMG rendering to look for where to focus my performance optimization efforts. That showed that Qt's rasterizer is the bottleneck that will dwarf any gains that could be made in the code. With the biggest bottlenecks being
drawPolylinesby and followed bydrawPolygons.So I decided to let Claude replace Qt's very slow rasterizer with Blend2D which is probably the fastest software rasterizer/vector graphics library in existence.
The new code seems to result in the same output but runs ~3 times faster!
Performance comparison
To make a comparison I recorded traces of me panning and zooming around the same area before and after the commit that switched to Blend2D and then used Tracy's compare feature.
drawPolylinessaw a massive 5x improvement, dropping from 569ms mean to 117ms:drawPolygonssaw a also impressive 2.5x improvement, from 170ms to 67ms:Looking at
drawis deceiving as the results of that are diluted by early exists from redraw requests.Render output
I took comparison screenshots before of the same view before and after, map used is a Freizeitkarte IMG.
Before:
After:
As you can see there is virtually no visible difference! The only thing I can notice on this view is the POIs shifting a little but they seem to shift by a similar amount with the previous renderer as well when panning minimally so I'm not sure if that is even a regression.
Next steps
Since this is a rather large change and I haven't checked if Claude did stupid things I made this just a draft for now. I do encourage people to try it out though!
I think now that the biggest bottleneck is gone I could also squeeze out probably at least another ~20% from optimizing the rest of the code.
Btw the massive amount of
ISO C++ prohibits anonymous structswarnings comes from the fact that our CMakeLists.txt unfortunately sets global compile options withadd_compile_optionsthat then affects subprojects as well. The better thing would be to set them only for our own targets withtarget_compile_options. But we can fix that if we get close to merging this, for now I didn't want to bother with that.Steps to perform a simple smoke test:
Open a IMG.
Tracy
If anyone wants to play with Tracy:
-DTRACY_ENABLE=1 -DTRACY_ON_DEMAND=1./profiler/build/tracy-profilerand connect to the running QMS instance.Does the code comply to the coding rules and naming conventions Coding Guidelines:
Is every user facing string in a tr() macro?
Did you add the ticket number and title into the changelog? Keep the numeric order in each release block.
No as only a draft for now.