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

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

sakertooth
Copy link
Contributor

A PR was made for this branch mainly for developers and users to see the effects removing the FIFO thread will have, either good or bad. The choice for merge, as with all PRs, is at the discretion of those who tested it and the overall community. I am not necessarily pushing a merge here. As I will discuss, there are a few gray areas involving this whole situation, and I think being able to see the changes yourself can clear things up a bit for everyone.

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.

This was the only main benefit I could think of as of right now: this thread acts as a bandage for maintaining somewhat accurate automation regardless of buffer sizes. Let me know if I missed something.

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.

Other notes:

  1. I limited the maximum buffer size to 1024 samples in this PR, as higher buffer sizes can cause a choppy feel in the GUI, which might throw users off that are familiar with the current behavior (because even with the highest buffer size, the FIFO thread also maintains a consistent, smooth feel in the GUI that isn't choppy, which might be another benefit depending on how you look at it)
output.mp4

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.

1 participant