Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always render SVG to Pixmap, to prevent main thread stucking due to File IO #12904

Closed
wants to merge 1 commit into from

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Mar 2, 2024

I noticed some stucking of the Main Thread, due to File IO. This happens from time to time during normal operation. The reason is re-rendering of many SVG files used in the skins at once. If the Main Thread stucks, of cause the waveform rendering and the Qt event queue stuck as well:
grafik

I could prevent this stucking by the experimental code of this PR, which always create a Pixmap for these images.

I don't know if this is a Windows specific issue, or also affect other platforms.

@github-actions github-actions bot added the ui label Mar 2, 2024
@Swiftb0y
Copy link
Member

Swiftb0y commented Mar 2, 2024

fyi for the QML skin in general it would make sense to have some infrastructure in place that renders our SVG sources to pixmaps of different sizes as part of the build because that is much much faster to render (https://doc.qt.io/qt-6/qml-qtquick-image.html#sourceSize-prop)

@daschuer
Copy link
Member

daschuer commented Mar 3, 2024

This is an interesting finding. Maybe we can implement a caching around QSvgRenderer We may cache the svg source and the pixels.

@daschuer
Copy link
Member

daschuer commented Mar 3, 2024

I have just skimmed through the Qt source and it look like there is already a caching in place. We do probably something wrong that invalidates the cache constantly.

@JoergAtGithub
Copy link
Member Author

Did you test this on Linux, does it improve performance there?

@ronso0
Copy link
Member

ronso0 commented Mar 29, 2024

@JoergAtGithub looks like the issue you discovered is also the reason some SVGs automagically update sometimes when I switch back to the Mixxx window after having edited them.

@JoergAtGithub
Copy link
Member Author

@JoergAtGithub looks like the issue you discovered is also the reason some SVGs automagically update sometimes when I switch back to the Mixxx window after having edited them.

Does this PR fixes this issue for you?

@ronso0
Copy link
Member

ronso0 commented Mar 29, 2024

I can't tell, I can't reproduce it reliably in the first place.

@daschuer
Copy link
Member

daschuer commented Jul 5, 2024

I should have taken more notes:

I have just skimmed through the Qt source and it look like there is already a caching in place. We do probably something wrong that invalidates the cache constantly.

Anyway, the rendering at this place is not ideal, because we may scale the picture and already have lost the vectors.

Did you consider to cache a QImage in the draw function?

@JoergAtGithub JoergAtGithub marked this pull request as ready for review August 16, 2024 22:10
@JoergAtGithub
Copy link
Member Author

JoergAtGithub commented Aug 16, 2024

I use this since some months on Windows now, and I' pretty sure, that this is only benficial. Slightly less overall CPU usage and less stucking events.

@daschuer
Copy link
Member

I am currently consider if this this is a 2.4.2 fix or not. On one hand it looks experimental, on the other hand it seems to have a good effect on windows. Is there a receipt for a test? I like to do the Linux test.
It would be great if we can backup this change with some insights why this helps and why it is not worse for other things.

@JoergAtGithub
Copy link
Member Author

Is there a receipt for a test? I like to do the Linux test. It would be great if we can backup this change with some insights why this helps and why it is not worse for other things.

I'm pretty sure, that it will not harm Linux. But I don't know if the addressed problem exist on Linux too. It might depend on X11 / Wayland. Testing both would make sense. I'm total unsure about macOS. Which was always handled different here - and this different handling is not touched by this PR.

To trigger complete rerendering of all SVG images, I need to do some operations with the Mixxx main window (minimize/maximize) or other windows (moving Mixxx preferences dialog or other app windows, change app focus, etc.). But in the profiler I see the SVG rendering also happen in normal operation, but there only for a few images at a time.

@daschuer
Copy link
Member

I have debugged a quite a bit through the Qt source, and it turns out that The QSvgRenderer does not use caching.
It renders the Svg again if we refresh the screen.

Apart form resizing, which is heavy anyway and the performance aspect can be neglected. It happens during hover actions and GUI interactions.

The disc access happens if qss states things like this:

image: url(skins:LateNight/palemoon/buttons/btn__pfl_active.svg) no-repeat center center;

A workaround would be to compile all that into a resource file that can be kept im memory.
The other measure would be to use QProxyStyle to pipe in caching of these images.

Using here a pre rendered QPixmaps for streched SVGs will probably introduce blurry pixel.
A workaround would be to cache the stretched QPixmap.

@JoergAtGithub
Copy link
Member Author

This PR solves the issue already, why searching for a workaround?

@daschuer
Copy link
Member

I cannot confirm, the on-the-fly file reading is still there even with this PR.
In addition this may lead to blurry pixel. Because we do not render the cached pixmap to device pixel.

@JoergAtGithub
Copy link
Member Author

Hmm - here it helped against the on the fly file reading...
And I didn't noticed blurry images, but they might occur somewhere where I haven't seen them.
In case of Shade we do this anyway, because of WPixmapStore::willCorrectColors().

@JoergAtGithub
Copy link
Member Author

BTW: There are big improvements in this area in Qt6.8, but mainly affecting QtQuick/QML which has the issue as well: https://www.qt.io/blog/vector-graphics-in-qt-6.8

@daschuer
Copy link
Member

This is the call stack where in the fly rendering with file access happens without Paintable involved:

loadDocument<QString>() at qsvgrenderer.cpp:342 0x7ffff7121cf8	
QSvgRenderer::load() at qsvgrenderer.cpp:342 0x7ffff7121cf8	
QSvgIconEnginePrivate::tryLoad() at qsvgiconengine.cpp:146 0x7fffe832e393	
QSvgIconEnginePrivate::loadDataForModeAndState() at qsvgiconengine.cpp:154 0x7fffe832e3e2	
QSvgIconEngine::pixmap() at atomic_base.h:413 0x7fffe832f810	
QSvgIconEngine::actualSize() at qsvgiconengine.cpp:119 0x7fffe832eb4a	
QIcon::paint() at qsize.h:119 0x7ffff65c2512	
QRenderRule::drawImage() at atomic_base.h:413 0x7ffff6c4cc73	
QRenderRule::drawImage() at qstylesheetstyle.cpp:1.381 0x7ffff6c513d4	
QRenderRule::drawRule() at qstylesheetstyle.cpp:1.389 0x7ffff6c513d4	
QStyleSheetStyle::drawPrimitive() at qstylesheetstyle.cpp:4.409 0x7ffff6c5cd4f	
QStylePainter::drawPrimitive() at qstylepainter.h:80 0x55555622ef55	
WPushButton::paintOnDevice() at wpushbutton.cpp:322 0x55555622ef55	

The Paintable load the SVG file during construction and it is kept in memory as QSvgTinyDocument

The Shade skin does not suffer the issue, because it does not use many SVGs.

@daschuer
Copy link
Member

It could be interesting where the bottle neck is. Probably disc access.
Does this bring a significant improvement with the pending disc issue: #13679?

@JoergAtGithub
Copy link
Member Author

It could be interesting where the bottle neck is. Probably disc access.

According to the profiler picture in the description it's I/O (violett color). The Windows file I/O system is usually slower than on Unix systems. The bandwith itself should be similar, but the access time differs.

@JoergAtGithub
Copy link
Member Author

Closing this in favor of #13679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants