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

slicert.mp4

samples2.mp4

The sample in this video is 11 minutes long:

automation.mp4

[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]

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

[sakertooth]

@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

I don't want to talk in terms of absolutes because I think there a couple of ways we could go about this. I'll try to work out an implementation and let you know what I come up with.

[sakertooth]

My current plan is to keep the regular sample cache and the sample thumbnail cache separate. The code in the core will only need the data stored in the regular sample cache, and generating that data alongside it is unnecessarily coupled. The sample thumbnail cache only concerns the visualization in the GUI, so it makes more sense to put it there.

So this PR will probably have to wait for sample caching to be implemented, as my idea is that the sample thumbnail cache will request data from the regular sample cache (everybody that needs a sample, for just playback, visualization, etc, will be requesting from there).

In terms of it being a separate class, I think the sample thumbnail cache can still reside in the private section of SampleThumbnail since there is no other class that will be accessing that cache, so there's no need for it to have public visibility.

[khoidauminh]

Ah, so the sample thumbnails itself still reside in the GUI, and request data from the Core. Got it

How should we decide when to signal the thumbnails to be regenerated? Do we do passive updates, which is regenerating when we need to redraw, or active updates where the Core calls into the thumbnails to regenerate as soon as there's a sample change? I do have some ideas for the passive updates similar to the needsUpdate() in SampleClipView, but idk about active updates yet.

[sakertooth]

The callers should be responsible for updating the thumbnails on sample changes. This way they are only regenerated when needed per use.