Stream sample previews instead of loading all contents into memory

[sakertooth]

This PR adds streaming support for sample files. This means that sample files can be streamed in instead of being loaded all at once into memory. This is especially useful for when you do not necessarily depend on all the sample data, but as they are processed. Currently, only sample previews in the FileBrowser make use of sample streams. This should help improve performance when previewing large sample files.

[tresf]

This works great for previewing longer samples. Sadly, something about this branch is buggy when testing with Sample Tracks. It crashes just about every time I try it. I was unable to reproduce this crash on master.

stream.mov

[sakertooth]

This works great for previewing longer samples. Sadly, something about this branch is buggy when testing with Sample Tracks. It crashes just about every time I try it. I was unable to reproduce this crash on master.

stream.mov

Odd.. how unfortunate. I'll draft this PR and take a look when I get the chance. I need to deal with this crash as well as figuring out the code overlap between the SampleFile class I added here and the classes that handle exporting.

[tresf]

This works great for previewing longer samples. Sadly, something about this branch is buggy when testing with Sample Tracks. It crashes just about every time I try it. I was unable to reproduce this crash on master.

sakertooth requested a review from tresf 8 hours ago

I re-tested (everything still works as described) but I can no longer reproduce the crashes with Sample Tracks!

[tresf]

Approving from a testing perspective. Would appreciate at least one C++ review as well.

[bratpeki]

I've been summoned! How do I test this?

[sakertooth]

I've been summoned! How do I test this?

Try to play a couple of your bigger audio files. See if it takes less time to play it. Also try to consider crashes, bugs, etc.

[andboss25]

Tested it , works perfectly 👍

[szeli1]

Ideally this should be implemented in SampleBuffer in the future. How can this be done? I would rather not approve this until there is a plan to implement this for SampleBuffer.

I did not test this PR.

Please request an other review from me when you answered my questions.

[sakertooth]

Ideally this should be implemented in SampleBuffer in the future. How can this be done? I would rather not approve this until there is a plan to implement this for SampleBuffer.

SampleBuffer can load an audio file from memory, but everything in that file is loaded at once. This doesn't do that, so it doesn't make much sense to implement it for SampleBuffer AFAICT.

This new class handles streaming from an audio file by reading it in chunks on another thread while the reader (audio thread) reads data from that buffer. This also handles sample rate conversion.

[szeli1]

This means that sample files can be streamed in instead of being loaded all at once into memory. This is especially useful for when you do not necessarily depend on all the sample data, but as they are processed. Currently, only sample previews in the FileBrowser make use of sample streams

It would be ideal if large audio files were not stored in memory for Sample clips. I think this ought to be designed in a way that will allow streaming for sample clips. This feature will make previews in the FileBrowser more memory efficient and faster to read, but if the user decides to use those audio files, the memory efficiency problem will still be there. If you would rather avoid storing journal states in strings, then this could be improved upon also.

[sakertooth]

It would be ideal if large audio files were not stored in memory for Sample clips. I think this ought to be designed in a way that will allow streaming for sample clips. This feature will make previews in the FileBrowser more memory efficient and faster to read, but if the user decides to use those audio files, the memory efficiency problem will still be there.

We could theoretically stream all sample files in during processing. However, as more sample clips are generated for example, there is more thread contention because 200 sample clips will need 200 writer tasks (these are tasks designated for reading from a sample file and into the shared audio buffer), and they can all be competing to run on a limited space of 4 cores on the machine, not to mention that these threads are not running with priority (which makes me question if the solution in this PR is even real-time safe, but maybe we can get away with doing this? or maybe this PR was a bad idea after all).

If you would rather avoid storing journal states in strings, then this could be improved upon also.

I am pretty sure that this PR is not related to the JO system at all.

Edit: The issues I mentioned above are a bit more relaxed as they seem (maybe besides the priority problem) because only one preview should happen at a time.

[bratpeki]

Tested with MP3, WAV, FLAC and OGG. Works great! Approving.

---

I noticed an issue, though. MP3 files don't appear in the "Root Directory" or "My Home" tabs of the browser for me. @sakertooth could you confirm?

Master:

PR:

Maybe it's down to AppImages not supporting MP3?

Confirmed, it's the AppImages! PR looks good, merge ASAP! 🙏

[szeli1]

We could theoretically stream all sample files in during processing. However, as more sample clips are generated for example, there is more thread contention because 200 sample clips will need 200 writer tasks (these are tasks designated for reading from a sample file and into the shared audio buffer), and they can all be competing to run on a limited space of 4 cores on the machine, not to mention that these threads are not running with priority

My idea was that only 1 GB < samples were streamed, others were loaded in to memory. Also a slider in the settings could be added that controls the size limit that decides if a sample is loaded in or streamed.

[sakertooth]

We could theoretically stream all sample files in during processing. However, as more sample clips are generated for example, there is more thread contention because 200 sample clips will need 200 writer tasks (these are tasks designated for reading from a sample file and into the shared audio buffer), and they can all be competing to run on a limited space of 4 cores on the machine, not to mention that these threads are not running with priority

My idea was that only 1 GB < samples were streamed, others were loaded in to memory. Also a slider in the settings could be added that controls the size limit that decides if a sample is loaded in or streamed.

This does not scale. I don't think this idea is a good one for the reasons I already mentioned.

[szeli1]

This does not scale. I don't think this idea is a good one for the reasons I already mentioned.

How would you fix the large audio file taking up too much memory issue?

[sakertooth]

Don't assume that this streaming is for free. You are definitely paying something here.

[sakertooth]

And remember, the entire thumbnail will have be generated either way, so we don't really have a choice, but at that point we've already loaded all the data in anyways.

[bratpeki]

It's on my list for this month, testing now!

[bratpeki]

Please merge ASAP

[messmerd]

I don't fully understand SamplePreviewPlayHandle::play or SamplePreviewPlayHandle::runDiskStream yet, so this is mostly a code review for the rest of the PR

[sakertooth]

Thank you all for the testing/reviews. There are a few things I want to change though.

[szeli1]

I don't want to approve this until there is a clear plan how can this be implemented for Sample clips

[sakertooth]

I don't want to approve this until there is a clear plan how can this be implemented for Sample clips

I agree. I think I can set some stuff up in a way such that it is usable in the future for things like sample clips, the AFP, other plugins, etc.

[szeli1]

I agree. I think I can set some stuff up in a way such that it is usable in the future for things like sample clips, the AFP, other plugins, etc.

Please request me to review if you are finished that.

[messmerd]

Quick review

[sakertooth]

I've noticed I do very similar stuff that is already within #7858, so I think I will wait for that PR to later incorporate it here.