Skip to content
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

Implement sample caching #6390

Closed
wants to merge 48 commits into from
Closed

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented May 2, 2022

The goal of this PR is to implement an LRU cache for the SampleBuffer class. The data within the cache will also be able to share SampleBuffer's between users, allowing for improved memory efficiency.

The refactoring that previously underwent this PR has been moved into another PR. This should make this PR quite simple and straight to the point.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented May 2, 2022

I think it would be better to do the sample workflow/performance improvements first and then sample buffer can be done as a different pr at a later time. This is just my opinion.

@sakertooth
Copy link
Contributor Author

I think it would be better to do the sample workflow/performance improvements first and then sample buffer can be done as a different pr at a later time. This is just my opinion.

For the sample workflow to be improved, the SampleBuffer, quite frankly, needs to be changed. Improvements in SampleBuffer lead to an improvement in sample workflow. They go hand in hand. Its just that this PR is not solely for sample caching. Theres more to it (such as performance improvements when visualizing samples).

@Rossmaxx
Copy link
Contributor

Rossmaxx commented May 2, 2022

Oh i see. Btw does this pr fix the abnormal memory / cpu usage on sample track.

@sakertooth
Copy link
Contributor Author

sakertooth commented May 2, 2022

Oh i see. Btw does this pr fix the abnormal memory / cpu usage on sample track.

Indeed. I put a milestone video that shows me putting 18 sample tracks with the memory hovering around ~170 MB. The sample is only loaded once and is shared after that throughout the project. It will not matter if you are in the AFP (still has to be integrated with the new Sample), a sample clip, etc. It will load it one time and share it using the cache. The cache doesnt make duplicates of the samples either.

In a previous iteration of that milestone video I played the sample tracks but that obviously was not ideal, so I just left it be.

@LmmsBot
Copy link

LmmsBot commented May 2, 2022

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/cef3bfe4-4fd7-443e-94ef-3eb73316107f/artifacts/0/lmms-1.3.0-alpha.1.193+g59c80d022-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17093?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/22c17497-b5e6-4c1e-9ca4-8049b36d69f0/artifacts/0/lmms-1.3.0-alpha.1.193+g59c80d022-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17090?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/4keiu020mq0naxkv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43774634"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/qb01634o59wmrqpa/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43774634"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/c140b1d6-f9f1-43b6-a32c-ab3603d4ba40/artifacts/0/lmms-1.3.0-alpha.1.193+g59c80d022-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17092?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/3b672d58-ed99-4e05-bf82-a28167d3163f/artifacts/0/lmms-1.3.0-alpha.1.193+g59c80d022-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17091?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "917a6f644e0c0b6b1682f81952bb77b657310bcf"}

@Rossmaxx
Copy link
Contributor

Rossmaxx commented May 2, 2022

i tried out this build and found some issues.

  1. the sample track colour seems broken
  2. the theming is broken while using custom themes

https://youtu.be/4UYxu4BxuGg here is the video where i did the test

the sample track performance seemed to have improved.
will do further testing and tell you more.

@sakertooth
Copy link
Contributor Author

i tried out this build and found some issues.

  1. the sample track colour seems broken
  2. the theming is broken while using custom themes

https://youtu.be/4UYxu4BxuGg here is the video where i did the test

the sample track performance seemed to have improved. will do further testing and tell you more.

Visualization of samples is on the back-burner for now. I will get to it soon though (unless somebody else would like to contribute).
I'm looking towards changing the playing of samples up a bit and making resampling faster (resampling takes time).

@sakertooth sakertooth changed the title Sample caching and general improvement in workflow with samples Sample sharing and general improvement in workflow with samples May 2, 2022
@Spekular
Copy link
Member

Spekular commented May 7, 2022

Why are you setting the minimum length of a clip to one bar again? This was intentionally changed in #4933 to allow e.g. the use of sample tracks for percussion. With sample caching implemented this would become even more feasible, but setting samples to a bar long makes it less so.

@sakertooth
Copy link
Contributor Author

sakertooth commented May 7, 2022

Why are you setting the minimum length of a clip to one bar again? This was intentionally changed in #4933 to allow e.g. the use of sample tracks for percussion. With sample caching implemented this would become even more feasible, but setting samples to a bar long makes it less so.

It was just that when I was importing certain samples of a small duration, it would make it seem as if it did not import at all. Reverting.

At 100% zoom, it becomes difficult to see the TCO if it is at minimum size, but this already occurs with BB Track TCOs, so I wouldn't consider it a regression.

Ah, I see. Nevermind then.

@sakertooth sakertooth changed the title Sample sharing and general improvement in workflow with samples Sample sharing and general improvement in performance with samples May 12, 2022
@sakertooth sakertooth changed the title Sample sharing and general improvement in performance with samples Implement sample sharing May 18, 2022
sakertooth added a commit to sakertooth/lmms that referenced this pull request May 19, 2022
This implements a framework for the caching/sharing of samples.
Reintegration still needs to occur, so not everything is complete.
This is just to simplify the changes.

SampleBufferV2 is where samples and any pertaining data to that sample
is stored. From there, Sample's use that buffer but do not change it.
They do that by storing a std::shared_ptr<const SampleBufferV2>,
which gives read-only access to the buffer.

SampleBufferCache tracks the samples used in a project from a file or
from Base64. If something needs a certain sample that is tracked by
this cache, it will just create a std::shared_ptr from that tracked
sample.

The tracking is done with std::weak_ptr's within the SampleBufferCache.
@PhysSong PhysSong self-requested a review May 20, 2022 02:25
@sakertooth
Copy link
Contributor Author

The ambiguity between sample sharing and sample caching is somewhat problematic. While the samples are infact shared (no duplicates of the same SampleBufferV2 derived from a file or Base64 are made), key entries of std::weak_ptr are cached, which come from the first std::shared_ptr using that SampleBufferV2. Sharing comes from the usage of std::weak_ptr::lock.

Ergo, I think I will just stick to sample caching. Ultimately, the entries are cached, but the sample data itself is shared. Plus, I cannot think of a better fitting name for SampleBufferCache.

@sakertooth sakertooth changed the title Implement sample sharing Implement sample caching May 25, 2022
@DomClark DomClark self-requested a review May 28, 2022 12:01
sakertooth added a commit to sakertooth/lmms that referenced this pull request Jun 6, 2022
This implements a framework for the caching of samples.

SampleBufferV2 is where samples and any pertaining data to that sample
is stored. From there, Sample's use that buffer but do not change it.
They do that by storing a std::shared_ptr<const SampleBufferV2>,
which gives read-only access to the buffer.

SampleBufferCache tracks the samples used in a project from a file or
from Base64. If something needs a certain sample that is tracked by
this cache, it will just create a std::shared_ptr from that tracked
sample.

The tracking is done with std::weak_ptr's within the SampleBufferCache.
@sakertooth sakertooth force-pushed the samplecaching branch 2 times, most recently from 4f5d5bd to 917a6f6 Compare June 6, 2022 20:59
sakertooth added a commit to sakertooth/lmms that referenced this pull request Jun 8, 2022
@PhysSong
Copy link
Member

PhysSong commented Jun 9, 2022

It seems like only std::experimental::filesystem is available(in <experimental/filesystem>) prior to GCC 8 on Linux and XCode 11 on macOS.
Also, on Windows, std::filesystem::path::value_type is wchar_t, so you will have to use sf_wchar_open on Windows(tested by #ifdef LMMS_BUILD_WIN32).

@sakertooth sakertooth marked this pull request as draft June 9, 2022 12:19
@sakertooth sakertooth changed the title Implement sample caching Rework SampleBuffer and implement sample caching Dec 4, 2022
+ SampleBufferV2 is a QObject so that it can resample itself accordingly to the sample rate changes

+ Some functionality, such as loop points, that were once used in Sample has been removed. The way Sample is made allows you to loop sections already by calling setFrameIndex when necessary

+ The loadSampleFile and loadBase64 functions in Sample have been replaced with its constructor

+ The original sample rate and current sample rate have been distinguished in SampleBufferV2
+ Make rescaleMarkers function
+ Remove procedure comments
+ Add a newline at the very end where needed
Using QString for simplicity reasons for now
@sakertooth sakertooth marked this pull request as ready for review December 4, 2022 22:14
@sakertooth sakertooth requested review from PhysSong and removed request for DomClark December 4, 2022 22:28
Before, calling Sample::play would create audible artifacts on playback
after reversing. This was fixed by using an intermediate sample buffer
to do any last changes before copying it into dst, such as resampling.

The interpolation of the resampling occuring in
Sample::play is now SRC_SINC_FASTEST, as SRC_LINEAR would make
the audio quality poorer, as stated in Secret Rabbit's Code's
documentation. The code also now returns early when an error
occurs while resampling, instead of moving forward in an erroneous
state.
After realizing that SampleFileDialog::openSampleFile
is only used in one location, the SampleClipView, theres not much
reason to have an extra file just for opening sample files.
If the current state of SampleBufferV2 was given an audio file
that has more than 2 channels, it would only grab the first 2
samples from each frame. This is an erroneous mixdown to stereo,
as the other samples in the frame are not considered.

To fix this, SampleBufferV2 now has mix up and down to stereo functions,
which are meant to properly handle audio files with more than 2
channels, and simplify the mixing up and down process where the code
is more understandable and readable.

The mixing up to stereo from mono functionality was present already,
but was made more readable with the addition of mixMonoToStereo, rather
than a ternary operator.
We can use the auxillary function from Secret Rabbit Code to make
the short* to float* conversion more readable.
@sakertooth sakertooth changed the title Rework SampleBuffer and implement sample caching Implement sample caching Dec 25, 2022
@sakertooth sakertooth marked this pull request as draft January 14, 2023 22:52
@sakertooth
Copy link
Contributor Author

Closing in favor of #6703

@sakertooth sakertooth closed this May 4, 2023
@sakertooth sakertooth deleted the samplecaching branch December 31, 2023 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants