-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Follow up] Improve performance when rendering sample waveforms #7695
base: master
Are you sure you want to change the base?
Conversation
…ix lag on AutomationEditor
I also noticed there's some unfinished business I have with the draw and generation code possibly. I want to follow up with that too in this PR if that's okay with you. |
Ye sure @sakertooth Btw there are some changes regarding the VisualizeParameters so check that out first |
Glad to see this! @khoidauminh, could you load any samples with a low pitch (kicks, toms, basses, etc) and zoom in on the waveform? I tried this in OFP and it generates a fill on the part of the waveform below 0! Must've missed checking short samples in my testing, as I was so focused on long ones! 🤣 |
@bratpeki I think I know what the issue is. Originally the Peak struct min and max default values are +infinity and -infinity respectively But in a commit it was changed to +infinity and 0 (::max() and ::min() in source code), so max gets stuck at 0 when the waveform sits below 0, making a fill Simply change the min() back to -max() and it's all good (currently away from laptop so can't do yet) |
|
We might want to use |
After that fix has been applied, is this ready for testing? I view it as an extension of 7366, so it only makes sense to take a look at it as well. |
…of Peaks respectively
Hi @bratpeki and @khoidauminh, I pushed a change that uses If you need my assistance with anything though let me know. Feel free to test the new commits @bratpeki. |
Is it just me or the |
Alright! |
You also have to consider that caches are generally small to avoid keeping unnecessary, unwanted data, keeping it "hot" basically. Too big of a size and you run the risk of increasing the memory usage, even when in some cases evicting a thumbnail to store a new fresh one is better than keeping the old one around. That being said, I'm more than happy to help come up with a better size that will better fit the average workload and system. If we have more than 32 thumbnails, there are two things that can happen if another thumbnail is requested to be generated: one is a cache miss if the thumbnail is not cached, the other a cache hit if it is. The cache will only keep regenerating new thumbnails if every request for a new thumbnail after meeting that size limit wasn't cached. Sidebar: We might also want to consider renaming |
Ideally, this idea will be extended to all resources, like sample resources and thumbnail resources, and maybe more (essentially anything that is costly to regenerate and store a lot of all at once is a resource). |
@sakertooth Looking at the code, from my understanding, when the cache gets full, the unused thumbnails will be destroyed, but the least still used thumbnails will be detached instead. Existing clips will hold on to this detached cache until all of them are destroyed. So I'm guessing it's not as bad as I'm concerned. All good then |
I guess it should be using an LRU eviction policy actually. I wont stop you from making changes, but there are plans to overhaul resource management in the future anyways as mentioned. Maybe since its really needed I might start work on it soon. |
Before I test, I'd like to know what to test for!
Could you elaborate on this? Are you reffering to the black space I was showing in my screenshots 7636? |
Right, so I should expect the things listed below, and if they're accomplished, this has been tested?
|
@bratpeki Yes, they have been tested. I have tried the following: zooming in, scrolling, overlaying another clip on the sample and zooming, overlaying a window and zooming, moving a sample (with and without an overlay). They should be fixed. But do test it anyway in order to catch more potential hidden bugs. There's a behavior when the viewportRect doesn't contain the start of the clip (when it's outside of view or overlayed), the border will draw anyway and the sample filename will move around it until the clip start appears again. Screencast_20250214_071652.mp4In the view you'll also see that the sample gets shifted by the border size. I will need to do something about it.
|
Hello, a user was facing an issue with a "blurry" waveform. I believe this is the antialiasing I added to make the waveform look smoother. In this follow up, we might consider either removing it, or making it an option you can toggle. I did add the antialiasing on a whim I suppose. I thought it looked better, especially when zooming in the AFP, but we might make it a toggle. How do you all feel about the antialiasing? |
I personally don't have a problem with it. But since the thumbnail we select to render is always larger than the clip size, I'm sure we won't run into a "blocky" appearance issue when we turn it off. So it's safe to remove it |
I, too, am pretty indifferent, at least for now. I'd have to load in a lot of samples at different zoom levels to check which works best, so in the meantime it might be best to make it a toggleable option. |
Cool, then, I'll test the things I listed! As for the clip overlap issue, I don't know, that's too Qt-technical for me to think about. (Side note, but) I don't even think you can push a clip of your choosing to the front. If it can be solved so the text is always at the start of the sample, independent of clips that overlap with it, that's great! If not, we can name that as an issue for a future PR to fix in the final report of this PR. |
Hmmm, I'll try to crash lmms locally. Finding a few hour long samples to concatenate |
Using a 4 hour long file:
Haven't got a crashed yet |
I believe the crash mainly happened the moment I tried playing the track. |
I played the sample at random places but still couldn't get a crash. I'm using Archlinux so it probably disables memory protection measures or I somehow turned them off. What if you try to reproduce this in a fresh system? |
Sometimes when it says "Killed", it might not signify that LMMS has crashed, but something else on the system killed it. I don't know if using an AppImage changes whether or not it prints "Segmentation fault (core dumped)" instead though. @bratpeki, I'm curious if you could try a compiled build and see if anything changes. |
GDB didn't assist too much:
The issue happens when attempting to play with this really large file loaded in. It creates an infobox with messed up drawing and exits: I still believe this is a fix for a future PR, though, and if @khoidauminh would be interested in investigating this, I would be more than greatful! |
When loading samples which take a bit of time, that infobox says "Sample loading/This may take a bit of time..." Sometimes, this info is displayed properly, sometimes not. Can anyone replicate this? |
2025-03-01.14-04-12.mp4A little video demonstration. |
In any case, as this is also on the nightly, it's up to a future fix!
Both fixed! |
@sakertooth, please take a look and merge if all is well. I'll open the corresponding issues on the latest nightly once this is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look fine. Will test soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Garbage rendering was fixed, thumbnails that exactly matched the original sample data are no longer generated, and some more. Fine with me. Thanks @khoidauminh for the PR and @bratpeki for the testing. We should be good if there isn't any other issue we need to address.
Garbage rendering is on master for me, so there's nothing left to address! Please merge 🚀 |
Can we consider this issue fixed @khoidauminh? The issue specifically mentioned sample clips so I think it is safe to make that assumption. |
@sakertooth We still have glitched rendering with other clips so I don't think we can consider the entire issue fixed yet. I was thinking about making an attempt to fix the remaining ones once this PR is merged. I suppose @zonkmachine can modify #3378 to track which types of clips have been resolved. |
I can do that. |
Could you elaborate on that a bit? From my understanding, this generates the clip thumbnail based on how much clip is currently visible, so there being garbage rendering is a bit confusing to me. 😅 |
@bratpeki This PR fixes the garbage rendering for sample clips, but midi, pattern and automation clips still suffer from it |
I see. It's out of scope for this PR, though, no? |
@@ -105,6 +103,7 @@ class LMMS_EXPORT SampleThumbnail | |||
|
|||
Thumbnail zoomOut(float factor) const; | |||
|
|||
Peak* data() { return m_peaks.data(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added this, but can you make it return const Peak*
for me instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait nevermind I see there was an issue somewhere. Let me review a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dereferencing logic was solid, it's odd how it appeared. I think it must be some problem with the bounds. I got this crash too I believe so I'll confirm if the fix works or not.
Yeah, most likely. It can be dealt with later in a PR with a clear focus on fixing it for the other clip types. |
How hard would it be to add tests for sample thumbnails? To minimize the chance of any bugs and regressions. |
Do you mean testing in general or automated tests? I usually test in 4 places: Song editor, AFP, SliceT and Automation editor. Actions usually include, zooming all the way in, reversing, sliding, scrolling, overlaying another clip/window on top and trigger repaints, etc TBH, I think the best way to test this is to roll it out to master, where it's exposed to all nightly users. The master branch is for development in general, but it's definitely used catching bugs/regressions as well. |
I mean automated testing. Our testing suite is really lacking right now, and with more features being added without tests to cover our mistakes, it's the equivalent of just winging it and hoping there's no problems. |
I don't even think we can work towards a stable release without tests. |
I think that the dereference bug could've been caught in an automated testing suite, as long as it's comprehensive enough. That way, if new changes are applied, it will always check against it, whereas with manual testing the tester may not be interested in doing a comprehensive test if the change seems localized in one place, and they don't consider potential side effects. How can we reach a stable release if we are dealing with regressions all the time? The problem is we could check off that a bug was fixed, but when another PR comes around and brings the bug out of dormancy again, it's basically whack-a-mole software development. |
@sakertooth I'd recommend opening a project for automated tests and solving it one issue at a time. Hell, add every single one of them to the 1.3 milestone. |
This PR adds additional changes on top of #7366:
Details
In the old code, `m_paintPixmap` in `SampleClipView` is allocated to the size of the whole clip, and painted at (0, 0) no matter what. On large samples, when the width exceeds 32768 (QPixmap's resolution limit), garbage data will be shown.In this PR,
m_paintPixmap
is allocated to the paintEvent's size (but with the clip's height), and painted onto the clip at the paintEvent's X coordinate. To make sure the m_paintPixmap doesn't get drawn on the wrong coordinate (due to something overlaying the clip and changing the paintEvent region), we paint on the coordinate obtained from the last full redraw.This change allows some fields of VisualizeParameters to be omitted (drawRect).
Fixes lagging in Automation Editor by specifying the viewportRect.
(Should) fix crash in AFP when the entire sample is displayed (Crash upon opening Audio File Player's GUI #7723).
Simplifies and enhances the implementation.
Should resolve the sample clips part of #3378