lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Real time safe recording with ring buffer stage one

Open steven-jaro opened this issue 8 months ago • 19 comments

This is the first stage of the implementation of the ring buffer for real time safety when recording as this is needed to solve issue number 4 in #7786. Issue 4 says that recording is not safe with the current method. Here are the changes made to the code in this PR:

  1. AudioEngine.h:
  • Included "#include "LocklessRingBuffer.h"
  • Added needed ringbuffer variables.
  • Added new function "processBufferedInputFrames".
  1. AudioEngine.cpp:
  • Included: #include #include #include #include #include "LocklessRingBuffer.h"
  • Created static constant for the buffer size: static const f_cnt_t FIXED_INPUT_BUFFER_CAPACITY = DEFAULT_BUFFER_SIZE * 100;
  • In the constructor, we make use of the constant "DEFAULT_BUFFER_SIZE". Changed the name of "wt" to "workerThread". Added the initialization in the constructor for the 3 ringbuffer needed variables.
  • Added code to the destructor to safely delete the worker threads.
  • Changed "pushInputFrames" function to write in the ringbuffer.
  • Created new function "void AudioEngine::processBufferedInputFrames()". This function runs continuously. First checks if there is data in the ringbuffer, if not, it returns. Then reads the data read in the ringbuffer into two halfs so that data can be written continously. Then it writes the data of the two halfs from the ringbuffer to m_inputBufferFrames.
  1. AudioDevie.h:
  • Added function "isProcessing()" to check the value of m_inProcess. This is so that the destructor can check if "m_inProcess" is true so that it closes the processing correctly.

How to test this PR?" This PR just makes that recording is memmory safe, so one should look for memmory leaks, segfaults or possible reallocatio in the code.

steven-jaro avatar May 23 '25 20:05 steven-jaro

No downloable .exe

firiox avatar May 23 '25 21:05 firiox

Oh yeah that's because the workflows weren't running. They're going now, so an exe should be available in the next 15-20 minutes.

regulus79 avatar May 23 '25 21:05 regulus79

Ok how I test It dude? . .

firiox avatar May 23 '25 22:05 firiox

Ok how I test It dude? . .

https://lmms.io/download/pull-request/7903

tresf avatar May 24 '25 05:05 tresf

Works well but i get these occasional crackles and the audio peaks. image

edit: Audacity doesn't have this issue

AW1534 avatar May 24 '25 11:05 AW1534

Works well but i get these occasional crackles and the audio peaks. image

edit: Audacity doesn't have this issue

Hey, thanks for the feedback.

steven-jaro avatar May 24 '25 13:05 steven-jaro

I changed the approach as it was pretty unstable. Please feel free to test the code and review it. My new approach also is supposed to fix the random segfaults and clipping audio. I also tried to fix all the naming issues with the variables for the camelCase, if some are still remaining, I will fix it in the next days.

steven-jaro avatar May 25 '25 14:05 steven-jaro

image You fixed it, great job! Still seems to run smoothly too, haven't had any crashes or bugs

AW1534 avatar May 25 '25 17:05 AW1534

image You fixed it, great job! Still seems run smoothly too, haven't had any crashes or bugs

Thank you for the feedback and testing.

steven-jaro avatar May 25 '25 17:05 steven-jaro

My most recent push has the intent to remove all the allocation I could think of to get more real time safety. I think there is still some points with non real time safety, but at least I would like to get feedback as I consider this a progress and not the final deal.

steven-jaro avatar May 25 '25 22:05 steven-jaro

Help me understand this code...

Before this PR, were we using a single buffer m_inputBuffer[m_inputBufferWrite] for recording audio which would just be scaled up 2x every time it got filled up?

And this PR replaces that with a ring buffer.... which is slowly filled up and copied over to the other buffer ?

regulus79 avatar May 26 '25 00:05 regulus79

Help me understand this code...

Before this PR, were we using a single buffer m_inputBuffer[m_inputBufferWrite] for recording audio which would just be scaled up 2x every time it got filled up?

And this PR replaces that with a ring buffer.... which is slowly filled up and copied over to the other buffer ?

Hello, this is the explaination of the code for this PR. First I will answer your questions:

Before this PR, were we using a single buffer "m_inputBuffer[m_inputBufferWrite]" for recording audio which would just be scaled up 2x every time it got filled up? R/ Basically yes. Here is how old recording used to work: a. The function "AudioEngine::pushInputFrames()" was called directly by the os audio driver's callback. b. It would directly write incoming audio frames to "m_inputBuffer[m_inputBufferWrite]". c. If the space "m_inputBuffer[m_inputBufferWrite]" wasn't enough anymore, "pushInputFrames" would: Lock a mutex using "requestChangeInModel()", ALLOCATE a new larger buffer, copy data from the old buffer to the new one and delete the old buffer and the lock the mutex again.

All of that is not real time safe as we were allocating with "new" and deallocating with "delete[]".

"And this PR replaces that with a ring buffer.... which is slowly filled up and copied over to the other buffer ?" R/ Yes. My most recent approach does this: a. "AudioEngine::pushInputFrames()" is called by the audio driver.

  • But now it would only write incoming audio to the lockless ring buffer "m_inputAudioRingBuffer".
  • The functions lightweight as it doesn't perform a mutex locking or allocation. If the ring buffer is full, it drops the frames which is better than allocating.

b. I added the function "AudioEngine::processBufferedInputFrames()".

  • It is called from "AudioEngine::renderStageNoteSetup()" which is called continously.
  • It first checks if "m_inputAudioRingBuffer" has new data.
  • Then it copies the ring buffer into "m_inputBuffer[m_inputBufferWrite]".
  • It is safe as it doesn't run in a less critical context than the OS calling it directly. So now it can use "requestChangeInModel()" safely.
  • "m_inputBuffer[0]" and "m_inputBuffer[1]" are now allocanted one time by the constructor to a fixed capacity. So now "processBufferedInputFrames" doesn't do reallocation.
  • "std::vector<SampleFrame> m_tempInputProcessingBuffer" is used to consolidate the data from the ring buffer. The vector is reserver in the constructor to a capacity that is based on the ring buffer size. In "processBufferedInputFrames" I use "resize()" but to the actual number of frames that are being processed. And this is withing the reserved capacity so we are not doing heal reallocations.

steven-jaro avatar May 26 '25 17:05 steven-jaro

I solved the problems with the resize and reserve. Now it is supposed to be completely real time safe.

steven-jaro avatar May 30 '25 00:05 steven-jaro

I solved the problems with the resize and reserve. Now it is supposed to be completely real time safe.

OK, I will look later at it. But please, can you fix the indentation issues first? Thanks!

JohannesLorenz avatar May 30 '25 08:05 JohannesLorenz

All changes requested will be applied. But I just have that doubt with the and statement.

steven-jaro avatar May 30 '25 12:05 steven-jaro

All the resquests were applied.

steven-jaro avatar May 30 '25 18:05 steven-jaro