Add sample thumbnails

[khoidauminh]

This is an attempt in adding sample thumbnails in order to speed up rendering (mainly) in the song editor and other places (currently being Audio File Processor, SlicerT and the Automation editor).

Brief summary of the PR:

• A Bit is a sample thumbnail, which is a struct containing max, min and rms.
• A Thumbnail is a vector of Bits and a ThumbnailCache is a vector of Thumbnails.
• The class SampleThumbnail contains a non-static member that is a shared_ptr to a ThumbnailCache, ~and the static member s_sampleThumbnailCacheMap.~ Both are private.

How this implementation originally avoided duplicates

s_sampleThumbnailCacheMap is an std::map with keys being the sample file path and the value being a shared_ptr to ThumbnailCache. All thumbnail caches are stored here.

This implementation assumes that a unique sample has a unique full file path. If this is violated or the file path is just the file name, the implementation may break completely.

When a sample is loaded into the song editor, the constructor of SampleThumbnail looks into s_sampleThumbnailCacheMap to find the thumbnail cache for this sample. If the sample is new, it proceeds to generate a new thumbnail cache for this sample and insert it into the map. In other places like AFP, SlicerT, when loading a preexisting preset or project, the thumbnail caches might not be generated until you open the plugin GUI, which triggers the paintEvents.

The SampleThumbnail class attaches itself to classes that uses thumbnails, such as SampleClipView. This ensures that thumbnail cache that is being used will have a use_count() of 2 or more (one in the std::map and others in the objects). A use_count() of 1 indicates the sample is out of use and the thumbnail list will be deleted. Cleaning operations is done when loading new samples or closing LMMS.

Currently the global map behavior, explained in the collapsed section above has been deprecated in favor of the (in development) sample cache PR #7058.

As of current, the the thumbnail size divisor is calculated based on the log2 of sample size to avoid too many thumbnails for large samples.

This PR now also makes use of partial rendering, which offloads some computation cost from zooming to other UI updates, such as scrolling, resizing the window, as it now only renders the areas not yet drawn.

[Rossmaxx]

This implementation assumes that a unique sample has a unique full file path. If this is violated or the file path is relative, the implementation may break completely.

Factory samples would like to have a word with you

[khoidauminh]

This implementation assumes that a unique sample has a unique full file path. If this is violated or the file path is relative, the implementation may break completely.

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)

[zachtyll]

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?

[khoidauminh]

@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):

https://github.com/user-attachments/assets/f862be19-3228-488c-aff6-a94557fe28fb

https://github.com/user-attachments/assets/f67260ce-1db5-4cca-acaa-9ad7313d33d7

The sample in this video is 11 minutes long:

https://github.com/user-attachments/assets/8c779a37-e36b-4d57-8f61-f8390a6eee2b

[sakertooth]

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.

[khoidauminh]

@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

[sakertooth]

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).

[khoidauminh]

Thanks for the commits @sakertooth ! Things look nice

I noticed a logic error in the code that selects which thumbnails to use. We instead want to select the smallest thumbnail that is larger than or equal to widthSelect. I fixed that in my commit just now.

Also renamed sampleLength to sampleViewLength in visualize() to make it more specific.

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).

Right now I'm thinking of slicing the sample into multiple regions. Let's call them tiles. Each tile has a fixed width of, for example, 512 pixels. Everytime a tile is in view but not drawn we'll start rendering that tile and mark it as done. We will not be splitting the QPixmap of the SampleClipView into multiple smaller QPixmaps, we just need to find a way to mark a tile. I'm thinking of using a boolean vector that simply tells whether a tile is drawn or not.

To do that we need a way to know how much of the SampleClipView is visible and where. However I haven't found out a way to do that yet. If you can help me or know someone who can, let me know

[sakertooth]

I noticed a logic error in the code that selects which thumbnails to use. We instead want to select the smallest thumbnail that is larger than or equal to widthSelect. I fixed that in my commit just now.

Oh yeah, my bad, forgot to invert the condition in the while loop :sweat_smile:

[sakertooth]

To do that we need a way to know how much of the SampleClipView is visible and where. However I haven't found out a way to do that yet. If you can help me or know someone who can, let me know

Qt provides a rectangle that we can use to paint only in the requested region inside their paint events. I think just configuring the outside code to only specify the visualization to be for that region is all that needs to be done here, so it should be simpler than the tile idea? Let me know how it goes if you come around making this optimization.

I'll try and profile the code to find any bottlenecks later.

[khoidauminh]

@sakertooth Thanks! I'll look into it soon

[khoidauminh]

Whoops, last commit is broken

[khoidauminh]

I think I've managed to make partial repaints work. Please give this a test

[sakertooth]

Hmm, still too slow when zooming (still a lot better than before though).

I have another optimization idea (that may not even need to worry about partial repainting possibly).

By default, Qt uses the Raster engine for drawing, which operates completely on the CPU. I think we should start looking into using OpenGL instead for drawing the waveform, but that seems way too out of scope here.

I believe before we weren't even caching the results like we are doing now, so I think this is a good first step, so maybe we can look at using OpenGL later.

Regardless of that, I have some thoughts:

• I don't know how I feel about the global map. Say you had two different samples on disk: sample A and sample B. sample A was loaded into LMMS and then stored in the global map. Now, say we copied sample B to sample A on disk (i.e., sample A = sample B in terms of its contents, but they both keep their paths intact). When sample A is loaded back into LMMS as another sample clip, it does not have the correct waveform that matches its contents. I tested to confirm this and this seems to be what happens. You can try it for yourself too.
  • What I suggest is to remove the global map until sample caching is implemented. This is because situations like the one explained above will already be considered and handled appropriately. All this PR would have to worry about is just caching the different resolutions of the waveform, even if we temporarily have to deal with duplicates until we figure something out.
• I don't like the functionality that goes around the thumbnail implementation and uses the original sample. Personally, I think we should have an implementation that doesn't go against the grain if possible and uses the old way of drawing that we know is subpar. Is there any way we can enforce the new thumbnail idea, while also ensuring it meets the requirements of whatever uses it in the program?
• Would Pixel be a better term for Bit? This is because the generate function seems to be getting the proper sample chunk to then merge into their corresponding pixel locations, that seems to make sense to me. I also think we shouldn't put a limit for the thumbnail size and have one of the thumbnails cover for the whole sample (which should address my previous point).

[khoidauminh]

@sakertooth Using OpenGL will be a great idea to accelerate the rendering even further, however I don't have any knowledge on GPU related programming, so for this one I'm gonna have to rely on external help.

For the global map removal, I've always been seeing it as a hacky and incomplete way to avoid duplicate. It's still being too problematic as mentioned, so I'll go ahead and remove it.

Also with the high bound on the size of the thumbnails, if memory cost can be tolerated, I can remove that too. With that visualizeOriginal is no longer needed and the implementation should look cleaner ye

I'm not sure about the use of Pixel since I've always understood it as some kind of a dot. I tried to use Line or Column before but they didn't sound right either? But if you think Pixel is a better term for the struct then I'm good with it

[sakertooth]

Also with the high bound on the size of the thumbnails, if memory cost can be tolerated, I can remove that too. With that visualizeOriginal is no longer needed and the implementation should look cleaner ye

Since just removing that bound will probably increase the number of thumbnails to an insane amount, you might also want to increase the divisor (or calculate it based on the length of the sample).

I'm not sure about the use of Pixel since I've always understood it as some kind of a dot. I tried to use Line or Column before but they didn't sound right either? But if you think Pixel is a better term for the struct then I'm good with it

I'm honestly not sure (I'm just realizing the thumbnails aren't generated to represent being in a box yet, but use only the samples, so if Bit works fine I'm okay with it).

[khoidauminh]

Since just removing that bound will probably increase the number of thumbnails to an insane amount, you might also want to increase the divisor (or calculate it based on the length of the sample).

Will keep that in mind, thanks!

[khoidauminh]

A few changes happened simultaneously in the commit so I'm gonna briefly go over them:

• Fixed the thumbnail selecting code so that it's only selecting the smallest larger thumbnail and not the largest thumbnail.

• I've implemented a QRect m_paintPixmapDrawnRegion member in SampleClipView to track which region has been painted, so regions that are already drawn will not be rendered again.

• The high bound on the thumbnail size is gone an the largest thumbnail will be the sample size divided by 4. I don't know if we would really need a full resolution thumbnail since we rarely need to look at individual samples in AFP or SlicerT. Let me know your thoughts.

• thumbnailSizeDivisor is now proportional to the log2 of the sample size.

• VisualizeParameter has an additional member allowHighResolution which determines if the visualize code is allowed to use the first (largest) thumbnail. This is true for SlicerT, AFP and Automation editor, and false for SampleClipView.

• The member pointer originalSample and function visualizeOriginal() is gone since we have the "High Resolution" thumbnail.

• ~The static member s_sampleThumbnailCacheMap and its methods are also gone. This coupled with the large thumbnails, if you have a project with many duplicates, both the loading time and the memory usage will shoot up pretty high, since each SampleClipView instance gets its own cache regardless of the samples.~ They've been added back but marked as deprecated until Sample Cache PR is ready.

[khoidauminh]

Also since LMMS has continuous scrolling now, it might be better if the update region is extended by an amount so that it renders a good chunk in advance.

I think this is still too early to remove the global map as it has been making project loading time painfully long. I'm gonna add it back for the sake of testing conveniences, then finally replace it when the new Sample Cache functionally is ready.

(Should I merge master into this branch to keep up with stuff?)

[sakertooth]

Also since LMMS has continuous scrolling now, it might be better if the update region is extended by an amount so that it renders a good chunk in advance.

That is true, but shouldn't the thumbnails be already rendered in full? When we continuous scroll, we only should have to update a couple lines, or am I mistaken?

I think this is still too early to remove the global map as it has been making project loading time painfully long. I'm gonna add it back for the sake of testing conveniences, then finally replace it when the new Sample Cache functionally is ready.

The high bound on the thumbnail size is gone an the largest thumbnail will be the sample size divided by 4. I don't know if we would really need a full resolution thumbnail since we rarely need to look at individual samples in AFP or SlicerT. Let me know your thoughts.

Okay, maybe you're right. My main goal was to just make all the functionality go through the new thumbnail mechanism, and it seemed like we needed that high resolution for the AFP and SlicerT (for reasons I'm not sure of, I was just basing it off of what I saw in the code), unless I misunderstood something.

I still feel like there are a couple of optimizations we can make though, but I need to work out an implementation first before I suggest anything here.

Edit: And yes, feel free to merge master.

[khoidauminh]

That is true, but shouldn't the thumbnails be already rendered in full? When we continuous scroll, we only should have to update a couple lines, or am I mistaken?

The thumbnails are fully generated, but only the visible region is rendered onto the QPixmap. During continuous scroll we only have to render a few lines but I think rendering a chunk in advance may be faster (so we can scroll for a while until we need to render another chunk), hence the extended update region. The code extends the region in both directions, but maybe it's more optimized to just extend to the right (I'll test and change it later).

My main goal was to just make all the functionality go through the new thumbnail mechanism, and it seemed like we needed that high resolution for the AFP and SlicerT.

We do need the high resolution, but we rarely need it to be at the original sample level, so the high resolution for now is the sample size downscaled by 4 (The high resolution is just another thumbnail in the cache lol) I did realize visualizeOriginal is redundant and kinda duplicate code so I thought it was better to remove it and place all future optimizations to just one drawing code.

[khoidauminh]

New stuff in the commit:

• The fields in VisualizeParameter had been really messy and convoluted to work with so I've encapsulated all the needed data into QRects and named them accordingly. See header file comments.

• I have some concerns about how we determine whether the clip needs updating. The single boolean member is too ambiguous. We don't know why we need to update the clip, is it that the clip got muted, selected, zoomed, the sample changed? etc... needsUpdate() also constantly returns true during scrolling, which is unnecessary in my opinion. I haven't found out where that is set. So in this commit I added m_muted and m_selected into SampleClipView as a temporary workaround until this can be improved.

[sakertooth]

I think I have a simple idea for how this functionality should work. I'm starting from scratch, but the idea still consists of same cache and generating thumbnails.

When a sample is loaded, a limited number of thumbnails will be generated for the entire sample. As an example, we can generate 3 thumbnails per unique sample: one for low, medium, and high resolutions (if we want more granularity, the maximum number of resolutions can be increased, but be sure to consider any performance implications that may arise from this).

After those thumbnails at different resolutions have been generated, we "pre-draw" those in QPixamp's using QPainter and store them accordingly. The dimensions of the QPixmap's that we pre-draw in can be some sane values (ideally based on how long the sample is).

Now, when someone wants to draw a waveform with a certain sample, we will have a function that give us a rectangle to draw in, and we return the QPixmap that can be drawn into that box to give the waveform they are looking for. The QPixmap returned depends on the width of the box, so if the user was drawing in small, wide, and very wide boxes, we would return the low, medium, and high resolutions respectively.

However, before we return them, theres a likely chance the QPixmap won't be at the exact size necessary (since the box they were drawn in had some default dimensions, and most definitely not the dimensions we need). This is where we make a copy of that QPixmap and scale it just enough so that it fits the box, while also representing the expected detail the waveform should have.

The thumbnails and the QPixmap's could then just be stored in the cache map, where the cache would store struct's, each containing the thumbnails and QPixmap for now.

Let me know if you think this idea is a good one or not. I feel like the drawing of a QPixmap is a lot cheaper than drawing every line in a thumbnail individually, even if its one specific region, and so for simplicity we might be able to get away with redrawing the entire waveform.

I have some concerns about how we determine whether the clip needs updating. The single boolean member is too ambiguous. We don't know why we need to update the clip, is it that the clip got muted, selected, zoomed, the sample changed? etc... needsUpdate() also constantly returns true during scrolling, which is unnecessary in my opinion. I haven't found out where that is set. So in this commit I added m_muted and m_selected into SampleClipView as a temporary workaround until this can be improved.

Is this a problem because we are drawing only the necessary regions? If it is causing too much complexity, I think redrawing the waveform using a QPixmap can simplify this process. It should cheap enough to just draw it as is.

[khoidauminh]

@saker I think it's a working solution. I can see some advantages in this:

The drawing code should be more minimal since these have been abstracted away in QPixmap itself.

In the case of a sample appearing in the song editor as well as AFP/SlicerT, we can use some color manipulation. The QPixmap can be grayscale (or palette indexed even, if it's possible), then when we copy the QPixmap over we multiply with the desired color (or apply the desired palette).

Clips using the same sample in the song editor can copy from the same QPixmap and not have to redraw.

Though there are some cons of this (I believe):

As you've mentioned, we limit the amount of QPixmaps generated since the memory cost will be much higher. QPixmaps also have a size limitation (32767x32767), though the clip itself will break anyway before we get to draw on them.

Lost Robot mentioned in the discord that if we don't properly scale the thumbnail into the clip, we'll lost details. I'm wondering if the issue will happen if we scale from the bigger QPixmap into the smaller Rect. If we choose to always scale from a smaller Qpixmap, I think at some point the clip will look very blocky until we jump Qpixmaps resulting in the quality jump.

Is this a problem because we are drawing only the necessary regions? If it is causing too much complexity, I think redrawing the waveform using a QPixmap can simplify this process. It should cheap enough to just draw it as is.

~~I think it'll still be expensive if we draw out of bounds or redraw the regions in clip already in view. Though we can improve some of that by tracking which region has already been drawn, limiting the painting region, and only explicitly repaint when mute, select and color changes (which I'm doing in this PR).~~ Giving up trying

If you have more thoughts, let me know. I'm gonna clone this branch and implement the new ideas on it.

[sakertooth]

Hi @khoidauminh, I believe I have implemented an ideal way to solve this problem. I'll make a new PR soon and share more details then.

[khoidauminh]

Nice! Looking forward to it

[sakertooth]

So..., I did have an idea for how it should be implemented, but since there were so many issues with my implementation and this PR has good accuracy and performance already, there was no point in me branching off to continue with my own approach (it was already very similar to what was done here anyways).

It's a very hard problem since there is really no documentation or details on how DAWs should really go about doing something like this, but I think what we have here is okay. I completely forgot that the implementation was already at such a good standard and I think all that is left is cleaning things up a bit. Though, at least now I have a better understanding of what was done in this PR from what I learned.

We can delete SampleWaveform.cpp and remove it from the src/gui/CMakeLists.txt file since it is no longer being used. I'm also considering solidifying the waveform cache as its own separate, official class (with proper detection of file changes so we know when to regenerate the thumbnails), instead of it currently being shown as deprecated in the code.

[khoidauminh]

@saker About the deprecated part, we do plan to leverage the sample cache PR to detect sample changes and duplicates, so I think we can drop it once the PR gets merged. Although, if LMMS has a mechanism to deal with sample changes already, we can try to call update where it is done too.

The rest sounds good to me

[sakertooth]

@saker About the deprecated part, we do plan to leverage the sample cache PR to detect sample changes and duplicates, so I think we can drop it once the PR gets merged. Although, if LMMS has a mechanism to deal with sample changes already, we can try to call update where it is done too.

I was thinking we could separate it from the actual sample cache. This is because we are looking to enforce the split between the GUI and core code, and sample thumbnails are with the GUI. So if we were to cache it with the actual sample cache, the split wouldn't be there. Apologies if I mentioned that we should just use the actual sample cache previously, this just came to mind as I continued to think about the problem recently.

Though, I'm open to objections or any other thoughts you have about this.

[khoidauminh]

@sakertooth So if I'm understanding it correctly, the globalThumbnailMap code should be in its own class, and the SampleThumbnail class calls into it to get a reference to the thumbnails. Any sample change and ~~thumbnail (re)generation~~ is managed by this new globalThumbnailMap code. Whenever a sample change happens, the new class marks a thumbnail as deprecated and the drawing code regenerates the thumbnails before using it