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

Remove the FIFO thread #7568

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Oct 31, 2024

This PR removes AudioEngine::fifoWriter, which is a thread that facilities the multiple buffering mechanism we have currently for our audio rendering. The way the mechanism works (to my understanding):

  1. The FIFO thread, when started, splits the buffer size into chunks of 256 sample frames and generates them as fast as it can.
  2. The FIFO thread stops generating audio when the number of chunks generated equal original_buffer_size / 256 and waits for someone to request a chunk.
  3. The main audio thread requests chunks from the FIFO and uses that for playback.
  4. While the main audio thread is requesting and receiving chunks, the FIFO thread continues rendering more data.

Pros to having this thread:

  1. Automation changes happen more regularly - Most processing in LMMS treat its parameters as having one value. That same value is used throughout the buffer. As a result, without the FIFO thread and the chunking of the full audio buffer, larger buffer sizes reduce the amount of automation that occurs through time.
    1. Sample-exactness can fix this for most parameters, which makes it so that parameters are treated as having values per sample in the buffer, rather than just one single value. There are complaints of having sample-exactness for most of our native parameters will cause major drops in performance, which is why removing this thread may not be that straightforward. Note: In the case of VST2 parameters, I believe sample-exactness cannot be applied, as sample-exact automation was added in VST3. This is to show that inevitably, not all the parameters can be made sample-exact, and most professional DAWs deal with this same issue and also have problems involving automation timing accuracy.
    2. I fixed this problem but was still able to remove the FIFO thread by doing the chunking on the callback thread.

Cons to having this thread:

  1. Export bugs - This was the main reason why I wanted to remove this thread. The communication between the main audio thread, the export thread, and the FIFO thread is complex and has lead to deadlocks when exporting like in Constant freezing at 0% during export #7320. Trying to fix the FIFO thread and remove the deadlock does not feel like a good use of time considering that it really shouldn't be there in the first place if things were more correctly implemented in the beginning.
  2. Higher buffer sizes != more performance - For reasons I am currently unaware of, higher buffer sizes do not necessarily imply more performance when using the FIFO thread (at least in this instance). I discovered this when I was investigating the infamous "automating VST knobs" bottleneck. The video I attached demonstrates a project automating a VST knob with an LFO controller with this branch merged in my build. In the video, you can see how having a buffer size of 1024 samples keeps the CPU meter at acceptable levels, while in contrast a buffer size of 256 samples has a bad impact on performance.
    1. This improvement in performance was only seen when this thread was removed and the main audio thread was directly rendering the audio buffers. Current master exhibits bad performance regardless of what buffer size you select in the settings when automating VST knobs, while here the situation is less of a problem if a higher buffer size is selected.
    2. Note: I checked out master and moved into a new branch in the video for some reason, but merged this branch and nothing else. I was planning to do a real fix, but @DomClark seems to be working on it already, and I quickly came up with the idea to see if this PR would help performance, so I merged it in and it did.

The solution now is to keep the chunking of the buffer size and do it on the audio callback thread, and still remove the FIFO thread.

output.mp4

@sakertooth
Copy link
Contributor Author

Changed the PR to still remove the FIFO thread but keep the chunking of the buffer size (to avoid problems with non sample accurate automation but to also fix the problems this thread has caused).

@sakertooth sakertooth marked this pull request as ready for review December 17, 2024 16:10
@sakertooth sakertooth added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant