Improve waveform rendering performance #7366
Conversation
… refactor some loops
Factory samples would like to have a word with you |
As long as the factory sample paths remain unique for each sample (which I believe they do) then it won't break. I'm mostly concerned with when the sample name is just the filename. (also changed the PR description a bit) |
|
You've got some code that is commented out, is it supposed to stay or to be erased? Since this change is also visual, could you provide a screenshot of what it looks like? |
|
@zachtyll Oh right they were supposed to be removed but I forgot to. Will make a commit to clear them out Here are some pictures and videos (left is the old code and right is the new code):
slicert.mp4samples2.mp4The sample in this video is 11 minutes long: automation.mp4 |
|
I see a number of style issues/some code that can be cleaned up. Instead of making a huge review for style, do I have permission to go in and fix it myself? I want to focus on the implementation here (the design/visualization code), not too much of the style, but its an important problem here still. Edit: But if you would prefer a review, then I don't mind. |
|
@sakertooth Feel free to push commits to adjust the style. For the implementation adjustments, I think it can benefit from a bit of discussion/reviewing before commits are pushed |
+ Replace while loop with std::find_if
|
Hey @khoidauminh, I just pushed some style changes, along with some fixes and rearrangements. Let me know if you have any objections with them. I plan to look into this PR more closely soon. Edit: Forgot to mention, one thing I noticed is that there is still a lot of lag when the number of sample clips get around 20 and up and each are around 2 minute long. I think another optimization we might consider is just drawing the necessary region, and not the entire thumbnail (not sure if that will fix the issue, but it was something I had in mind). |
|
I'm eyeing these two lines: lmms/src/gui/SampleThumbnail.cpp Line 146 in 3dd6217 lmms/src/gui/SampleThumbnail.cpp Line 150 in 3dd6217 Is there a performance impact on these? I think if we take an iterator-based approach, we could have almost zero performance cost. I know exactly how to do this cleanly in Rust but in C++ I have no idea. LMMS just moved to C++20 so I think this should be possible. Will take a look at the C++ references. |
You're right. We don't need to copy anything at all and we can take an iterator-based approach to represent the range of samples to consider. Iterators in C++ are the In most cases however, it is simple to consider Sidebar, but just as another good C++ reference, the guidelines are really good. |
… entire thumbnail
…nail width to display width To improve performance with especially long samples (~20 minutes)
QPainter::drawLine calls drawLines with a line count of 1. Therefore, calling drawLine in a loop means we are simply calling drawLines a lot more times than necessary.
Thought using 2 would help performance (when rendering). Maybe it does, but it's still quite slow when rendering a bunch of thumbnails at once.
|
Okay, I build this! Now... how exactly does one test this? LMAO! |
Load a large sample and see if you can view it without much lag while scrolling in the song editor. There are some crashes/performance issues I still want to address, but any testing as of right now will be appreciated. |
|
I tested it, and it does seem to be much much faster than normal sample drawing, nice! It does crash when you try to scroll outside of a sample, but I think you already mentioned that. I feel like I am able to tell when the different thumbnails change based on when the sample suddenly feels denser as you change the zoom, but it's not very noticeable. |
Performance in the zoomOut function was bad because of the dynamic memory allocation. Huge chunks of memory were being allocated quite often, casuing a ton of cache misses and all around slower performance. To fix this, all the necessary downsampling is now done within the for loop and handled one pixel after another, instead of all at once. To further avoid allocations in the draw call, the change to use drawLines has been reverted. We now pass VisualizeParameters by value (it is only 64 bytes, which will fit quite nicely in a cache line, which is more benefical than reaching for that data by reference to some other part of the code). After applying these changes, we are now practically only bounded by the painting done by Qt (according to profiling done by Valgrind). Proper use of OpenGL could resolve this issue, which should finally make the performance quite optimal in variety of situations.
khoidauminh
changed the title
Update copyright and unused functions. Move in newly created thumbnail into the cache instead of copying it.
|
For me, LMMS crashes any relatively large sample, independent of if you stretched the clip to cover the full sample. This is making testing rather tedious and, if possible, it would be nice to address that first, apply the fix as a diff to this PR, and test again. This is, of course, only if we know what's causing the crash. |
I wonder if you have pulled the most recent commits? |
|
No, I just cloned this and built it. Perhaps the PR author can update the branch to match the upatream. |
|
Additionally, I've recently spoken with Lost about it, it seems to be in master as well, but I'll check when/if the PR is updated. |
|
I'll take a look soon. |
|
I unfortunately don't think I can reproduce @bratpeki. Can you share any more information about the crash (any logs, maybe how long the sample clip was, etc)? |
This PR attempts a number of improvements to the sample rendering (in the song editor, AFP, SlicerT and Automation editor) in LMMS:
Thumbnails: Samples are aggregated into a set of thumbnails of multiple resolutions. The smallest larger thumbnail is then chosen for rendering during sample drawing. Each set of thumbnails is stored with its sample file metadata in an
unordered_map, so that duplicate samples will use the same set of thumbnails.Partial rendering: additionally, this PR only renders the portions of the sample clips visible to the viewer to save rendering time.
Fixes #7576.
Fixes #7546.