Waveform frame rate drops when scrolling library

[foss-]

Bug Description

The problematic behavior has existed for as long as I can remember using Mixxx on macOS. Filing this issue so it can be discussed, investigated and tracked (searched, but could not find an existing issue).

There seem to be a few issues with Qt and scrolling on macOS. This P1 Critical bug filed 2019 got resolved summer 2022: https://bugreports.qt.io/browse/QTBUG-73117 in Qt 6.2, 6.3, 6.4.

Reproduce

1. open Mixxx Preferences > Waveforms
2. switch back to Library window and scroll up / down continuously

Currently

With frame rate set to 60, when scrolling frame rate drops to 30, while dropped frames accumulate. It goes back to 60 as soon as scrolling stops. This results in waveform not scrolling smoothly when scrolling the library.

Expected

Frame rate should not drop when scrolling. Not a blocker or P1 bug but rather a nuisance.

Zulip: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/macOS.20lagging.20library.20table.20scrolling.20-.20QTBUG-73117

https://github.com/mixxxdj/mixxx/assets/5708172/9f6d25c9-9e93-4a20-a398-031f3757f36f

Version

2.4-alpha-1508-gb763121761 (HEAD)

OS

macOS 13.4

[m0dB]

I did some profiling with Instruments and when scrolling the library more than 30% CPU is spent in QTableView::paintEvent

32.9% QTableView::paintEvent(QPaintEvent) 32.4% QTableViewPrivate::drawCell(QPainter, QStyleOptionViewItem const&, QModelIndex const&) 26.8% QStyledItemDelegate::paint(QPainter*, QStyleOptionViewItem const&, QModelIndex const&) const

and this holds up the event queue.

[daschuer]

@m0dB can you confirm the issue? I cannot confirm the issue with Ubuntu Focal. I see only a minimal drop to 57 f/s or such.

@foss- are you able to blame a specific column? You may remove the columns one by one and check if it makes a difference.

[m0dB]

Yes, I definitely can confirm this and the profiling clearly shows the problem is in the drawing of the table (cells) being slow. I don't think it's a particular cell, it simply all adds up.

[m0dB]

I have done a bit more profiling and most time is passed in drawText. I tried using QStaticText and to draw without eliding. This does improve things a bit but it still holds up the event loop enough to delay the opengl swaps.

[m0dB]

I had an idea for a very simple fix, and low and behold! It works perfectly! See PR https://github.com/mixxxdj/mixxx/pull/11644

[m0dB]

@foss- can you confirm this works for you?

[foss-]

macOS 13.4 Mixxx 2.4-beta-16-g35a6da79c7 (HEAD)

confirming scrolling with track playing no longer results in dropped frames 😊

[m0dB]

Excellent! @daschuer do you see any reason not to take this PR?

It is in fact a solution that is mentioned in the qt wiki: https://wiki.qt.io/Threads_Events_QObjects , so I think that fully legitimates it.

Forcing event dispatching

So, what do we do if we have a long task to run and don't want to block the event loop? One possible answer is to move the task into another thread: in the next sections we'll see how to do that. We also have the option to manually force the event loop to run, by (repeatedly) calling QCoreApplication::processEvents() inside our blocking task. QCoreApplication::processEvents() will process all the events in the event queue and return to the caller.

It is not exactly the same (QCoreApplication::processEvents would process the other events for the same tableview / table items, so that would not solve this issue, but moreover Qt didn't like that because "A paint device can only be painted by one painter at a time."), but a slightly more advanced version of the same idea.

I don't see a risk of regression.

[daschuer]

Does this issue disappear, when we apply a hack that removes all texts?

[daschuer]

The workaround in https://github.com/mixxxdj/mixxx/pull/11644 hides the issue, but the high CPU load is still there during scrolling, wasting CPU power that can be better used. Does one of you have interest to find the culprit is it the text rendering engine or the skins styling?

Currently every visible cell is rendered from the scratch when scrolling. So small inefficiencies have a big impact.

[m0dB]

I have revived https://github.com/mixxxdj/mixxx/pull/11644 . I don't know if we still want to take this for 2.4.0. While it is not critical for usability, I must say that it does make quite a difference when it comes to the impression you get from the app. The dropping frame rate is a bit embarrassing.

I agree that it would be better to fix the underlying issue, but I think that could be a lot of work. I have not analysed the performance bottleneck any further but I do remember that the text rendering definitely played a role. Not sure about the skins styling. (There is something that I noticed when working on other code: eliding makes text rendering very slow. This could be a low hanging fruit. The solution I applied there was rendering without eliding, and overlaying a fade-transparent-to-black gradient at the far right of the cell. Not perfect but it works quite well)

Something that might be interesting to explore (and probably a lot more attractive than spending time trying to fix this) is to replace the library view with a QQuickWidget and use QML for all the library management. Theoretically this should be possible. I don't know how far developed the library view is in the QML UI. But if we can make that work, we could improve both the QtWidgets and the QML UI as the same time (and it's the kind of gradual refactoring I am always advocating 😄). I guess @Holzhaus is the developer who is most familiar with the QML implementation?

Currently every visible cell is rendered from the scratch when scrolling.

But even if we can avoid that, with heavy scrolling it is still unavoidable that a lot of cells have to be rendered.

(I seem to remember that QTableView does do some optimisation with cell reuse. But it has been a while since I looked into this, so I could be wrong)

[m0dB]

On a side note, I implemented a proof of concept QML library browser some time ago, because I wanted to get familiar with QML's TableView and with QAbstractTableModel. It has drag&drop and ordered and resizable columns and threaded SQL querying. https://github.com/m0dB/qmlmixxxlibrary

[m0dB]

As I still had occassional graphical glitches with the approach used in https://github.com/mixxxdj/mixxx/pull/11644 (and, to be fair, because it was a bit of a hack), I decided to follow up on @daschuer 's suggestion to find the culprit. Profiling (using Instruments on macOS) pointed to QStyleItemDelegate::paint in general, in the call to drawText therein in particular (but not only).

https://github.com/mixxxdj/mixxx/pull/12093 replaces QStyleItemDelegate with QItemDelegate and draws the text manually with painter->drawText(...) (and without eliding, which makes a huge difference).

The improvement of huge. With very frantic scrolling I can still trigger some framedrops, but not anymore the reduction from 60 to below 30 fps.

@foss- could you give this a try and see if it works for you too?

What I couldn't get to work is the checkbox (lock icon) in the BPMDelegate. But let's discuss that in the PR. Maybe @ronso0 has more insights.

[foss-]

Thanks for your continued efforts. In a brief test with 2.4-beta-184-g60638be265 (HEAD) from https://github.com/mixxxdj/mixxx/actions/runs/6498808938 on macOS 13.6 (Intel mac) dropped frames do increase rapidly when scrolling:

Played shows as 0 or 1 as per your mention probably expected. Also not seeing the BPM lock icon.

[m0dB]

Thanks for testing, @foss- . Yes, it is expected that it doesn't render everrything correctly. And it is also expected that it doesn't solve the issue completely, but you should notice a difference compared with 2.4.

In any case, after talking with @ronso0 , I understand that replacing the styling for the table with something static is not really an option. But I do think that there is some low hanging fruit here: not eliding the text already makes a difference.

[ronso0]

But I do think that there is some low hanging fruit here: not eliding the text already makes a difference.

By simply not eliding we'd have no text overflow indicator anymore. I like your idea of replacing the elide ellipsis with a gradient (transparent >> bg color). Though that might be tricky since we'd need to adjust to alternating row colors :| And it's unclear if the required calculation is faster than eliding since it (probably) also involves QStyleOption->fontMetrics().

[m0dB]

IMO there is no calculation needed. Just always draw the gradient (a pre-rendered QImage is probably fastest?) at the right hand side of the cell. In the text is shoirt, it won't reach (be covered by) the gradient. If the text is long, the gradient fades it out, indicating it goes on outside the cell. Of course there is the corner case where the text is as long, or nearly as long, as the cell, but I think that's an acceptable trade-off.

But first let me try if only disabling eliding makes enough of a difference to be worthwhile.

And of course, in the end, while the drop in framerate during scrolling is ugly, it doesn't really effect usability as you are not looking at the waveforms anyway when you are scrolling.

[m0dB]

BTW, do you know how the alternating row colors are done?

[ronso0]

QAbstractItemView::setAlternatingRowColors done in WLibraryTableView ctor.

Though no idea where the actual painting is done.