Add sample thumbnails #7366

khoidauminh · 2024-07-04T14:57:15Z

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 exactly 10.

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.

Fixes #7576.
Fixes #7546.

…licit

… refactor some loops

…n editor

Rossmaxx · 2024-07-04T15:00:44Z

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 · 2024-07-04T15:04:11Z

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 · 2024-07-11T20:23:48Z

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 · 2024-07-12T03:32:10Z

@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 · 2024-07-17T09:17:31Z

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 · 2024-07-17T13:22:21Z

@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

sakertooth · 2024-09-10T13:37:43Z

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

src/gui/SampleThumbnail.cpp

sakertooth · 2024-12-28T08:26:05Z

Pushed two commits: one fixes formatting, and the other changes the use of cbegin to use begin instead (begin can work in const contexts)

khoidauminh · 2025-01-03T14:46:27Z

I'm eyeing these two lines:

lmms/src/gui/SampleThumbnail.cpp

Line 146 in 3dd6217

if (parameters.reversed) { thumbnail.reverse(); }

lmms/src/gui/SampleThumbnail.cpp

Line 150 in 3dd6217

thumbnail.clip(finerThumbnailBegin, finerThumbnailEnd);

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.

sakertooth · 2025-01-03T19:42:14Z

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 begin and end functions in Containers (see what classifies as a Container here).

In most cases however, it is simple to consider begin and end iterators as just pointers to the first element and one past the last element in a container respectively.

Sidebar, but just as another good C++ reference, the guidelines are really good.

…arately

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

messmerd · 2025-01-06T18:43:08Z

src/gui/editors/AutomationEditor.cpp

+			auto param = SampleThumbnail::VisualizeParameters{};
+			param.amplification = sample.amplification();
+			param.reversed = sample.reversed();
+			param.sampleRect = QRect(startPos, yOffset, sampleWidth, sampleHeight);
+			param.sampleStart = static_cast<float>(sample.startFrame()) / sample.sampleSize();
+			param.sampleEnd = static_cast<float>(sample.endFrame()) / sample.sampleSize();


You could use C++20's designated initializers here if you'd like. And also in a few other places where you create a VisualizeParameters object.

I'm not sure I understand the immediate benefit in doing that, but I agree we could use it here probably.

messmerd · 2025-01-06T18:58:43Z

src/gui/SampleThumbnail.cpp

+		const auto beginSample = buffer + static_cast<size_t>(std::floor(peakIndex * m_samplesPerPeak));
+		const auto endSample = buffer + static_cast<size_t>(std::ceil((peakIndex + 1) * m_samplesPerPeak));


I'm not familiar enough with the logic to know whether the beginSample and endSample pointers will always be within the bounds of the array. Maybe you could add an assert to ensure that endSample < buffer + size?

I think its worth understanding it so we know for sure. The maximum number peakIndex will reach is width - 1. Therefore, endSample = buffer + ceil(width * m_samplesPerPeak) = buffer + ceil(width * size / width). width is an integer type so they cancel out safely, making endSample = buffer + ceil(size). Since size is an integer type as well, endSample = buffer + size when peakIndex = width - 1.

The problem here is that I shouldn't be accessing buffer viewing it as a bunch of SampleFrames. In the scenario I explained above, endSample points to at most one after the last SampleFrame in the buffer, making endSample->left() invalid. Instead, I need to be viewing the buffer in a flat manner (i.e., disregarding the notion of a left and right channel and accessing the float values directly.)

Or, just minus endSample by 1 and change endSample->left() to endSample->right() + 1.

In summary, the assert(endSample <= buffer + size) will always pass, but std::minmax_element can (and probably will) access invalid data.

messmerd · 2025-01-06T19:15:41Z

src/gui/SampleThumbnail.cpp

+		const auto it = s_sampleThumbnailCacheMap.find(entry);
+		if (it != s_sampleThumbnailCacheMap.end())
+		{
+			m_thumbnailCache = it->second;
+			return;
+		}
+
+		if (s_sampleThumbnailCacheMap.size() == MaxSampleThumbnailCacheSize)
+		{
+			const auto leastUsed = std::min_element(s_sampleThumbnailCacheMap.begin(), s_sampleThumbnailCacheMap.end(),
+				[](const auto& a, const auto& b) { return a.second.use_count() < b.second.use_count(); });
+			s_sampleThumbnailCacheMap.erase(leastUsed->first);
+		}
+
+		s_sampleThumbnailCacheMap[std::move(entry)] = m_thumbnailCache;


The s_sampleThumbnailCacheMap accesses aren't thread-safe. Could that be a problem?

No, not at all. Visualization only happens on the UI thread. We also only create SampleThumbnails on the main thread.

messmerd · 2025-01-06T19:22:50Z

include/SampleThumbnail.h

+		Peak* peaks() { return m_peaks.data(); }
+		const Peak* peaks() const { return m_peaks.data(); }
+
+		int width() const { return m_peaks.size(); }


Should this return std::size_t?

Since the math involved uses int quite often (stuff like std::max(rect->x(), thumbnail->width()), making width a size_t is more of an inconvenience than anything else, though one could argue that returning size_t is more correct here. Using std::max<int> is an option, but I don't like the possible implicit conversion (which had caused bugs in the past relating to sample waveform visualization)

khoidauminh and others added 13 commits July 2, 2024 08:41

Experimental sample thumbnail

5ad7845

Rename some classes and type aliases. Make some type declarations exp…

795ad7f

…licit

Use a combination of audioFile name and shared_ptrs to track samples;…

140006b

… refactor some loops

That weird line at the end of the sample is now gone

14d1e91

Small changes to the code; Add comments

71284b0

Add missing word to comment; Fix typo

4553283

Track SharedSampleThumbnailLists instead

7a6e2aa

Major refactor; implement thumbnailing for SlicerT, AFP and Automatio…

f281961

…n editor

Code clean up, renames and documenting

e03dacd

Merge branch 'LMMS:master' into sample-thumbnail

4a1689e

Add the namespace lmms comments

82c51e0

More code updates and documentation

35589f9

Fix error in comment

92b9c15

khoidauminh added 3 commits July 5, 2024 11:21

Comment out qDebugs

fa24cfe

Fix formatting

101199e

Use alternative initialization of SampleThumbnailVisualizeParameters

b903d9f

Remove commented code

7a393ef

sakertooth added 4 commits September 10, 2024 07:33

Fix style and simplify code

4e39c16

Use auto

9880d83

Draw lines using floating point

69cc6ba

Merge the classes into one nested class

d7f89ac

+ Replace while loop with std::find_if

sakertooth added 2 commits September 10, 2024 09:43

Fix comparison of different signedness

9605dec

Include memory header

7f3d43f

sakertooth added 2 commits December 28, 2024 03:04

Fix formatting

5df559a

Use begin instead of cbegin

26545b0

sakertooth reviewed Dec 28, 2024

View reviewed changes

src/gui/SampleThumbnail.cpp Outdated Show resolved Hide resolved

src/gui/SampleThumbnail.cpp Outdated Show resolved Hide resolved

sakertooth added 13 commits January 1, 2025 06:10

Improve generation of thumbnails

f522e1a

Improve expressiveness of the draw code

55c55c8

Add support for reversing

2476ce1

Fix drawing code

3bc7f3b

Fix draw code (part 2)

2a81387

Apply more fixes and simplifications

70a01b8

Undo some out of scope changes

fea47be

Remove SampleWaveform

ffe4915

Improve documentation

a9f9194

Use size_t for some counters

6f1becb

Change width parameter to be size_t

b6eeccb

Remove temporary aggregated peak variable

5a0ad5e

Bump up AggregationPerZoomStep to 10

3dd6217

sakertooth mentioned this pull request Jan 3, 2025

Fix corruption crash when zooming on a large sample #7577

Closed

sakertooth and others added 9 commits January 3, 2025 16:07

Zoom out only requested range of thumbnail instead of clipping it sep…

ae44632

…arately

Rename targetSampleWidth to targetThumbnailWidth

dd5dac1

Handle reversing for AFP; Iterate in reverse instead of reversing the…

3579edb

… entire thumbnail

Change names to be more accurate

5c9039d

Improve implementation of sample thumbnail cache map

7d5ba17

Move AggregationPerZoomStep back down to 2, do not cap smallest thumb…

5378902

…nail width to display width To improve performance with especially long samples (~20 minutes)

Simplify sample thumbnail cache handling in constructor

5952123

Call drawLines instead of drawLine in a loop

313af1e

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

141a20d

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.

messmerd reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sample thumbnails #7366

Add sample thumbnails #7366

khoidauminh commented Jul 4, 2024 •

edited by sakertooth

Loading

Rossmaxx commented Jul 4, 2024

khoidauminh commented Jul 4, 2024 •

edited

Loading

zachtyll commented Jul 11, 2024

khoidauminh commented Jul 12, 2024

sakertooth commented Jul 17, 2024 •

edited

Loading

khoidauminh commented Jul 17, 2024

sakertooth commented Sep 10, 2024 •

edited

Loading

sakertooth commented Dec 28, 2024

khoidauminh commented Jan 3, 2025

sakertooth commented Jan 3, 2025

messmerd Jan 6, 2025

sakertooth Jan 6, 2025

messmerd Jan 6, 2025

sakertooth Jan 6, 2025 •

edited

Loading

sakertooth Jan 6, 2025

messmerd Jan 6, 2025

sakertooth Jan 6, 2025 •

edited

Loading

messmerd Jan 6, 2025

sakertooth Jan 6, 2025

		const auto beginSample = buffer + static_cast<size_t>(std::floor(peakIndex * m_samplesPerPeak));
		const auto endSample = buffer + static_cast<size_t>(std::ceil((peakIndex + 1) * m_samplesPerPeak));

Add sample thumbnails #7366

Are you sure you want to change the base?

Add sample thumbnails #7366

Conversation

khoidauminh commented Jul 4, 2024 • edited by sakertooth Loading

Brief summary of the PR:

Rossmaxx commented Jul 4, 2024

khoidauminh commented Jul 4, 2024 • edited Loading

zachtyll commented Jul 11, 2024

khoidauminh commented Jul 12, 2024

sakertooth commented Jul 17, 2024 • edited Loading

khoidauminh commented Jul 17, 2024

sakertooth commented Sep 10, 2024 • edited Loading

sakertooth commented Dec 28, 2024

khoidauminh commented Jan 3, 2025

sakertooth commented Jan 3, 2025

messmerd Jan 6, 2025

Choose a reason for hiding this comment

sakertooth Jan 6, 2025

Choose a reason for hiding this comment

messmerd Jan 6, 2025

Choose a reason for hiding this comment

sakertooth Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

sakertooth Jan 6, 2025

Choose a reason for hiding this comment

messmerd Jan 6, 2025

Choose a reason for hiding this comment

sakertooth Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

messmerd Jan 6, 2025

Choose a reason for hiding this comment

sakertooth Jan 6, 2025

Choose a reason for hiding this comment

khoidauminh commented Jul 4, 2024 •

edited by sakertooth

Loading

khoidauminh commented Jul 4, 2024 •

edited

Loading

sakertooth commented Jul 17, 2024 •

edited

Loading

sakertooth commented Sep 10, 2024 •

edited

Loading

sakertooth Jan 6, 2025 •

edited

Loading

sakertooth Jan 6, 2025 •

edited

Loading