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). |
|
Glad to see it, honestly! Very little use for it, IMHO as well. This looks merge-ready to me, then, but give me another day or so to test it a bit more and write up a little report for myself regarding the long waveform rendering issue. |
|
@khoidauminh Is there a posibility for a large enough sample to be loaded into SlicerT and AFP for it to not render some part of it? I could test that, but if you know the properties of the thumbnails used for the plugins, I could generate a properly sized audio file to test that edge case. |
|
I exported the sample from #7366 (comment) looped 3 times:
AFP is dealing with the file wonderfully!
SlicerT as well! I would still believe there is a large enough sample size for the thumbnail to not be able to fit it all into the view? Seems like no sane person is gonna reach that, so we should be in the clear, at least until there is a valid reason to make changes. |
|
So, the only thing to note is that there is an existing issue of samples not being representable in their entirety on master, if they exceed draw limitations. #7366 (comment) #7366 (comment) IMO this is merge-ready, but please have some dev test and review the code as well, maybe Saker or Dalton. Will take another look tommorrow, just to be safe, since that also gives devs time to review. |
|
Just checked the end of the views, everything is fine! |
The same issue happened before my commit where the loop end point was met earlier than it should've and stopped the rendering midway, but it should be fixed now. |
I also agree that we should deal with this as soon as the PR get merged, or at least open a tracking issue, before the feature makes it way to stable to avoid confusion/false reports. |
|
I worked with the SampleClipView code for a bit and managed to get it to draw without garbage! The sample below is a one hour long track.
The downside is that this requires a repaint call for some interaction that draws over the clip, like moving the playhead around.
But this is a great start. I don't think I should delay merging the PR since this is a hack, I still need to test this, and there are new behaviors, so I plan to discuss this somewhere else, like Discord or in a new PR. |
|
@sakertooth @bratpeki I've pushed the potential fix to a new branch in my fork. Come test it out In a nutshell, I made the m_paintPixmap the size of the update region, rendered the waveform and other stuff on it, then painted it on the clip at the appropriate coordinates instead of at (0, 0). That coordinate is the topLeft() of the update region, obtained when we do a complete redraw (needsUpdate() == true). |
Correct. Here's the issue for long samples cutting out visually. #3378 |
I've pushed more code improvements to the garbage rendering fix. Things are going nicely. Hopefully I can open a new PR soon after this one. I do have a few small improvements to the current PR, but since it's being reviewed, I can offload it to the new one |
|
@khoidauminh I think if the garbage rendering fix doesn't have to branch off of this PR, we should do it starting from |
I plan to make a new PR which should be based on master. But if the new code is well tested and stable early, I can try to get it merged here as well. |
|
I would also prefer a new PR 👍 |
There was a problem hiding this comment.
Had to make one last adjustment, but I believe this PR is merge-ready as well. I have ideas on how to improve the caching mechanism a bit (it follows what @messmerd had in mind, where we map UUIDs to file paths and give clients a single UUID. I can see the benefit of doing this now unlike before.), but I think there would be little to no harm merging this now.
|
Testing today, writing everything up, and we can merge! 🙏 |
|
Got the MSVC Windows build and everything LGTM! 😎 This PR also helped me check for another issue, that being that when the clip end is out of the visible part of the Song-Editor, moving it show its tooltip where the end of the clip is:
I'll open an issue, if one isn't already up. TYSM for this, @khoidauminh and @sakertooth! If this passed all the reviews, please merge this ASAP and we can take a look at @khoidauminh's experimental branch and see if that's a possible solution to out current long waveform thumbnail drawing problem! If not, @sakertooth, please review or have Messmerd review, since I see he's listed as a reviewer! |
|
Thank you for your help with this PR everyone! 🎉 I will rebase my other branch to master soon |
| if (sample.sampleSize() == 0) { return; } | ||
|
|
||
| const auto fullResolutionWidth = sample.sampleSize() * DEFAULT_CHANNELS; | ||
| m_thumbnailCache->emplace_back(&sample.buffer()->data()->left(), fullResolutionWidth, fullResolutionWidth); |
There was a problem hiding this comment.
&sample.buffer()->data()->left() is the same as sample.buffer()->data()->data(). Both are a pointer to the interleaved (i.e. LRLRLRLR layout) buffer.
There was a problem hiding this comment.
I've pushed the change to the the follow up PR #7695, you should do new reviews over there too
This PR attempts a number of improvements to the sample rendering (in the song editor, automation editor, AudioFileProcessor, SlicerT, etc) 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. * Experimental sample thumbnail * Rename some classes and type aliases. Make some type declarations explicit * Use a combination of audioFile name and shared_ptrs to track samples; refactor some loops * That weird line at the end of the sample is now gone * Small changes to the code; Add comments * Add missing word to comment; Fix typo * Track `SharedSampleThumbnailList`s instead * Major refactor; implement thumbnailing for SlicerT, AFP and Automation editor * Code clean up, renames and documenting * Add the namespace lmms comments * More code updates and documentation * Fix error in comment * Comment out `qDebug`s * Fix formatting * Use alternative initialization of `SampleThumbnailVisualizeParameters` * Remove commented code * Fix style and simplify code * Use auto * Draw lines using floating point * Merge the classes into one nested class + Replace while loop with std::find_if * Fix comparison of different signedness * Include memory header * Fix a logic error when selecting samples; Rename a const * Fix more issues with signedness * Fix sample drawing error in `visualizeOriginal` * Only render regions in view of the sample * Allow partial repaints of sample clips * Remove unused variable * Limit most of the painting to the visible region * Revert back to using rect() in some places * Partial rendering for AutomationEditor * Don't redraw painted regions; allowHighResolution; remove `visualizeOriginal`; Remove s_sampleThumbnailCacheMap * Add s_sampleThumbnailCacheMap back for testing convenience * Minor change to the way `thumbnailSizeDivisor` is calculated * Extend update region by an amount * forgot about this * Adapt to master; Redesign VisualizeParameters; Don't rely entirely on needsUpdate() * Don't try to preserve painted regions * Allow for a bit more thumbnails; Fix incorrect rendering when vertically scrolling * Fix missing include statement * Remove the unused variable * Code optimization; Remove RMS member from Bit; Rename viewRect to drawRect * More code optimizations * Fix formatting * Use begin instead of cbegin * Improve generation of thumbnails * Improve expressiveness of the draw code * Add support for reversing * Fix drawing code * Fix draw code (part 2) * Apply more fixes and simplifications * Undo some out of scope changes * Remove SampleWaveform * Improve documentation * Use size_t for some counters * Change width parameter to be size_t * Remove temporary aggregated peak variable * Bump up AggregationPerZoomStep to 10 * Zoom out only requested range of thumbnail instead of clipping it separately * Rename targetSampleWidth to targetThumbnailWidth * Handle reversing for AFP; Iterate in reverse instead of reversing the entire thumbnail * Change names to be more accurate * Improve implementation of sample thumbnail cache map * Move AggregationPerZoomStep back down to 2, do not cap smallest thumbnail width to display width To improve performance with especially long samples (~20 minutes) * Simplify sample thumbnail cache handling in constructor * Call drawLines instead of drawLine in a loop 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. * Bump up AggregationPerZoomStep to 10 again 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. * Fix off-by-one error when constructing Thumbnail from buffer * Fix crash when viewport is not in bounds * Apply performance improvements 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. * Apply minor changes Update copyright and unused functions. Move in newly created thumbnail into the cache instead of copying it. * Use C++20's designated initializers * Create param right before visualizing * Fix regressions with reversing * Fix incorrect rendering in AFP and SlicerT * Move MaxSampleThumbnailCacheSize and AggregationPerZoomStep into implementation file * Remove static keyword * Remove getter and setter for peak data --------- Co-authored-by: Sotonye Atemie <sakertooth@gmail.com>
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.